[PATCH v4 0/2] MR4045: kernelbase: Rewrite ExpandEnvironmentStringsA to return ansi string size
We can't return the unicode string size, this returns sizes to small for multi-byte characters. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54295 -- v4: kernelbase: Rewrite ExpandEnvironmentStringsA to return ansi string size kernelbase: Rework ExpandEnvironmentStringsW error cases https://gitlab.winehq.org/wine/wine/-/merge_requests/4045
From: Fabian Maurer <dark.shadow4(a)web.de> It doesn't set error code ERROR_INSUFFICIENT_BUFFER, and doesn't null terminate the string either --- dlls/kernel32/tests/environ.c | 49 +++++++++++++++++++++++++++++++++++ dlls/kernelbase/process.c | 10 ++++--- 2 files changed, 56 insertions(+), 3 deletions(-) diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index c1a49e175be..2a15fb92dfd 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -379,6 +379,54 @@ static void test_GetSetEnvironmentVariableAW(void) ok(lstrcmpW(bufW, valueW) == 0, "expected %s, got %s\n", debugstr_w(valueW), debugstr_w(bufW)); } +static void test_ExpandEnvironmentStringsW(void) +{ + const WCHAR *value = L"Long long value"; + WCHAR buf[256]; + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, buf, 0); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"abcdef"), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, buf, 1); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"abcdef"), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, buf, 2); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"Lbcdef"), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, buf, 3); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"Locdef"), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef"); + ExpandEnvironmentStringsW(value, buf, 15); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"Long long valucdefabcdefabcdefabcdefabcdefabcdef"), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef"); + ExpandEnvironmentStringsW(value, buf, 16); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, value), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, NULL, 0); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"abcdef"), "got %s\n", debugstr_w(buf)); +} + static void test_ExpandEnvironmentStringsA(void) { const char* value="Long long value"; @@ -780,6 +828,7 @@ START_TEST(environ) test_GetSetEnvironmentVariableA(); test_GetSetEnvironmentVariableW(); test_GetSetEnvironmentVariableAW(); + test_ExpandEnvironmentStringsW(); test_ExpandEnvironmentStringsA(); test_GetComputerName(); test_GetComputerNameExA(); diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 4807e84594d..8aa732602ec 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1425,6 +1425,7 @@ DWORD WINAPI DECLSPEC_HOTPATCH ExpandEnvironmentStringsW( LPCWSTR src, LPWSTR ds UNICODE_STRING us_src, us_dst; NTSTATUS status; DWORD res; + WCHAR last_char; TRACE( "(%s %p %lu)\n", debugstr_w(src), dst, len ); @@ -1437,14 +1438,17 @@ DWORD WINAPI DECLSPEC_HOTPATCH ExpandEnvironmentStringsW( LPCWSTR src, LPWSTR ds us_dst.MaximumLength = len * sizeof(WCHAR); us_dst.Buffer = dst; + /* Tests show windows keeps the last character instead of a null terminator */ + if (dst && len) last_char = dst[len - 1]; res = 0; status = RtlExpandEnvironmentStrings_U( NULL, &us_src, &us_dst, &res ); res /= sizeof(WCHAR); - if (!set_ntstatus( status )) + if (status != STATUS_BUFFER_TOO_SMALL) { - if (status != STATUS_BUFFER_TOO_SMALL) return 0; - if (len && dst) dst[len - 1] = 0; + if (!set_ntstatus( status )) return 0; } + else if (dst && len) dst[len - 1] = last_char; + return res; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4045
From: Fabian Maurer <dark.shadow4(a)web.de> We can't return the unicode string size, this returns sizes to small for multi-byte characters. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54295 --- dlls/kernel32/tests/environ.c | 30 ++++++++++++++++++++++++++++++ dlls/kernelbase/process.c | 27 +++++++++++++++++---------- dlls/shlwapi/tests/shreg.c | 7 ++----- 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/dlls/kernel32/tests/environ.c b/dlls/kernel32/tests/environ.c index 2a15fb92dfd..abf09a9e1cc 100644 --- a/dlls/kernel32/tests/environ.c +++ b/dlls/kernel32/tests/environ.c @@ -441,8 +441,10 @@ static void test_ExpandEnvironmentStringsA(void) "ExpandEnvironmentStrings returned %ld\n", ret_size); /* Try to get the required buffer size 'the natural way' */ + SetLastError(0xdeadbeef); strcpy(buf, "%EnvVar%"); ret_size = ExpandEnvironmentStringsA(buf, NULL, 0); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); ok(ret_size == strlen(value)+1 || /* win98 */ ret_size == (strlen(value)+1)*2 || /* NT4 */ ret_size == strlen(value)+2 || /* win2k, XP, win2k3 */ @@ -459,7 +461,9 @@ static void test_ExpandEnvironmentStringsA(void) ret_size, lstrlenA(value)+1); /* Try with a buffer that's too small */ + SetLastError(0xdeadbeef); ret_size = ExpandEnvironmentStringsA(buf, buf1, 12); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); /* v5.1.2600.2945 (XP SP2) returns len + 2 here! */ ok(ret_size == strlen(value)+1 || ret_size == strlen(value)+2 || ret_size == (strlen(value)+1)*2 /* NT4 */, @@ -509,6 +513,32 @@ static void test_ExpandEnvironmentStringsA(void) SetEnvironmentVariableA("IndirectVar", NULL); SetEnvironmentVariableA("EnvVar", NULL); + + if (GetACP() == 932) /* shift-jis */ + { + const char* japanese_test = "\x88\xEA\x93\xF1\x8E\x4F\x8E\x6C\x8C\xDC\x98\x5A\x8E\xB5\x94\xAA\x8B\xE3"; /* Japanese Kanji for "one" to "nine", in shift-jis */ + int japanese_len = strlen(japanese_test); + + SetLastError(0xdeadbeef); + ret_size = ExpandEnvironmentStringsA(japanese_test, NULL, 0); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(ret_size >= japanese_len, "Needed at least %d, got %lu\n", japanese_len, ret_size); + + SetLastError(0xdeadbeef); + ret_size = ExpandEnvironmentStringsA(japanese_test, buf, ret_size); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(ret_size >= japanese_len, "Needed at least %d, got %lu\n", japanese_len, ret_size); + ok(!strcmp(buf, japanese_test), "Got %s\n", debugstr_a(buf)); + + SetLastError(0xdeadbeef); + ret_size = ExpandEnvironmentStringsA(japanese_test, buf, japanese_len / 2); /* Buffer too small */ + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(ret_size >= japanese_len, "Needed at least %d, got %lu\n", japanese_len, ret_size); + } + else + { + skip("Skipping japanese language tests\n"); + } } static void test_GetComputerName(void) diff --git a/dlls/kernelbase/process.c b/dlls/kernelbase/process.c index 8aa732602ec..8e604ec62fb 100644 --- a/dlls/kernelbase/process.c +++ b/dlls/kernelbase/process.c @@ -1400,23 +1400,30 @@ DWORD WINAPI DECLSPEC_HOTPATCH ExpandEnvironmentStringsA( LPCSTR src, LPSTR dst, { UNICODE_STRING us_src; PWSTR dstW = NULL; - DWORD ret; + DWORD count_neededW; + DWORD count_neededA = 0; RtlCreateUnicodeStringFromAsciiz( &us_src, src ); - if (count) - { - if (!(dstW = HeapAlloc(GetProcessHeap(), 0, count * sizeof(WCHAR)))) return 0; - ret = ExpandEnvironmentStringsW( us_src.Buffer, dstW, count); - if (ret) WideCharToMultiByte( CP_ACP, 0, dstW, ret, dst, count, NULL, NULL ); - } - else ret = ExpandEnvironmentStringsW( us_src.Buffer, NULL, 0 ); + /* We always need to call ExpandEnvironmentStringsW, since we need the result to calculate the needed buffer size */ + count_neededW = ExpandEnvironmentStringsW( us_src.Buffer, NULL, 0 ); + if (!(dstW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, count_neededW * sizeof(WCHAR)))) goto cleanup; + count_neededW = ExpandEnvironmentStringsW( us_src.Buffer, dstW, count_neededW); + + /* Calculate needed buffer */ + count_neededA = WideCharToMultiByte( CP_ACP, 0, dstW, count_neededW, NULL, 0, NULL, NULL ); + if (!count_neededA) goto cleanup; + + /* If provided buffer is enough, do actual conversion */ + if (count >= count_neededA) + count_neededA = WideCharToMultiByte( CP_ACP, 0, dstW, count_neededW, dst, count, NULL, NULL ); + +cleanup: RtlFreeUnicodeString( &us_src ); HeapFree( GetProcessHeap(), 0, dstW ); - return ret; + return count_neededA; } - /*********************************************************************** * ExpandEnvironmentStringsW (kernelbase.@) */ diff --git a/dlls/shlwapi/tests/shreg.c b/dlls/shlwapi/tests/shreg.c index bc411779c8f..75d4dfae7eb 100644 --- a/dlls/shlwapi/tests/shreg.c +++ b/dlls/shlwapi/tests/shreg.c @@ -290,11 +290,8 @@ static void test_SHQueryValueEx(void) broken(ERROR_SUCCESS == dwRet), /* < IE5.5*/ "Expected ERROR_MORE_DATA, got (%lu)\n", dwRet); - todo_wine - { - ok( (0 == strcmp("", buf)) || (0 == strcmp(sTestpath2, buf)), - "Expected empty or unexpanded string (win98), got (%s)\n", buf); - } + ok( (0 == strcmp("", buf)) || (0 == strcmp(sTestpath2, buf)), + "Expected empty or unexpanded string (win98), got (%s)\n", buf); ok( dwSize >= nUsedBuffer2 || broken(dwSize == (strlen("") + 1)), /* < IE 5.5 */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4045
Nikolay Sivov (@nsivov) commented about dlls/kernel32/tests/environ.c:
+static void test_ExpandEnvironmentStringsW(void) +{ + const WCHAR *value = L"Long long value"; + WCHAR buf[256]; + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, buf, 0); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"abcdef"), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, buf, 1); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"abcdef"), "got %s\n", debugstr_w(buf)); This does not test return values, or cases when expansion actually happens.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_53764
Nikolay Sivov (@nsivov) commented about dlls/kernel32/tests/environ.c:
+ wcscpy(buf, L"abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef"); + ExpandEnvironmentStringsW(value, buf, 15); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"Long long valucdefabcdefabcdefabcdefabcdefabcdef"), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdefabcdefabcdefabcdefabcdefabcdefabcdefabcdef"); + ExpandEnvironmentStringsW(value, buf, 16); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, value), "got %s\n", debugstr_w(buf)); + + SetLastError(0xdeadbeef); + wcscpy(buf, L"abcdef"); + ExpandEnvironmentStringsW(value, NULL, 0); + ok(GetLastError() == 0xdeadbeef, "got last error %ld\n", GetLastError()); + ok(!wcscmp(buf, L"abcdef"), "got %s\n", debugstr_w(buf)); A few unnecessary lines here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_53765
Nikolay Sivov (@nsivov) commented about dlls/kernelbase/process.c:
us_dst.Buffer = dst;
+ /* Tests show windows keeps the last character instead of a null terminator */ + if (dst && len) last_char = dst[len - 1]; res = 0; status = RtlExpandEnvironmentStrings_U( NULL, &us_src, &us_dst, &res ); res /= sizeof(WCHAR); - if (!set_ntstatus( status )) + if (status != STATUS_BUFFER_TOO_SMALL) { - if (status != STATUS_BUFFER_TOO_SMALL) return 0; - if (len && dst) dst[len - 1] = 0; + if (!set_ntstatus( status )) return 0; } + else if (dst && len) dst[len - 1] = last_char; +
There must be a better way. Instead of putting it back, it shouldn't have been written to in a first place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_53766
Nikolay Sivov (@nsivov) commented about dlls/kernelbase/process.c:
- if (ret) WideCharToMultiByte( CP_ACP, 0, dstW, ret, dst, count, NULL, NULL ); - } - else ret = ExpandEnvironmentStringsW( us_src.Buffer, NULL, 0 );
+ /* We always need to call ExpandEnvironmentStringsW, since we need the result to calculate the needed buffer size */ + count_neededW = ExpandEnvironmentStringsW( us_src.Buffer, NULL, 0 ); + if (!(dstW = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, count_neededW * sizeof(WCHAR)))) goto cleanup; + count_neededW = ExpandEnvironmentStringsW( us_src.Buffer, dstW, count_neededW); + + /* Calculate needed buffer */ + count_neededA = WideCharToMultiByte( CP_ACP, 0, dstW, count_neededW, NULL, 0, NULL, NULL ); + if (!count_neededA) goto cleanup; + + /* If provided buffer is enough, do actual conversion */ + if (count >= count_neededA) + count_neededA = WideCharToMultiByte( CP_ACP, 0, dstW, count_neededW, dst, count, NULL, NULL ); Do we have a test that shows that this condition is necessary?
It makes sense to add tests first, then remove todos to make it obvious. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_53767
It doesn't set error code ERROR_INSUFFICIENT_BUFFER, and doesn't null terminate the string either
Depending on how it really works we might want to separate this in two fixes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_53768
On Sat Nov 25 02:20:03 2023 +0000, Nikolay Sivov wrote:
It doesn't set error code ERROR_INSUFFICIENT_BUFFER, and doesn't null terminate the string either Depending on how it really works we might want to separate this in two fixes. Is it okay if I leave that MR open and rebase it later? Then I would just make a new MR for only the ERROR_INSUFFICIENT_BUFFER (and its tests) and rebase this MR later on.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_53776
On Sat Nov 25 01:15:47 2023 +0000, Nikolay Sivov wrote:
A few unnecessary lines here. Why unnecessary?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_53777
I'll postpone this until after codefreeze -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_54746
On Sat Nov 25 02:21:39 2023 +0000, Fabian Maurer wrote:
Why unnecessary? You're not passing in 'buf' argument.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_54786
On Sat Nov 25 02:20:03 2023 +0000, Fabian Maurer wrote:
Is it okay if I leave that MR open and rebase it later? Then I would just make a new MR for only the ERROR_INSUFFICIENT_BUFFER (and its tests) and rebase this MR later on. You can have both as separate commits on this MR.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4045#note_54787
participants (3)
-
Fabian Maurer -
Fabian Maurer (＠DarkShadow44)
-
Nikolay Sivov (＠nsivov)