[PATCH] kernelbase: Fix GetEnvironmentVariableA with multi-byte strings.

6 Aug 2020

From: Vladimir Panteleev <git(a)vladimir.panteleev.md>

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(a)vladimir.panteleev.md>
Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com>
---
 dlls/kernel32/tests/environ.c | 62 +++++++++++++++++++++++++++++++++++
 dlls/kernelbase/process.c     | 49 +++++++++++++++++++--------
 2 files changed, 97 insertions(+), 14 deletions(-)

diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c
index 3c1563a331..de6db899c9 100644
--- a/dlls/kernel32/tests/environ.c
+++ b/dlls/kernel32/tests/environ.c
@@ -320,6 +320,67 @@ static void test_GetSetEnvironmentVariableW(void)
     }
 }
 
+static void test_GetSetEnvironmentVariableAW(void)
+{
+    static const WCHAR nameW[] = {0x540D, 0x524D, 0};
+    static const char name[] = "\x96\xBC\x91\x4F";
+    static const WCHAR valueW[] = {0x5024, 0};
+    static const char value[] = "\x92\x6C";
+    WCHAR bufW[256];
+    char buf[256];
+    DWORD ret_size;
+    BOOL ret;
+
+    if (GetACP() != 932)
+    {
+        skip("GetACP() == %d, need 932 for A/W tests\n", GetACP());
+        return;
+    }
+
+    /* Write W, read A */
+    ret = SetEnvironmentVariableW(nameW, valueW);
+    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, L"foo");
+    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";
@@ -720,6 +781,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 58fba52415..3bb309e988 100644
--- a/dlls/kernelbase/process.c
+++ b/dlls/kernelbase/process.c
@@ -1460,33 +1460,54 @@ BOOL WINAPI DECLSPEC_HOTPATCH SetEnvironmentStringsW( WCHAR *env )
 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;
 }
 
-- 
2.28.0

    

Gijs Vermeulen

Alexandre Julliard

jancren＠charter.net

tags

participants (3)