[PATCH] add: SetEnvironmentStringsW

[Frank Uhlig]

From: Frank Uhlig

Signed-off-by: Frank Uhlig 
---
 ...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};
+
+    buf_len = sizeof(buf) / 2;
+
+    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");
+
+    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 )
+{
+
+    BOOL rc = FALSE;
+    WCHAR *var, *val;
+    const WCHAR delim[] = {' ','=','\n',0};
+
+    TRACE( "(%s)\n", debugstr_w(NewEnvironment));
+
+    if (NULL == NewEnvironment)
+        return rc;
+
+    var = wcstok(NewEnvironment, delim);
+    val = wcstok(NULL, delim);
+
+    while (var != NULL) {
+        if (FALSE == (rc = SetEnvironmentVariableW(var, val)))
+            break;
+        var = wcstok(NULL, delim);
+        val = wcstok(NULL, delim);
+    }
+
+    return rc;
+}
+
+
 /***********************************************************************
  * 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);

[Marvin]

Hi,

While running your changed tests, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=63595

Your paranoid android.

=== wxppro (32 bit report) ===

kernel32:
environ: Timeout

=== w2003std (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== wvistau64 (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w2008s64 (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w7u (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w8 (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w8adm (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w864 (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1507 (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1809 (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1809_2scr (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1809_ar (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1809_he (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1809_ja (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1809_zh_CN (32 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== wvistau64 (64 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w2008s64 (64 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w864 (64 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1507 (64 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== w1064v1809 (64 bit report) ===

kernel32:
environ.c:623: Test failed: Setting environment strings failed
environ.c:626: Test failed: Environment String settings resulted in different value
environ.c:630: Test failed: Environment String settings resulted in different value

=== debian10 (32 bit report) ===

kernel32:
debugger.c:305: Test failed: GetThreadContext failed: 5

=== debian10 (32 bit Chinese:China report) ===

kernel32:
comm.c:918: Test failed: OutQueue should not be empty
debugger.c:305: Test failed: GetThreadContext failed: 5

=== debian10 (64 bit WoW report) ===

kernel32:
comm.c:918: Test failed: OutQueue should not be empty
debugger: Timeout

[Frank Uhlig]

OK, so I reproduced errors from Marvin's previous report before
submitting this particular patch using a mingw64 environment and
resolved them by adding a few forgotten null terminations.
However, Marvin tells me that the tests are still not working, yet I
don't see why. Might this be some issue related to double-null
termination? Any other issue between 32 and 64bit wine versions?

The one other thing I have tried beyond this, is running the GOG
Galaxy open beta and running some games in it, which is where this
function is used and it appears to work fine.

Again, any help appreciated, and have a nice weekend,

Frank

On Wed, Jan 22, 2020 at 11:48 PM Marvin  wrote:
> Hi,
>
> While running your changed tests, I think I found new failures.
> Being a bot and all I'm not very good at pattern recognition, so I might be
> wrong, but could you please double-check?
>
> Full results can be found at:
> https://testbot.winehq.org/JobDetails.pl?Key=63595
>
> Your paranoid android.
>
> === wxppro (32 bit report) ===
>
> kernel32:
> environ: Timeout
>
> === w2003std (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === wvistau64 (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w2008s64 (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w7u (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w8 (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w8adm (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w864 (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1507 (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1809 (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1809_2scr (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1809_ar (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1809_he (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1809_ja (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1809_zh_CN (32 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === wvistau64 (64 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w2008s64 (64 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w864 (64 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1507 (64 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === w1064v1809 (64 bit report) ===
>
> kernel32:
> environ.c:623: Test failed: Setting environment strings failed
> environ.c:626: Test failed: Environment String settings resulted in different value
> environ.c:630: Test failed: Environment String settings resulted in different value
>
> === debian10 (32 bit report) ===
>
> kernel32:
> debugger.c:305: Test failed: GetThreadContext failed: 5
>
> === debian10 (32 bit Chinese:China report) ===
>
> kernel32:
> comm.c:918: Test failed: OutQueue should not be empty
> debugger.c:305: Test failed: GetThreadContext failed: 5
>
> === debian10 (64 bit WoW report) ===
>
> kernel32:
> comm.c:918: Test failed: OutQueue should not be empty
> debugger: Timeout

[Zebediah Figura]

On 1/24/20 4:05 PM, Frank Uhlig wrote:
> OK, so I reproduced errors from Marvin's previous report before
> submitting this particular patch using a mingw64 environment and
> resolved them by adding a few forgotten null terminations.
> However, Marvin tells me that the tests are still not working, yet I
> don't see why. Might this be some issue related to double-null
> termination? Any other issue between 32 and 64bit wine versions?

It's not unlikely that SetEnvironmentStrings() expects double
null-termination. You might verify this by manually placing both 0 and a
nonzero character after the first null terminator.
> The one other thing I have tried beyond this, is running the GOG
> Galaxy open beta and running some games in it, which is where this
> function is used and it appears to work fine.
>
> Again, any help appreciated, and have a nice weekend,
>
> Frank
>
> On Wed, Jan 22, 2020 at 11:48 PM Marvin  wrote:
> > Hi,
> >
> > While running your changed tests, I think I found new failures.
> > Being a bot and all I'm not very good at pattern recognition, so I might be
> > wrong, but could you please double-check?
> >
> > Full results can be found at:
> > https://testbot.winehq.org/JobDetails.pl?Key=63595
> >
> > Your paranoid android.
> >
> > === wxppro (32 bit report) ===
> >
> > kernel32:
> > environ: Timeout
> >
> > === w2003std (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === wvistau64 (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w2008s64 (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w7u (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w8 (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w8adm (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w864 (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1507 (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1809 (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1809_2scr (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1809_ar (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1809_he (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1809_ja (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1809_zh_CN (32 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === wvistau64 (64 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w2008s64 (64 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w864 (64 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1507 (64 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === w1064v1809 (64 bit report) ===
> >
> > kernel32:
> > environ.c:623: Test failed: Setting environment strings failed
> > environ.c:626: Test failed: Environment String settings resulted in different value
> > environ.c:630: Test failed: Environment String settings resulted in different value
> >
> > === debian10 (32 bit report) ===
> >
> > kernel32:
> > debugger.c:305: Test failed: GetThreadContext failed: 5
> >
> > === debian10 (32 bit Chinese:China report) ===
> >
> > kernel32:
> > comm.c:918: Test failed: OutQueue should not be empty
> > debugger.c:305: Test failed: GetThreadContext failed: 5
> >
> > === debian10 (64 bit WoW report) ===
> >
> > kernel32:
> > comm.c:918: Test failed: OutQueue should not be empty
> > debugger: Timeout

[Zebediah Figura]

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()."

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
> Signed-off-by: Frank Uhlig
>
> ...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).

I suspect that you also want either local variables or (when applicable)
"static const". There's no point declaring non-constant variables as static.
> -
> - 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.
> -
> - 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.
> -
> - 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.
> +{
> -

Stray newline.
> - 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.
> -
> - 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().]
> -
> - 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?

* Are values not present in the call to SetEnvironmentStrings() deleted
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".)

* 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)?

* What about strings that violate the assumptions this implementation
makes? For example, "one=", "one", "=two", "one=two=three", "one=two
three=four"...

* Is this function really supposed to modify its argument? (Yes, it's
not declared constant, but that doesn't always mean that it will.)
> -
> -
>
> /***********************************************************************
> - 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);

[Frank Uhlig]

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  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
> > Signed-off-by: Frank Uhlig
> >
> > ...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).
> 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.
> > -
> > - 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.
> > +{
> > -
>
> 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.
> > -
> > - 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).
> > -
> > - 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.
> - 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 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.
> - 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);

[Zebediah Figura]

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  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
> > > Signed-off-by: Frank Uhlig
> > >
> > > ...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.
> > > -
> > > - 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".
> > > +{
> > > -
> >
> > 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.
> > > -
> > > - 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.
> > > -
> > > - 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.
> > - 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.
> > - 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.
> > - 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);

[Frank Uhlig]

Sorry for the late reply, but have finally gotten back to this patch.

On Thu, Feb 20, 2020 at 10:35 PM Zebediah Figura  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  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
> > > > Signed-off-by: Frank Uhlig
> > > >
> > > > ...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?
> > > > -
> > > > - 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.
> > > - 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.
> > > - 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);

[Zebediah Figura]

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  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  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
> > > > > Signed-off-by: Frank Uhlig
> > > > >
> > > > > ...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 ;-)

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...)
> > > > - 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);