[PATCH v2 0/3] MR187: ntdll/tests: Test if RtlFormatMessage modifies va_list on error.
Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> -- v2: kernelbase: Pass va_list copy to internal RtlFormatMessage. kernel32/tests: Test if FormatMessage modifies va_list on error. ntdll/tests: Test if RtlFormatMessage modifies va_list on error. https://gitlab.winehq.org/wine/wine/-/merge_requests/187
From: Daniel Lehman <dlehman25(a)gmail.com> Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- dlls/ntdll/tests/rtlstr.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dlls/ntdll/tests/rtlstr.c b/dlls/ntdll/tests/rtlstr.c index 49f6ef65785..2aebe1deda1 100644 --- a/dlls/ntdll/tests/rtlstr.c +++ b/dlls/ntdll/tests/rtlstr.c @@ -2660,6 +2660,25 @@ static void WINAPIV testfmt( const WCHAR *src, const WCHAR *expect, ULONG width, ok( size == (lstrlenW(expect) + 1) * sizeof(WCHAR), "%s: wrong size %lu\n", debugstr_w(src), size ); } +static void testfmt_arg_eaten( const WCHAR *src, ... ) +{ + va_list args; + NTSTATUS status; + WCHAR *arg, buffer[1]; + ULONG size = 0xdeadbeef; + + buffer[0] = 0xcccc; + va_start( args, src ); + status = pRtlFormatMessage( src, 0, FALSE, FALSE, FALSE, &args, buffer, ARRAY_SIZE(buffer), &size ); + ok( status == STATUS_BUFFER_OVERFLOW, "%s: failed %lx\n", debugstr_w(src), status ); + todo_wine + ok( buffer[0] == 0xcccc, "%s: got %x\n", debugstr_w(src), buffer[0] ); + ok( size == 0xdeadbeef, "%s: wrong size %lu\n", debugstr_w(src), size ); + arg = va_arg( args, WCHAR * ); + ok( !wcscmp( L"unused", arg ), "%s: wrong arg %s\n", debugstr_w(src), debugstr_w(arg) ); + va_end( args ); +} + static void test_RtlFormatMessage(void) { WCHAR buffer[128]; @@ -2907,6 +2926,8 @@ static void test_RtlFormatMessage(void) ok( !lstrcmpW( buffer, L"abcxxxxxxx" ), "got %s\n", wine_dbgstr_w(buffer) ); ok( size == 0xdeadbeef, "wrong size %lu\n", size ); + /* va_arg is eaten even in case of buffer overflow */ + testfmt_arg_eaten( L"%1!s! %2!s!", L"eaten", L"unused" ); } START_TEST(rtlstr) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/187
From: Daniel Lehman <dlehman25(a)gmail.com> Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> --- dlls/kernel32/tests/format_msg.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dlls/kernel32/tests/format_msg.c b/dlls/kernel32/tests/format_msg.c index 2d317d1bb9e..e2a4f05ffdb 100644 --- a/dlls/kernel32/tests/format_msg.c +++ b/dlls/kernel32/tests/format_msg.c @@ -1005,6 +1005,25 @@ static void test_message_wrap(void) ok(!strcmp("short\r\nlong?line", out),"failed out=[%s]\n",out); } +static void test_message_arg_eaten( const WCHAR *src, ... ) +{ + DWORD ret; + va_list list; + WCHAR *arg, out[1]; + + out[0] = 0xcccc; + va_start(list, src); + SetLastError(0xdeadbeef); + ret = FormatMessageW(FORMAT_MESSAGE_FROM_STRING, src, 0, 0, out, ARRAY_SIZE(out), &list); + ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, + "Expected GetLastError() to return ERROR_INSUFFICIENT_BUFFER, got %lu\n", GetLastError()); + ok(ret == 0, "Expected FormatMessageW to return 0, got %lu\n", ret); + ok(out[0] == 0, "Expected null, got %ls\n", out); + arg = va_arg( list, WCHAR * ); + ok(!wcscmp( L"unused", arg ), "Expected 'unused', got %s\n", wine_dbgstr_w(arg)); + va_end(list); +} + static void test_message_insufficient_buffer(void) { static const char init_buf[] = {'x', 'x', 'x', 'x', 'x'}; @@ -1153,6 +1172,9 @@ static void test_message_insufficient_buffer_wide(void) HeapFree( GetProcessHeap(), 0, tmp ); HeapFree( GetProcessHeap(), 0, buf ); } + + /* va_arg is eaten even in case of insufficient buffer */ + test_message_arg_eaten( L"%1!s! %2!s!", L"eaten", L"unused" ); } static void test_message_null_buffer(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/187
This merge request was approved by Alexandre Julliard. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/187
From: Daniel Lehman <dlehman25(a)gmail.com> va_list passed to RtlFormatMessage is modified even on error in this case, if the buffer is not large enough, STATUS_BUFFER_OVERFLOW is returned and FormatMessage tries again, but the va_list pointer is now moved to a later argument, so the next call reads off the end, crashing. Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com> Signed-off-by: Alexandre Julliard <julliard(a)winehq.org> --- dlls/kernelbase/locale.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/dlls/kernelbase/locale.c b/dlls/kernelbase/locale.c index 257d9f693ba..0c20c858a7a 100644 --- a/dlls/kernelbase/locale.c +++ b/dlls/kernelbase/locale.c @@ -5457,6 +5457,7 @@ DWORD WINAPI DECLSPEC_HOTPATCH FormatMessageW( DWORD flags, const void *source, if (flags & FORMAT_MESSAGE_ALLOCATE_BUFFER) { WCHAR *result; + va_list args_copy; ULONG alloc = max( size * sizeof(WCHAR), 65536 ); for (;;) @@ -5466,9 +5467,17 @@ DWORD WINAPI DECLSPEC_HOTPATCH FormatMessageW( DWORD flags, const void *source, status = STATUS_NO_MEMORY; break; } - status = RtlFormatMessage( src, width, !!(flags & FORMAT_MESSAGE_IGNORE_INSERTS), - FALSE, !!(flags & FORMAT_MESSAGE_ARGUMENT_ARRAY), args, - result, alloc, &retsize ); + if (args && !(flags & FORMAT_MESSAGE_ARGUMENT_ARRAY)) + { + va_copy( args_copy, *args ); + status = RtlFormatMessage( src, width, !!(flags & FORMAT_MESSAGE_IGNORE_INSERTS), + FALSE, FALSE, &args_copy, result, alloc, &retsize ); + va_end( args_copy ); + } + else + status = RtlFormatMessage( src, width, !!(flags & FORMAT_MESSAGE_IGNORE_INSERTS), + FALSE, TRUE, args, result, alloc, &retsize ); + if (!status) { if (retsize <= sizeof(WCHAR)) HeapFree( GetProcessHeap(), 0, result ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/187
participants (3)
-
Alexandre Julliard (＠julliard) -
Daniel Lehman
-
Daniel Lehman (＠dlehman25)