22 Jan
2020
22 Jan
'20
9:33 p.m.
From: Frank Uhlig <fuulish(a)users.noreply.github.com>
Signed-off-by: Frank Uhlig <uhlig.frank(a)gmail.com>
---
...ms-win-core-processenvironment-l1-1-0.spec | 2 +-
...ms-win-core-processenvironment-l1-2-0.spec | 2 +-
dlls/kernel32/kernel32.spec | 2 +-
dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++
dlls/kernelbase/kernelbase.spec | 2 +-
dlls/kernelbase/process.c | 29 +++++++++
include/winbase.h | 1 +
7 files changed, 95 insertions(+), 4 deletions(-)
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
index e3698d6efd..7a62b74390 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
@@ -15,7 +15,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
index 2c25ee1a07..c93d221c5e 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
@@ -17,7 +17,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
index be48ef1694..2b74a4182e 100644
--- a/dlls/kernel32/kernel32.spec
+++ b/dlls/kernel32/kernel32.spec
@@ -1387,7 +1387,7 @@
# @ stub SetDynamicTimeZoneInformation
@ stdcall -import SetEndOfFile(long)
# @ stub SetEnvironmentStringsA
-# @ stub SetEnvironmentStringsW
+@ stdcall -import SetEnvironmentStringsW (ptr)
@ stdcall -import SetEnvironmentVariableA(str str)
@ stdcall -import SetEnvironmentVariableW(wstr wstr)
@ stdcall -import SetErrorMode(long)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
index 44a6a0cff0..53b2803e77 100644
--- a/dlls/kernel32/tests/environ.c
+++ b/dlls/kernel32/tests/environ.c
@@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void)
FreeEnvironmentStringsW(env2);
}
+static void test_SetEnvironmentStringsW(void)
+{
+ DWORD buf_len;
+ BOOL ret;
+ DWORD ret_size;
+
+ static WCHAR buf[256];
+
+ static WCHAR name[] = {'N','a','m','e',0};
+ static WCHAR value[] =
{'V','a','l','u','e',0};
+ static WCHAR env[] =
{'N','a','m','e','=','V','a','l','u','e',0};
+
+ static WCHAR eman[] = {'e','m','a','N',0};
+ static WCHAR eulav[] =
{'e','u','l','a','V',0};
+ static WCHAR vne[] =
{'e','m','a','N','=','e','u','l','a','V',0};
+
+ static WCHAR var[] = {'V','a','r',0};
+ static WCHAR val[] = {'V','a','l',0};
+ static WCHAR rav[] = {'r','a','V',0};
+ static WCHAR lav[] = {'l','a','V',0};
+ static WCHAR mul[] =
{'V','a','r','=','V','a','l','
','r','a','V','=','l','a','V',0};
+
+ static WCHAR empty[] = {'V','a','r','=',0};
+
+ 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);
--
2.24.1
22 Jan
22 Jan
10:48 p.m.
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
24 Jan
24 Jan
10:05 p.m.
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 <testbot(a)winehq.org> 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
10 Feb
10 Feb
3:10 a.m.
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 <testbot(a)winehq.org> 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
3:37 a.m.
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
<fuulish(a)users.noreply.github.com>
Signed-off-by: Frank Uhlig <uhlig.frank(a)gmail.com>
---
...ms-win-core-processenvironment-l1-1-0.spec | 2 +-
...ms-win-core-processenvironment-l1-2-0.spec | 2 +-
dlls/kernel32/kernel32.spec | 2 +-
dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++
dlls/kernelbase/kernelbase.spec | 2 +-
dlls/kernelbase/process.c | 29 +++++++++
include/winbase.h | 1 +
7 files changed, 95 insertions(+), 4 deletions(-)
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
index e3698d6efd..7a62b74390 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
@@ -15,7 +15,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
index 2c25ee1a07..c93d221c5e 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
@@ -17,7 +17,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
index be48ef1694..2b74a4182e 100644
--- a/dlls/kernel32/kernel32.spec
+++ b/dlls/kernel32/kernel32.spec
@@ -1387,7 +1387,7 @@
# @ stub SetDynamicTimeZoneInformation
@ stdcall -import SetEndOfFile(long)
# @ stub SetEnvironmentStringsA
-# @ stub SetEnvironmentStringsW
+@ stdcall -import SetEnvironmentStringsW (ptr)
@ stdcall -import SetEnvironmentVariableA(str str)
@ stdcall -import SetEnvironmentVariableW(wstr wstr)
@ stdcall -import SetErrorMode(long)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
index 44a6a0cff0..53b2803e77 100644
--- a/dlls/kernel32/tests/environ.c
+++ b/dlls/kernel32/tests/environ.c
@@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void)
FreeEnvironmentStringsW(env2);
}
+static void test_SetEnvironmentStringsW(void)
+{
+ DWORD buf_len;
+ BOOL ret;
+ DWORD ret_size;
+
+ static WCHAR buf[256];
+
+ static WCHAR name[] = {'N','a','m','e',0};
+ static WCHAR value[] =
{'V','a','l','u','e',0};
+ static WCHAR env[] =
{'N','a','m','e','=','V','a','l','u','e',0};
+
+ static WCHAR eman[] = {'e','m','a','N',0};
+ static WCHAR eulav[] =
{'e','u','l','a','V',0};
+ static WCHAR vne[] =
{'e','m','a','N','=','e','u','l','a','V',0};
+
+ static WCHAR var[] = {'V','a','r',0};
+ static WCHAR val[] = {'V','a','l',0};
+ static WCHAR rav[] = {'r','a','V',0};
+ static WCHAR lav[] = {'l','a','V',0};
+ static WCHAR mul[] =
{'V','a','r','=','V','a','l','
','r','a','V','=','l','a','V',0};
+
+ static WCHAR empty[] = {'V','a','r','=',0};
You can use wide character string literals in tests (which should also
allow you to get rid of some of these declarations).
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);
20 Feb
20 Feb
9:12 p.m.
Hi Zebediah, thanks for looking into this. Please find my responses
inline below. I've already incorporated many things, but still have
some remaining questions and comments.
I would re-submit it soon, and include a "versioning" in the mail tag
as I've seen on other submissions (i.e., [PATCH v2]).
On Mon, Feb 10, 2020 at 4:38 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
Changed to ARRAY_SIZE(buf)
Made the checks more specific, and included more info in the output
Any particular reason for this? That's exactly what LPWCH is defined as.
Done.
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.
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).
Confusing. GetEnvironmentStringsW doesn't return a pointer. Haven't
seen any info on the GetEnvironmentStringsA variant.
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:
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).
From: Frank Uhlig
<fuulish(a)users.noreply.github.com>
Signed-off-by: Frank Uhlig <uhlig.frank(a)gmail.com>
---
...ms-win-core-processenvironment-l1-1-0.spec | 2 +-
...ms-win-core-processenvironment-l1-2-0.spec | 2 +-
dlls/kernel32/kernel32.spec | 2 +-
dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++
dlls/kernelbase/kernelbase.spec | 2 +-
dlls/kernelbase/process.c | 29 +++++++++
include/winbase.h | 1 +
7 files changed, 95 insertions(+), 4 deletions(-)
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
index e3698d6efd..7a62b74390 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
@@ -15,7 +15,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
index 2c25ee1a07..c93d221c5e 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
@@ -17,7 +17,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
index be48ef1694..2b74a4182e 100644
--- a/dlls/kernel32/kernel32.spec
+++ b/dlls/kernel32/kernel32.spec
@@ -1387,7 +1387,7 @@
# @ stub SetDynamicTimeZoneInformation
@ stdcall -import SetEndOfFile(long)
# @ stub SetEnvironmentStringsA
-# @ stub SetEnvironmentStringsW
+@ stdcall -import SetEnvironmentStringsW (ptr)
@ stdcall -import SetEnvironmentVariableA(str str)
@ stdcall -import SetEnvironmentVariableW(wstr wstr)
@ stdcall -import SetErrorMode(long)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
index 44a6a0cff0..53b2803e77 100644
--- a/dlls/kernel32/tests/environ.c
+++ b/dlls/kernel32/tests/environ.c
@@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void)
FreeEnvironmentStringsW(env2);
}
+static void test_SetEnvironmentStringsW(void)
+{
+ DWORD buf_len;
+ BOOL ret;
+ DWORD ret_size;
+
+ static WCHAR buf[256];
+
+ static WCHAR name[] = {'N','a','m','e',0};
+ static WCHAR value[] =
{'V','a','l','u','e',0};
+ static WCHAR env[] =
{'N','a','m','e','=','V','a','l','u','e',0};
+
+ static WCHAR eman[] = {'e','m','a','N',0};
+ static WCHAR eulav[] =
{'e','u','l','a','V',0};
+ static WCHAR vne[] =
{'e','m','a','N','=','e','u','l','a','V',0};
+
+ static WCHAR var[] = {'V','a','r',0};
+ static WCHAR val[] = {'V','a','l',0};
+ static WCHAR rav[] = {'r','a','V',0};
+ static WCHAR lav[] = {'l','a','V',0};
+ static WCHAR mul[] =
{'V','a','r','=','V','a','l','
','r','a','V','=','l','a','V',0};
+
+ static WCHAR empty[] = {'V','a','r','=',0};
You can use wide character string literals in tests (which should also
allow you to get rid of some of these declarations).
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.
+
+ 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?
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);
9:29 p.m.
On 2/20/20 3:12 PM, Frank Uhlig wrote:
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.
Changed to ARRAY_SIZE(buf)
Made the checks more specific, and included more info in the output
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".
Done.
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.
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.
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.
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.
Hi Zebediah, thanks for looking into this. Please find
my responses
inline below. I've already incorporated many things, but still have
some remaining questions and comments.
I would re-submit it soon, and include a "versioning" in the mail tag
as I've seen on other submissions (i.e., [PATCH v2]).
On Mon, Feb 10, 2020 at 4:38 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
The wiki is out of date on this particular topic. Someone™ should
probably update it.
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:
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). From: Frank Uhlig
<fuulish(a)users.noreply.github.com>
Signed-off-by: Frank Uhlig <uhlig.frank(a)gmail.com>
---
...ms-win-core-processenvironment-l1-1-0.spec | 2 +-
...ms-win-core-processenvironment-l1-2-0.spec | 2 +-
dlls/kernel32/kernel32.spec | 2 +-
dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++
dlls/kernelbase/kernelbase.spec | 2 +-
dlls/kernelbase/process.c | 29 +++++++++
include/winbase.h | 1 +
7 files changed, 95 insertions(+), 4 deletions(-)
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
index e3698d6efd..7a62b74390 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
@@ -15,7 +15,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
index 2c25ee1a07..c93d221c5e 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
@@ -17,7 +17,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
index be48ef1694..2b74a4182e 100644
--- a/dlls/kernel32/kernel32.spec
+++ b/dlls/kernel32/kernel32.spec
@@ -1387,7 +1387,7 @@
# @ stub SetDynamicTimeZoneInformation
@ stdcall -import SetEndOfFile(long)
# @ stub SetEnvironmentStringsA
-# @ stub SetEnvironmentStringsW
+@ stdcall -import SetEnvironmentStringsW (ptr)
@ stdcall -import SetEnvironmentVariableA(str str)
@ stdcall -import SetEnvironmentVariableW(wstr wstr)
@ stdcall -import SetErrorMode(long)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
index 44a6a0cff0..53b2803e77 100644
--- a/dlls/kernel32/tests/environ.c
+++ b/dlls/kernel32/tests/environ.c
@@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void)
FreeEnvironmentStringsW(env2);
}
+static void test_SetEnvironmentStringsW(void)
+{
+ DWORD buf_len;
+ BOOL ret;
+ DWORD ret_size;
+
+ static WCHAR buf[256];
+
+ static WCHAR name[] = {'N','a','m','e',0};
+ static WCHAR value[] =
{'V','a','l','u','e',0};
+ static WCHAR env[] =
{'N','a','m','e','=','V','a','l','u','e',0};
+
+ static WCHAR eman[] = {'e','m','a','N',0};
+ static WCHAR eulav[] =
{'e','u','l','a','V',0};
+ static WCHAR vne[] =
{'e','m','a','N','=','e','u','l','a','V',0};
+
+ static WCHAR var[] = {'V','a','r',0};
+ static WCHAR val[] = {'V','a','l',0};
+ static WCHAR rav[] = {'r','a','V',0};
+ static WCHAR lav[] = {'l','a','V',0};
+ static WCHAR mul[] =
{'V','a','r','=','V','a','l','
','r','a','V','=','l','a','V',0};
+
+ static WCHAR empty[] = {'V','a','r','=',0};
You can use wide character string literals in tests (which should also
allow you to get rid of some of these declarations).
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.
+
+ 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?
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);
11 Mar
11 Mar
9:50 p.m.
Sorry for the late reply, but have finally gotten back to this patch.
On Thu, Feb 20, 2020 at 10:35 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
Changed to ARRAY_SIZE(buf)
Made the checks more specific, and included more info in the output
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.
Done.
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.
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?
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.
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.
On 2/20/20 3:12 PM, Frank Uhlig wrote:
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.
Hi Zebediah, thanks for looking into this. Please
find my responses
inline below. I've already incorporated many things, but still have
some remaining questions and comments.
I would re-submit it soon, and include a "versioning" in the mail tag
as I've seen on other submissions (i.e., [PATCH v2]).
On Mon, Feb 10, 2020 at 4:38 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
The wiki is out of date on this particular topic. Someone™ should
probably update it.
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:
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). From: Frank Uhlig
<fuulish(a)users.noreply.github.com>
Signed-off-by: Frank Uhlig <uhlig.frank(a)gmail.com>
---
...ms-win-core-processenvironment-l1-1-0.spec | 2 +-
...ms-win-core-processenvironment-l1-2-0.spec | 2 +-
dlls/kernel32/kernel32.spec | 2 +-
dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++
dlls/kernelbase/kernelbase.spec | 2 +-
dlls/kernelbase/process.c | 29 +++++++++
include/winbase.h | 1 +
7 files changed, 95 insertions(+), 4 deletions(-)
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
index e3698d6efd..7a62b74390 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
@@ -15,7 +15,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
index 2c25ee1a07..c93d221c5e 100644
---
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
+++
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
@@ -17,7 +17,7 @@
@ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
@ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
@ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
-@ stub SetEnvironmentStringsW
+@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
@ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
@ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
@ stdcall SetStdHandle(long long) kernel32.SetStdHandle
diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
index be48ef1694..2b74a4182e 100644
--- a/dlls/kernel32/kernel32.spec
+++ b/dlls/kernel32/kernel32.spec
@@ -1387,7 +1387,7 @@
# @ stub SetDynamicTimeZoneInformation
@ stdcall -import SetEndOfFile(long)
# @ stub SetEnvironmentStringsA
-# @ stub SetEnvironmentStringsW
+@ stdcall -import SetEnvironmentStringsW (ptr)
@ stdcall -import SetEnvironmentVariableA(str str)
@ stdcall -import SetEnvironmentVariableW(wstr wstr)
@ stdcall -import SetErrorMode(long)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
index 44a6a0cff0..53b2803e77 100644
--- a/dlls/kernel32/tests/environ.c
+++ b/dlls/kernel32/tests/environ.c
@@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void)
FreeEnvironmentStringsW(env2);
}
+static void test_SetEnvironmentStringsW(void)
+{
+ DWORD buf_len;
+ BOOL ret;
+ DWORD ret_size;
+
+ static WCHAR buf[256];
+
+ static WCHAR name[] = {'N','a','m','e',0};
+ static WCHAR value[] =
{'V','a','l','u','e',0};
+ static WCHAR env[] =
{'N','a','m','e','=','V','a','l','u','e',0};
+
+ static WCHAR eman[] = {'e','m','a','N',0};
+ static WCHAR eulav[] =
{'e','u','l','a','V',0};
+ static WCHAR vne[] =
{'e','m','a','N','=','e','u','l','a','V',0};
+
+ static WCHAR var[] = {'V','a','r',0};
+ static WCHAR val[] = {'V','a','l',0};
+ static WCHAR rav[] = {'r','a','V',0};
+ static WCHAR lav[] = {'l','a','V',0};
+ static WCHAR mul[] =
{'V','a','r','=','V','a','l','
','r','a','V','=','l','a','V',0};
+
+ static WCHAR empty[] = {'V','a','r','=',0};
You can use wide character string literals in tests (which should also
allow you to get rid of some of these declarations).
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.
+
+ 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?
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.
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);
12 Mar
12 Mar
2:24 a.m.
On 3/11/20 4:50 PM, Frank Uhlig wrote:
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.
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...)
Sorry for the late reply, but have finally gotten back
to this patch.
On Thu, Feb 20, 2020 at 10:35 PM Zebediah Figura <z.figura12(a)gmail.com> wrote:
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.
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.
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.)
On 2/20/20 3:12 PM, Frank Uhlig wrote:
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.
Hi Zebediah, thanks for looking into this. Please
find my responses
inline below. I've already incorporated many things, but still have
some remaining questions and comments.
I would re-submit it soon, and include a "versioning" in the mail tag
as I've seen on other submissions (i.e., [PATCH v2]).
On Mon, Feb 10, 2020 at 4:38 AM Zebediah Figura <z.figura12(a)gmail.com> wrote:
The wiki is out of date on this particular topic. Someone™ should
probably update it.
Hello Frank, thanks for the patch.
With regard to the title, convention is to put the name of the component
first, followed by a colon, then a verb describing what the patch does,
so e.g. "kernelbase: Add SetEnvironmentStringsW()."
done.
I have a few more comments inlined below which
might help improve the patch:
On 1/22/20 3:33 PM, Frank Uhlig wrote:
> From: Frank Uhlig <fuulish(a)users.noreply.github.com>
>
> Signed-off-by: Frank Uhlig <uhlig.frank(a)gmail.com>
> ---
> ...ms-win-core-processenvironment-l1-1-0.spec | 2 +-
> ...ms-win-core-processenvironment-l1-2-0.spec | 2 +-
> dlls/kernel32/kernel32.spec | 2 +-
> dlls/kernel32/tests/environ.c | 61 +++++++++++++++++++
> dlls/kernelbase/kernelbase.spec | 2 +-
> dlls/kernelbase/process.c | 29 +++++++++
> include/winbase.h | 1 +
> 7 files changed, 95 insertions(+), 4 deletions(-)
>
> diff --git
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
> index e3698d6efd..7a62b74390 100644
> ---
a/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
> +++
b/dlls/api-ms-win-core-processenvironment-l1-1-0/api-ms-win-core-processenvironment-l1-1-0.spec
> @@ -15,7 +15,7 @@
> @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
> @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
> @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
> -@ stub SetEnvironmentStringsW
> +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
> @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
> @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
> @ stdcall SetStdHandle(long long) kernel32.SetStdHandle
> diff --git
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
> index 2c25ee1a07..c93d221c5e 100644
> ---
a/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
> +++
b/dlls/api-ms-win-core-processenvironment-l1-2-0/api-ms-win-core-processenvironment-l1-2-0.spec
> @@ -17,7 +17,7 @@
> @ stdcall SearchPathW(wstr wstr wstr long ptr ptr) kernel32.SearchPathW
> @ stdcall SetCurrentDirectoryA(str) kernel32.SetCurrentDirectoryA
> @ stdcall SetCurrentDirectoryW(wstr) kernel32.SetCurrentDirectoryW
> -@ stub SetEnvironmentStringsW
> +@ stdcall SetEnvironmentStringsW(ptr) kernel32.SetEnvironmentStringsW
> @ stdcall SetEnvironmentVariableA(str str) kernel32.SetEnvironmentVariableA
> @ stdcall SetEnvironmentVariableW(wstr wstr) kernel32.SetEnvironmentVariableW
> @ stdcall SetStdHandle(long long) kernel32.SetStdHandle
> diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
> index be48ef1694..2b74a4182e 100644
> --- a/dlls/kernel32/kernel32.spec
> +++ b/dlls/kernel32/kernel32.spec
> @@ -1387,7 +1387,7 @@
> # @ stub SetDynamicTimeZoneInformation
> @ stdcall -import SetEndOfFile(long)
> # @ stub SetEnvironmentStringsA
> -# @ stub SetEnvironmentStringsW
> +@ stdcall -import SetEnvironmentStringsW (ptr)
> @ stdcall -import SetEnvironmentVariableA(str str)
> @ stdcall -import SetEnvironmentVariableW(wstr wstr)
> @ stdcall -import SetErrorMode(long)
> diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
> index 44a6a0cff0..53b2803e77 100644
> --- a/dlls/kernel32/tests/environ.c
> +++ b/dlls/kernel32/tests/environ.c
> @@ -579,6 +579,66 @@ static void test_GetEnvironmentStringsW(void)
> FreeEnvironmentStringsW(env2);
> }
>
> +static void test_SetEnvironmentStringsW(void)
> +{
> + DWORD buf_len;
> + BOOL ret;
> + DWORD ret_size;
> +
> + static WCHAR buf[256];
> +
> + static WCHAR name[] = {'N','a','m','e',0};
> + static WCHAR value[] =
{'V','a','l','u','e',0};
> + static WCHAR env[] =
{'N','a','m','e','=','V','a','l','u','e',0};
> +
> + static WCHAR eman[] = {'e','m','a','N',0};
> + static WCHAR eulav[] =
{'e','u','l','a','V',0};
> + static WCHAR vne[] =
{'e','m','a','N','=','e','u','l','a','V',0};
> +
> + static WCHAR var[] = {'V','a','r',0};
> + static WCHAR val[] = {'V','a','l',0};
> + static WCHAR rav[] = {'r','a','V',0};
> + static WCHAR lav[] = {'l','a','V',0};
> + static WCHAR mul[] =
{'V','a','r','=','V','a','l','
','r','a','V','=','l','a','V',0};
> +
> + static WCHAR empty[] = {'V','a','r','=',0};
You can use wide character string literals in tests (which should also
allow you to get rid of some of these declarations).
Didn't change this, b/c I was trying to keep the same style as already
there and described on the website
(https://wiki.winehq.org/Developer_Hints#Using_only_C89-compliant_code). 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.
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);
>
1514
days inactive
1564
days old
8 comments
4 participants
participants (4)
-
Frank Uhlig -
Marvin -
Zebediah Figura -
Zebediah Figura