[PATCH] add: SetEnvironmentStringsW
Zebediah Figura
z.figura12 at gmail.com
Tue Mar 17 20:39:05 CDT 2020
On 3/17/20 3:13 PM, Frank Uhlig wrote:
> On Thu, Mar 12, 2020 at 3:25 AM Zebediah Figura <zfigura at codeweavers.com> wrote:
>>
>> On 3/11/20 4:50 PM, Frank Uhlig wrote:
>>> Sorry for the late reply, but have finally gotten back to this patch.
>>>
>>> On Thu, Feb 20, 2020 at 10:35 PM Zebediah Figura <z.figura12 at gmail.com> wrote:
>>>>
>>>> On 2/20/20 3:12 PM, Frank Uhlig wrote:
>>>>> Hi Zebediah, thanks for looking into this. Please find my responses
>>>>> inline below. I've already incorporated many things, but still have
>>>>> some remaining questions and comments.
>>>>>
>>>>> I would re-submit it soon, and include a "versioning" in the mail tag
>>>>> as I've seen on other submissions (i.e., [PATCH v2]).
>>>>>
>>>>> On Mon, Feb 10, 2020 at 4:38 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
>>>>>>
>>>>>> Hello Frank, thanks for the patch.
>>>>>>
>>>>>> With regard to the title, convention is to put the name of the component
>>>>>> first, followed by a colon, then a verb describing what the patch does,
>>>>>> so e.g. "kernelbase: Add SetEnvironmentStringsW()."
>>>>>>
>>>>>
>>>>> done.
>>>>>
>>>>>> I have a few more comments inlined below which might help improve the patch:
>>>>>>
>>>>>> On 1/22/20 3:33 PM, Frank Uhlig wrote:
>>>>>>> From: Frank Uhlig <fuulish at users.noreply.github.com>
>>>>>>>
>>>>>>> Signed-off-by: Frank Uhlig <uhlig.frank at gmail.com>
>>>>>>> ---
>>>>>>> ...ms-win-core-processenvironment-l1-1-0.spec | 2 +-
>>>>>>> ...ms-win-core-processenvironment-l1-2-0.spec | 2 +-
>>>>>>> dlls/kernel32/kernel32.spec | 2 +-
>>>>>>> dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++
>>>>>>> dlls/kernelbase/kernelbase.spec | 2 +-
>>>>>>> dlls/kernelbase/process.c | 29 +++++++++
>>>>>>> include/winbase.h | 1 +
>>>>>>> 7 files changed, 95 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
>>>>>>> index e3698d6efd..7a62b74390 100644
>>>>>>> --- a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
>>>>>>> +++ b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
>>>>>>> @@ -15,7 +15,7 @@
>>>>>>> @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
>>>>>>> @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
>>>>>>> @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
>>>>>>> -@ stub SetEnvironmentStringsW
>>>>>>> +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
>>>>>>> @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
>>>>>>> @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
>>>>>>> @ stdcall SetStdHandle(long long) kernel32.SetStdHandle
>>>>>>> diff --git a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
>>>>>>> index 2c25ee1a07..c93d221c5e 100644
>>>>>>> --- a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
>>>>>>> +++ b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
>>>>>>> @@ -17,7 +17,7 @@
>>>>>>> @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
>>>>>>> @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
>>>>>>> @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
>>>>>>> -@ stub SetEnvironmentStringsW
>>>>>>> +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
>>>>>>> @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
>>>>>>> @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
>>>>>>> @ stdcall SetStdHandle(long long) kernel32.SetStdHandle
>>>>>>> diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
>>>>>>> index be48ef1694..2b74a4182e 100644
>>>>>>> --- a/dlls/kernel32/kernel32.spec
>>>>>>> +++ b/dlls/kernel32/kernel32.spec
>>>>>>> @@ -1387,7 +1387,7 @@
>>>>>>> # @ stub SetDynamicTimeZoneInformation
>>>>>>> @ stdcall -import SetEndOfFile(long)
>>>>>>> # @ stub SetEnvironmentStringsA
>>>>>>> -# @ stub SetEnvironmentStringsW
>>>>>>> +@ stdcall -import SetEnvironmentStringsW (ptr)
>>>>>>> @ stdcall -import SetEnvironmentVariableA(str str)
>>>>>>> @ stdcall -import SetEnvironmentVariableW(wstr wstr)
>>>>>>> @ stdcall -import SetErrorMode(long)
>>>>>>> diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
>>>>>>> index 44a6a0cff0..53b2803e77 100644
>>>>>>> --- a/dlls/kernel32/tests/environ.c
>>>>>>> +++ b/dlls/kernel32/tests/environ.c
>>>>>>> @@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void)
>>>>>>> FreeEnvironmentStringsW(env2);
>>>>>>> }
>>>>>>>
>>>>>>> +static void test_SetEnvironmentStringsW(void)
>>>>>>> +{
>>>>>>> + DWORD buf_len;
>>>>>>> + BOOL ret;
>>>>>>> + DWORD ret_size;
>>>>>>> +
>>>>>>> + static WCHAR buf[256];
>>>>>>> +
>>>>>>> + static WCHAR name[] = {'N','a','m','e',0};
>>>>>>> + static WCHAR value[] = {'V','a','l','u','e',0};
>>>>>>> + static WCHAR env[] = {'N','a','m','e','=','V','a','l','u','e',0};
>>>>>>> +
>>>>>>> + static WCHAR eman[] = {'e','m','a','N',0};
>>>>>>> + static WCHAR eulav[] = {'e','u','l','a','V',0};
>>>>>>> + static WCHAR vne[] = {'e','m','a','N','=','e','u','l','a','V',0};
>>>>>>> +
>>>>>>> + static WCHAR var[] = {'V','a','r',0};
>>>>>>> + static WCHAR val[] = {'V','a','l',0};
>>>>>>> + static WCHAR rav[] = {'r','a','V',0};
>>>>>>> + static WCHAR lav[] = {'l','a','V',0};
>>>>>>> + static WCHAR mul[] = {'V','a','r','=','V','a','l',' ','r','a','V','=','l','a','V',0};
>>>>>>> +
>>>>>>> + static WCHAR empty[] = {'V','a','r','=',0};
>>>>>>
>>>>>> You can use wide character string literals in tests (which should also
>>>>>> allow you to get rid of some of these declarations).
>>>>>>
>>>>>
>>>>> Didn't change this, b/c I was trying to keep the same style as already
>>>>> there and described on the website
>>>>> (https://wiki.winehq.org/Developer_Hints#Using_only_C89-compliant_code).
>>>>
>>>> The wiki is out of date on this particular topic. Someone™ should
>>>> probably update it.
>>>>
>>>>>
>>>>>> I suspect that you also want either local variables or (when applicable)
>>>>>> "static const". There's no point declaring non-constant variables as static.
>>>>>>
>>>>>
>>>>> Agreed, but kept as is to be consistent; and my test code is now not
>>>>> changing the input anymore.
>>>>
>>>> To be consistent with what? There may be existing code that uses bad
>>>> style, but that's not really a good reason to emulate it.
>>>>
>>>
>>> consistent style is less prone to errors. but further, I prefer the
>>> array initialization b/c of the visible double-0 termination of the
>>> strings.
>>> and yes, the static was a remnant of a previous static const, and got removed.
>>>
>>>>>
>>>>>>> +
>>>>>>> + buf_len = sizeof(buf) / 2;
>>>>>>
>>>>>> I personally think using an extra variable is redundant here. I also
>>>>>> think "ARRAY_SIZE(buf)" would be more expressive.
>>>>>>
>>>>>
>>>>> Changed to ARRAY_SIZE(buf)
>>>>>
>>>>>>> +
>>>>>>> + ret = SetEnvironmentStringsW(env);
>>>>>>> + ok( ret, "Setting environment strings failed\n" );
>>>>>>> +
>>>>>>> + ret_size = GetEnvironmentVariableW(name, buf, buf_len);
>>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, value))),
>>>>>>> + "Environment String settings resulted in different value\n");
>>>>>>
>>>>>> With all of these tests I think it would be helpful to print the actual
>>>>>> value of "ret_size" and contents of "buf" in the case the test fails. I
>>>>>> would also recommend testing "ret_size" against the exact expected
>>>>>> length, and perhaps splitting the two tests into two separate ok() calls.
>>>>>>
>>>>>
>>>>> Made the checks more specific, and included more info in the output
>>>>>
>>>>>>> +
>>>>>>> + ret = SetEnvironmentStringsW(vne);
>>>>>>> + ok( ret, "Setting environment strings failed\n" );
>>>>>>> +
>>>>>>> + ret_size = GetEnvironmentVariableW(eman, buf, buf_len);
>>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, eulav))),
>>>>>>> + "Environment String settings resulted in different value\n");
>>>>>>> +
>>>>>>> + ret = SetEnvironmentStringsW(mul);
>>>>>>> + ok( ret, "Setting environment strings failed\n" );
>>>>>>> +
>>>>>>> + ret_size = GetEnvironmentVariableW(var, buf, buf_len);
>>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, val))),
>>>>>>> + "Environment String settings resulted in different value\n");
>>>>>>> +
>>>>>>> + ret_size = GetEnvironmentVariableW(rav, buf, buf_len);
>>>>>>> + ok( ((0 != ret_size) && (0 == wcscmp(buf, lav))),
>>>>>>> + "Environment String settings resulted in different value\n");
>>>>>>> +
>>>>>>> + ret = SetEnvironmentStringsW(empty);
>>>>>>> + ok( ret, "Setting environment strings failed\n" );
>>>>>>> +
>>>>>>> + ret_size = GetEnvironmentVariableW(var, buf, buf_len);
>>>>>>> + ok( (0 == ret_size),
>>>>>>> + "Environment String settings resulted in different value\n");
>>>>>>> +
>>>>>>> +}
>>>>>>> +
>>>>>>> START_TEST(environ)
>>>>>>> {
>>>>>>> init_functionpointers();
>>>>>>> @@ -591,4 +651,5 @@ START_TEST(environ)
>>>>>>> test_GetComputerNameExA();
>>>>>>> test_GetComputerNameExW();
>>>>>>> test_GetEnvironmentStringsW();
>>>>>>> + test_SetEnvironmentStringsW();
>>>>>>> }
>>>>>>> diff --git a/dlls/kernelbase/kernelbase.spec b/dlls/kernelbase/kernelbase.spec
>>>>>>> index f36d4d525c..a5e30b938f 100644
>>>>>>> --- a/dlls/kernelbase/kernelbase.spec
>>>>>>> +++ b/dlls/kernelbase/kernelbase.spec
>>>>>>> @@ -1422,7 +1422,7 @@
>>>>>>> @ stdcall SetDefaultDllDirectories(long)
>>>>>>> # @ stub SetDynamicTimeZoneInformation
>>>>>>> @ stdcall SetEndOfFile(long)
>>>>>>> -@ stub SetEnvironmentStringsW
>>>>>>> +@ stdcall SetEnvironmentStringsW(ptr)
>>>>>>> @ stdcall SetEnvironmentVariableA(str str)
>>>>>>> @ stdcall SetEnvironmentVariableW(wstr wstr)
>>>>>>> @ stdcall SetErrorMode(long)
>>>>>>> diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c
>>>>>>> index a07dddb1fc..97b59b9548 100644
>>>>>>> --- a/dlls/kernelbase/process.c
>>>>>>> +++ b/dlls/kernelbase/process.c
>>>>>>> @@ -1345,6 +1345,35 @@ BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentVariableW( LPCWSTR name, LPCWSTR val
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> +/***********************************************************************
>>>>>>> + * SetEnvironmentStringsW (kernelbase.@)
>>>>>>> + */
>>>>>>> +BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentStringsW( LPWCH NewEnvironment )
>>>>>>
>>>>>> "WCHAR *" would probably be preferred.
>>>>>>
>>>>>
>>>>> Any particular reason for this? That's exactly what LPWCH is defined as.
>>>>
>>>> Broadly, use of pointer typedefs tends to be dispreferred in new code. I
>>>> also suspect that "env" would be nicer than "NewEnvironment".
>>>>
>>>
>>> OK, fine by me.
>>>
>>>
>>>>>>> +{
>>>>>>> +
>>>>>>
>>>>>> Stray newline.
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>>>> + BOOL rc = FALSE;
>>>>>>> + WCHAR *var, *val;
>>>>>>> + const WCHAR delim[] = {' ','=','\n',0};
>>>>>>> +
>>>>>>> + TRACE( "(%s)\n", debugstr_w(NewEnvironment));
>>>>>>> +
>>>>>>> + if (NULL == NewEnvironment)
>>>>>>> + return rc;
>>>>>>
>>>>>> This probably deserves a test.
>>>>>>
>>>>>
>>>>> I tried including a simple test by just calling
>>>>> SetEnvironmentStrings(NULL) and that lead to an unhandled exception on
>>>>> my windows machine (cross-compiled from 64 bit Linux to 64 bit Windows
>>>>> using mingw). No clue how and why, but would welcome any suggestions
>>>>> on why this simple thing might fail.
>>>>
>>>> It's possible that Windows doesn't check the pointer against NULL
>>>> either, and in that case there's not much point in doing it in Wine.
>>>>
>>>
>>> You're right, but this is such an elementary check that I would not remove it.
>>>
>>>>>
>>>>>>> +
>>>>>>> + var = wcstok(NewEnvironment, delim);
>>>>>>> + val = wcstok(NULL, delim);
>>>>>>
>>>>>> This isn't thread-safe. [It's not stated anywhere that
>>>>>> SetEnvironmentStrings() itself is thread-safe, but it costs little to
>>>>>> assume it is, and moreover this will race with anything else that calls
>>>>>> wcstok().]
>>>>>
>>>>> True, however I had trouble using the reentrant wcstok_s on my system
>>>>> (i.e., 'twas unavailable :). Is there an alternative, specific
>>>>> include? (I tride the expected #include <wchar.h> but that didn't
>>>>> work).
>>>>
>>>> It should probably be added to the headers. I think it goes in string.h
>>>> and wchar.h, but Jacek might correct me on this.
>>>>
>>>
>>> Switched to wcschr which is available and working.
>>> Was trying to include wcstok_s from msvcrt, but without success. What
>>> would be the proper way to go about this?
>>
>> I think that Jacek added it in the interim ;-)
>
> Interesting, wcstok_s is available in the headers, however linking for
> the kernelbase.dll.so with undefined reference. Not sure though which
> additional library should be included!?
Er, yes, I forgot this was kernelbase. Indeed you can't use wcstok_s in
that case, since ntdll doesn't export it.
>
>>
>> That said, you probably shouldn't use it, if tests show that
>> SetEnvironmentStrings() doesn't modify the argument. (If it does, I
>> guess that gives you free license.)
>>
>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> + while (var != NULL) {
>>>>>>> + if (FALSE == (rc = SetEnvironmentVariableW(var, val)))
>>>>>>> + break;
>>>>>>> + var = wcstok(NULL, delim);
>>>>>>> + val = wcstok(NULL, delim);
>>>>>>> + }
>>>>>>> +
>>>>>>> + return rc;
>>>>>>> +}
>>>>>>
>>>>>> Broadly, this seems surprising; is this really how the function is
>>>>>> supposed to work?
>>>>>>
>>>>>> I think your patch could use more interesting tests. For example:
>>>>>>
>>>>>> * Immediately I'd expect GetEnvironmentStrings() to return the exact
>>>>>> same pointer that SetEnvironmentStrings() set. Is this in fact the case?
>>>>>> If so, how do A and W variants (of both Get/Set) interact?
>>>>>>
>>>>>
>>>>> Confusing. GetEnvironmentStringsW doesn't return a pointer. Haven't
>>>>> seen any info on the GetEnvironmentStringsA variant.
>>>>
>>>> Er, what? GetEnvironmentStringsA() and GetEnvironmentStringsW() return
>>>> pointers to char and WCHAR respectively.
>>>>
>>>
>>> Sorry, misread. However, GetEnvironmentStringsW returns a pointer to a
>>> new copy of the environment strings with each call. Probably from the
>>> same source that gets modified by SetEnvironmentStringsW.
>>> Not really checkable from the outside, I gather.
>>
>> Okay, so part of that comment was based on the mistaken impression that
>> GetEnvironmentStrings() returned a pointer to a constant string instead
>> of allocating a new set every time. Apologies for the confusion that may
>> have caused.
>>
>>>
>>>>>
>>>>>> * Are values not present in the call to SetEnvironmentStrings() deleted
>>>>>> from the environment?
>>>>>
>>>>> doc doesn't specify anything about this. I'd expect no.
>>>>
>>>> The documentation is often less specific than desired, and also often
>>>> incorrect; that's why we write tests, and code our implementations
>>>> against them. It's not obvious whether SetEnvironmentStrings() means
>>>> "add these several strings to the environment" or "replace the entire
>>>> environment with these strings", hence the question.
>>>>
>>>
>>> Yes, on a Windows machine a fresh environment is set up and all the
>>> previous variables are discarded. In the wine patch now as well.
>>
>> One thing you'll need to take into consideration, then, is that there
>> are several environment variables Wine needs to function correctly. In
>> practice I think this means pretty much anything prefixed with WINE,
>> though you may want to maintain a manual list.
>>
>> Are there any others I'm not thinking of?
>>
>> (Host libraries would presumably need them—and if I'm using e.g.
>> GST_DEBUG I'd prefer not to have to jump through hoops to make that
>> work, although if SetEnvironmentStrings() is uncommon I can't complain
>> too much, and really all I'd need to do is hack
>> RtlSetCurrentEnvironment() to set that variable afterward. On the one
>> hand, host libraries would use getenv() anyway, but on the other hand,
>> if we move to PE files for some of them, this becomes a potential
>> problem again. All in all, I'm not sure this is a salient enough concern
>> to warrant doing anything about it...)
>>
>
> If you're using getenv, you're getting (as you say) variables from the
> host. Those vars are not touched by this function and ought to not get
> reset. Or is wine copying the host env into its own internal
> Windows-like representation (WinRep)?
> Are those WINE-prefixed environment variables within the host or the WinRep?
The host environment gets copied into the Win32 environment (with some
modifications; see is_special_env_var() in dlls/ntdll/env.c). Wine also
adds some variables to the Win32 environment not present in the host
environment; see set_additional_environment() and set_wow64_environment().
In particular I'm concerned about environment variables such as
WINEDLLDIR, WINECONFIGDIR, i.e. those set by set_wine_path_variable();
certain components (e.g. mscoree, setupapi) need those to be set to
function.
It's also maybe worth a test to see if even non-Wine-specific variables
set by ntdll (e.g. ProgramFilesDir) get stripped from the environment.
>
>>>
>>>>>
>>>>>>
>>>>>> * The PSDK header suggests that SetEnvironmentStrings() should receive a
>>>>>> doubly null-terminated argument. I'd advise testing with an environment
>>>>>> containing a null separator (e.g. L"one=two\0three=four\0".)
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>>> * You treat '\n' as a separator, but have no tests for it. Also, if both
>>>>>> space and newline are a separator, what about other whitespace
>>>>>> characters (tab, carriage return)?
>>>>>
>>>>> I have reduced the number of possible separators to just include '='
>>>>> and conform with the doubly-null terminated string expectation. This
>>>>> also conforms with the few examples I've found.
>>>>>
>>>>>>
>>>>>> * What about strings that violate the assumptions this implementation
>>>>>> makes? For example, "one=", "one", "=two", "one=two=three", "one=two
>>>>>> three=four"...
>>>>>>
>>>>>
>>>>> Included a few more tests.
>>>
>>> Just to note, "one=two=three" is valid and will assign 'two=three' to
>>> the variable 'one' (on windows).
>>> one= is also not illegal, AFAICT.
>>>
>>>>>
>>>>>> * Is this function really supposed to modify its argument? (Yes, it's
>>>>>> not declared constant, but that doesn't always mean that it will.)
>>>>>>
>>>>>
>>>>> Done, included a buffer that's modified instead.
>>>>>
>>>>>
>>>>>>> +
>>>>>>> +
>>>>>>> /***********************************************************************
>>>>>>> * Process/thread attribute lists
>>>>>>> ***********************************************************************/
>>>>>>> diff --git a/include/winbase.h b/include/winbase.h
>>>>>>> index 655eb48f0f..0c1aa19b42 100644
>>>>>>> --- a/include/winbase.h
>>>>>>> +++ b/include/winbase.h
>>>>>>> @@ -2621,6 +2621,7 @@ WINBASEAPI BOOL WINAPI SetEndOfFile(HANDLE);
>>>>>>> WINBASEAPI BOOL WINAPI SetEnvironmentVariableA(LPCSTR,LPCSTR);
>>>>>>> WINBASEAPI BOOL WINAPI SetEnvironmentVariableW(LPCWSTR,LPCWSTR);
>>>>>>> #define SetEnvironmentVariable WINELIB_NAME_AW(SetEnvironmentVariable)
>>>>>>> +WINBASEAPI BOOL WINAPI SetEnvironmentStringsW(LPWCH);
>>>>>>> WINBASEAPI UINT WINAPI SetErrorMode(UINT);
>>>>>>> WINBASEAPI BOOL WINAPI SetEvent(HANDLE);
>>>>>>> WINBASEAPI VOID WINAPI SetFileApisToANSI(void);
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
More information about the wine-devel
mailing list