[PATCH v4 1/4] kernelbase: Don't return ERROR_INSUFFICIENT_BUFFER from GetEnvironmentVariableW.
Windows doesn't do this (except XP, and then only for empty variables, where it returns ERROR_MORE_DATA).
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48471 Signed-off-by: Vladimir Panteleev git@vladimir.panteleev.md ---
Since v3: Tweak commit message, rebase, and reorder to front of patchset (so that the last two patches can take advantage of this and the next one). --- dlls/kernel32/tests/environ.c | 6 ++++++ dlls/kernelbase/process.c | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index 128a5fdbe5..e65d7af0f2 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -104,9 +104,12 @@ static void test_GetSetEnvironmentVariableA(void) GetLastError());
/* Try to retrieve the environment variable we just set */ + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(name, NULL, 0); ok(ret_size == strlen(value) + 1, "should return length with terminating 0 ret_size=%d\n", ret_size); + ok(GetLastError() == 0xdeadbeef, + "should not fail with zero size but GetLastError=%d\n", GetLastError());
lstrcpyA(buf, "foo"); ret_size = GetEnvironmentVariableA(name, buf, lstrlenA(value)); @@ -206,10 +209,13 @@ static void test_GetSetEnvironmentVariableW(void) GetLastError());
/* Try to retrieve the environment variable we just set */ + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(name, NULL, 0); ok(ret_size == lstrlenW(value) + 1, "should return length with terminating 0 ret_size=%d\n", ret_size); + ok(GetLastError() == 0xdeadbeef, + "should not fail with zero size but GetLastError=%d\n", GetLastError());
lstrcpyW(buf, fooW); ret_size = GetEnvironmentVariableW(name, buf, lstrlenW(value)); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index a07dddb1fc..788f03f220 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1274,7 +1274,8 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableW( LPCWSTR name, LPWSTR val
status = RtlQueryEnvironmentVariable_U( NULL, &us_name, &us_value ); len = us_value.Length / sizeof(WCHAR); - if (!set_ntstatus( status )) return (status == STATUS_BUFFER_TOO_SMALL) ? len + 1 : 0; + if (status == STATUS_BUFFER_TOO_SMALL) return len + 1; + if (!set_ntstatus( status )) return 0; if (size) val[len] = 0; return len; }
Do the same thing as Vista and up.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48471 Signed-off-by: Vladimir Panteleev git@vladimir.panteleev.md ---
Since v3: No changes. --- dlls/kernel32/tests/environ.c | 20 ++++++++++++++++++++ dlls/kernelbase/process.c | 3 ++- 2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index e65d7af0f2..b2c9a3a4ff 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -166,6 +166,16 @@ static void test_GetSetEnvironmentVariableA(void) "%s should be set to "" (NT) or removed (Win9x) but ret_size=%d GetLastError=%d and buf=%s\n", name, ret_size, GetLastError(), buf);
+ SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableA(name, NULL, 0); + ok(ret_size == 1 || + broken(ret_size == 0), /* XP */ + "should return 1 for empty string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + ok(GetLastError() == 0xdeadbeef || + broken(GetLastError() == ERROR_MORE_DATA), /* XP */ + "should not fail with zero size but GetLastError=%d\n", GetLastError()); + /* Test the limits */ SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(NULL, NULL, 0); @@ -275,6 +285,16 @@ static void test_GetSetEnvironmentVariableW(void) ret_size, GetLastError()); ok(lstrcmpW(buf, empty_strW) == 0, "should copy an empty string\n");
+ SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableW(name, NULL, 0); + ok(ret_size == 1 || + broken(ret_size == 0), /* XP */ + "should return 1 for empty string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + ok(GetLastError() == 0xdeadbeef || + broken(GetLastError() == ERROR_MORE_DATA), /* XP */ + "should not fail with zero size but GetLastError=%d\n", GetLastError()); + /* Test the limits */ SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(NULL, NULL, 0); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 788f03f220..612b0b2a2f 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1276,7 +1276,8 @@ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableW( LPCWSTR name, LPWSTR val len = us_value.Length / sizeof(WCHAR); if (status == STATUS_BUFFER_TOO_SMALL) return len + 1; if (!set_ntstatus( status )) return 0; - if (size) val[len] = 0; + if (!size) return len + 1; + val[len] = 0; return len; }
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=63779
Your paranoid android.
=== debian10 (32 bit report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0 environ.c:175: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit French report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0 environ.c:175: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit Japanese:Japan report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0 environ.c:175: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit Chinese:China report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0 environ.c:175: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit WoW report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0 environ.c:175: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (64 bit WoW report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0 environ.c:175: Test failed: should not fail with zero size but GetLastError=0
Avoid clobbering last error with NO_ERROR when GetEnvironmentVariableA succeeds, matching the behavior of GetEnvironmentVariableW and Windows.
Instead of naively saving and restoring the last error, call RtlQueryEnvironmentVariable_U directly to avoid unnecessarily setting it in the first place.
Signed-off-by: Vladimir Panteleev git@vladimir.panteleev.md ---
Since v3: Apply suggestion from review and copy the innards of GetEnvironmentVariableW instead of saving and restoring the last error. --- dlls/kernel32/tests/environ.c | 8 ++++---- dlls/kernelbase/process.c | 30 +++++++++++++++++++----------- 2 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index b2c9a3a4ff..3144b8e04e 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -158,10 +158,10 @@ static void test_GetSetEnvironmentVariableA(void) "should not fail with empty value but GetLastError=%d\n", GetLastError());
lstrcpyA(buf, "foo"); - SetLastError(0); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableA(name, buf, lstrlenA(value) + 1); ok(ret_size == 0 && - ((GetLastError() == 0 && lstrcmpA(buf, "") == 0) || + ((GetLastError() == 0xdeadbeef && lstrcmpA(buf, "") == 0) || (GetLastError() == ERROR_ENVVAR_NOT_FOUND)), "%s should be set to "" (NT) or removed (Win9x) but ret_size=%d GetLastError=%d and buf=%s\n", name, ret_size, GetLastError(), buf); @@ -276,10 +276,10 @@ static void test_GetSetEnvironmentVariableW(void) ok(ret == TRUE, "should not fail with empty value but GetLastError=%d\n", GetLastError());
lstrcpyW(buf, fooW); - SetLastError(0); + SetLastError(0xdeadbeef); ret_size = GetEnvironmentVariableW(name, buf, lstrlenW(value) + 1); ok(ret_size == 0 && - ((GetLastError() == 0 && lstrcmpW(buf, empty_strW) == 0) || + ((GetLastError() == 0xdeadbeef && lstrcmpW(buf, empty_strW) == 0) || (GetLastError() == ERROR_ENVVAR_NOT_FOUND)), "should be set to "" (NT) or removed (Win9x) but ret_size=%d GetLastError=%d\n", ret_size, GetLastError()); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 612b0b2a2f..3cd2934c20 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1232,24 +1232,32 @@ LPWSTR WINAPI DECLSPEC_HOTPATCH GetEnvironmentStringsW(void) */ DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value, DWORD size ) { - UNICODE_STRING us_name; + UNICODE_STRING us_name, us_value; PWSTR valueW; - DWORD ret; + NTSTATUS status; + DWORD len, ret;
/* limit the size to sane values */ size = min( size, 32767 ); if (!(valueW = HeapAlloc( GetProcessHeap(), 0, size * sizeof(WCHAR) ))) return 0;
RtlCreateUnicodeStringFromAsciiz( &us_name, name ); - SetLastError( 0 ); - ret = GetEnvironmentVariableW( us_name.Buffer, valueW, size); - if (ret && ret < size) WideCharToMultiByte( CP_ACP, 0, valueW, ret + 1, value, size, NULL, NULL ); - - /* this is needed to tell, with 0 as a return value, the difference between: - * - an error (GetLastError() != 0) - * - returning an empty string (in this case, we need to update the buffer) - */ - if (ret == 0 && size && GetLastError() == 0) value[0] = 0; + us_value.Length = 0; + us_value.MaximumLength = (size ? size - 1 : 0) * sizeof(WCHAR); + us_value.Buffer = valueW; + + status = RtlQueryEnvironmentVariable_U( NULL, &us_name, &us_value ); + len = us_value.Length / sizeof(WCHAR); + if (status == STATUS_BUFFER_TOO_SMALL) ret = len + 1; + else if (!set_ntstatus( status )) ret = 0; + else if (!size) ret = len + 1; + else + { + if (len) WideCharToMultiByte( CP_ACP, 0, valueW, len + 1, value, size, NULL, NULL ); + value[len] = 0; + ret = len; + } + RtlFreeUnicodeString( &us_name ); HeapFree( GetProcessHeap(), 0, valueW ); return ret;
The previous implementation of GetEnvironmentVariableA incorrectly assumed that, after WideCharToMultiByte conversion, the resulting A string will have the same number of CHARs as the number of WCHARs in the W string. This assumption resulted in failures when querying environment variables with non-ASCII values in locales with multi-byte character pages.
Address this by always retrieving the Unicode value of the variable, then measuring its length after code page conversion, and only then deciding whether it fits into the user-supplied buffer.
Since we are no longer preallocating a buffer with a user-specified size, the 32767 character limit on the size parameter is removed, as it should be enforced naturally by the size of the environment block.
Signed-off-by: Vladimir Panteleev git@vladimir.panteleev.md ---
Since v3: New patch to fix a deficiency observed during implementation of previous commit.
I haven't found how to get the test bot to run these tests against Wine. Selecting "Japanese:Japan" for the debian10 VM still causes the test suite to run in an environment where GetACP() still returns 1252.
The tests do run locally if I run `LANG=ja_JP.UTF-8 make environ.ok`, and pass. --- dlls/kernel32/tests/environ.c | 79 +++++++++++++++++++++++++++++++++++ dlls/kernelbase/process.c | 49 +++++++++++++++------- 2 files changed, 114 insertions(+), 14 deletions(-)
diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index 3144b8e04e..dd38aa1ee2 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -318,6 +318,84 @@ static void test_GetSetEnvironmentVariableW(void) } }
+static void test_GetSetEnvironmentVariableAW(void) +{ + char buf[256]; + WCHAR bufW[256]; + BOOL ret; + DWORD ret_size; + static const char name[] = "\x96\xBC\x91\x4F"; + static const WCHAR nameW[] = {0x540D, 0x524D, 0}; + static const char value[] = "\x92\x6C"; + static const WCHAR valueW[] = {0x5024, 0}; + static const WCHAR fooW[] = {'f','o','o',0}; + + if (GetACP() != 932) + { + skip("GetACP() == %d, need 932 for A/W tests\n", GetACP()); + return; + } + + /* Write W, read A */ + ret = SetEnvironmentVariableW(nameW, valueW); + if (ret == FALSE && GetLastError()==ERROR_CALL_NOT_IMPLEMENTED) + { + /* Must be Win9x which doesn't support the Unicode functions */ + win_skip("SetEnvironmentVariableW is not implemented\n"); + return; + } + ok(ret == TRUE, + "unexpected error in SetEnvironmentVariableW, GetLastError=%d\n", + GetLastError()); + + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableA(name, NULL, 0); + ok(GetLastError() == 0xdeadbeef && ret_size == lstrlenA(value) + 1, + "should return length for Shift JIS string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + + lstrcpyA(buf, "foo"); + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableA(name, buf, lstrlenA(value) + 1); + ok(GetLastError() == 0xdeadbeef && ret_size == lstrlenA(value) && lstrcmpA(buf, value) == 0, + "should return Shift JIS string but ret_size=%d GetLastError=%d and buf=%s\n", + ret_size, GetLastError(), buf); + + /* Write A, read A/W */ + ret = SetEnvironmentVariableA(name, value); + ok(ret == TRUE, + "unexpected error in SetEnvironmentVariableA, GetLastError=%d\n", + GetLastError()); + + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableA(name, NULL, 0); + ok(GetLastError() == 0xdeadbeef && ret_size == lstrlenA(value) + 1, + "should return length for Shift JIS string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + + lstrcpyA(buf, "foo"); + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableA(name, buf, lstrlenA(value) + 1); + ok(GetLastError() == 0xdeadbeef && ret_size == lstrlenA(value) && lstrcmpA(buf, value) == 0, + "should return Shift JIS string but ret_size=%d GetLastError=%d and buf=%s\n", + ret_size, GetLastError(), buf); + + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableW(nameW, NULL, 0); + ok(GetLastError() == 0xdeadbeef && ret_size == lstrlenW(valueW) + 1, + "should return length for UTF-16 string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + + lstrcpyW(bufW, fooW); + SetLastError(0xdeadbeef); + ret_size = GetEnvironmentVariableW(nameW, bufW, lstrlenW(valueW) + 1); + ok(GetLastError() == 0xdeadbeef && ret_size == lstrlenW(valueW), + "should return UTF-16 string but ret_size=%d GetLastError=%d\n", + ret_size, GetLastError()); + ok_w(GetLastError() == 0xdeadbeef && ret_size == lstrlenW(valueW) && lstrcmpW(bufW, valueW) == 0, + "should return UTF-16 string but bufW=%s\n", bufW); +} + static void test_ExpandEnvironmentStringsA(void) { const char* value="Long long value"; @@ -635,6 +713,7 @@ START_TEST(environ) test_Predefined(); test_GetSetEnvironmentVariableA(); test_GetSetEnvironmentVariableW(); + test_GetSetEnvironmentVariableAW(); test_ExpandEnvironmentStringsA(); test_GetComputerName(); test_GetComputerNameExA(); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 3cd2934c20..1630afdb22 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1233,33 +1233,54 @@ LPWSTR WINAPI DECLSPEC_HOTPATCH GetEnvironmentStringsW(void) DWORD WINAPI DECLSPEC_HOTPATCH GetEnvironmentVariableA( LPCSTR name, LPSTR value, DWORD size ) { UNICODE_STRING us_name, us_value; - PWSTR valueW; NTSTATUS status; DWORD len, ret;
- /* limit the size to sane values */ - size = min( size, 32767 ); - if (!(valueW = HeapAlloc( GetProcessHeap(), 0, size * sizeof(WCHAR) ))) return 0; + if (!RtlCreateUnicodeStringFromAsciiz( &us_name, name )) + return 0;
- RtlCreateUnicodeStringFromAsciiz( &us_name, name ); + ret = 0; us_value.Length = 0; - us_value.MaximumLength = (size ? size - 1 : 0) * sizeof(WCHAR); - us_value.Buffer = valueW; - + us_value.MaximumLength = 0; + us_value.Buffer = NULL; status = RtlQueryEnvironmentVariable_U( NULL, &us_name, &us_value ); - len = us_value.Length / sizeof(WCHAR); - if (status == STATUS_BUFFER_TOO_SMALL) ret = len + 1; - else if (!set_ntstatus( status )) ret = 0; - else if (!size) ret = len + 1; + if (status != STATUS_BUFFER_TOO_SMALL && !set_ntstatus(status)) + goto cleanup; + + if (us_value.Length) + { + if (!(us_value.Buffer = HeapAlloc( GetProcessHeap(), 0, us_value.Length ))) + goto cleanup; + us_value.MaximumLength = us_value.Length; + status = RtlQueryEnvironmentVariable_U( NULL, &us_name, &us_value ); + if (!set_ntstatus( status )) + goto cleanup; + + len = WideCharToMultiByte( CP_ACP, 0, us_value.Buffer, us_value.Length / sizeof(WCHAR), + NULL, 0, NULL, NULL ); + if (!len) + goto cleanup; + } else + len = 0; /* an empty W string is always an empty A string */ + + if (len + 1 <= size) { - if (len) WideCharToMultiByte( CP_ACP, 0, valueW, len + 1, value, size, NULL, NULL ); + if (len) + { + if (!WideCharToMultiByte( CP_ACP, 0, us_value.Buffer, us_value.Length / sizeof(WCHAR), + value, len, NULL, NULL )) + goto cleanup; + } value[len] = 0; ret = len; } + else + ret = len + 1;
+cleanup: + if (us_value.Buffer) HeapFree( GetProcessHeap(), 0, us_value.Buffer ); RtlFreeUnicodeString( &us_name ); - HeapFree( GetProcessHeap(), 0, valueW ); return ret; }
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=63778
Your paranoid android.
=== debian10 (32 bit report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit French report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit Japanese:Japan report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit Chinese:China report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (32 bit WoW report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0
=== debian10 (64 bit WoW report) ===
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0
On Fri, 24 Jan 2020 at 14:35, Marvin testbot@winehq.org wrote:
kernel32: environ.c:111: Test failed: should not fail with zero size but GetLastError=0
Oops, forgot the test suite still depends on the correct ordering - I tested only the cumulative patch, sorry. Everything passes if the third patch in this series is moved to the front. Should I resubmit?
-
Marvin -
Vladimir Panteleev