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; }