[PATCH 0/2] MR7189: win32u: Allocate a separate user buffer when packing a large WM_COPYDATA message for user32.
Before this patch, WM_COPYDATA message data was packed at the end of the message and then passed to KeUserModeCallback() to call user mode callbacks. However, WM_COPYDATA can contain a large amount of data so KeUserModeCallback() can cause stack overflows. Manual tests on Windows show that WM_COPYDATA messages with data <= 2048 bytes do have their data packed at the end of the message. But for WM_COPYDATA messages with data > 2048 bytes, KiUserCallbackDispatcher() gets a 120 bytes packed arguments, which is the same size when packing WM_COPYDATA messages with no data. So for WM_COPYDATA messages with data > 2048 bytes, the data must be packed differently. Fix FCSExpress hangs after sending WM_COPYDATA messages. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7189
From: Zhiyi Zhang <zzhang(a)codeweavers.com> Before this patch, WM_COPYDATA message data was packed at the end of the message and then passed to KeUserModeCallback() to call user mode callbacks. However, WM_COPYDATA can contain a large amount of data so KeUserModeCallback() can cause stack overflows. Manual tests on Windows show that WM_COPYDATA messages with data <= 2048 bytes do have their data packed at the end of the message. But for WM_COPYDATA messages with data > 2048 bytes, KiUserCallbackDispatcher() gets a 120 bytes packed arguments, which is the same size when packing WM_COPYDATA messages with no data. So for WM_COPYDATA messages with data > 2048 bytes, the data must be packed differently. Fix FCSExpress hangs after sending WM_COPYDATA messages. --- dlls/user32/winproc.c | 5 ++- dlls/win32u/hook.c | 13 ++++--- dlls/win32u/message.c | 66 ++++++++++++++++++++++++++++++++---- dlls/win32u/win32u_private.h | 2 +- 4 files changed, 73 insertions(+), 13 deletions(-) diff --git a/dlls/user32/winproc.c b/dlls/user32/winproc.c index 33b4c6f884f..be16c9e3bd4 100644 --- a/dlls/user32/winproc.c +++ b/dlls/user32/winproc.c @@ -771,7 +771,10 @@ void unpack_message( HWND hwnd, UINT message, WPARAM *wparam, LPARAM *lparam, vo case WM_COPYDATA: { COPYDATASTRUCT *cds = buffer; - if (cds->lpData) cds->lpData = cds + 1; + /* If cbData <= 2048 bytes, the data is packed at the end of message buffer. Otherwise, + * cds->lpData points to an extra user buffer. See pack_user_message() for WM_COPYDATA */ + if (cds->lpData && cds->cbData <= 2048) + cds->lpData = cds + 1; break; } case EM_GETSEL: diff --git a/dlls/win32u/hook.c b/dlls/win32u/hook.c index d47aed4c6d6..1acaabe35ab 100644 --- a/dlls/win32u/hook.c +++ b/dlls/win32u/hook.c @@ -244,8 +244,9 @@ static LRESULT call_hook( struct win_hook_params *info, const WCHAR *module, siz HHOOK prev = thread_info->hook; BOOL prev_unicode = thread_info->hook_unicode; struct win_hook_params *params = info; + void *ret_ptr, *extra_buffer = NULL; + SIZE_T extra_buffer_size = 0; size_t reply_size; - void *ret_ptr; ULONG ret_len; size = FIELD_OFFSET( struct win_hook_params, module[module ? lstrlenW( module ) + 1 : 1] ); @@ -295,21 +296,21 @@ static LRESULT call_hook( struct win_hook_params *info, const WCHAR *module, siz CBT_CREATEWNDW *cbtc = (CBT_CREATEWNDW *)params->lparam; LPARAM lp = (LPARAM)cbtc->lpcs; pack_user_message( (char *)params + message_offset, message_size, - WM_CREATE, 0, lp, FALSE ); + WM_CREATE, 0, lp, FALSE, &extra_buffer ); } break; case WH_CALLWNDPROC: { CWPSTRUCT *cwp = (CWPSTRUCT *)((char *)params + lparam_offset); pack_user_message( (char *)params + message_offset, message_size, - cwp->message, cwp->wParam, cwp->lParam, ansi ); + cwp->message, cwp->wParam, cwp->lParam, ansi, &extra_buffer ); } break; case WH_CALLWNDPROCRET: { CWPRETSTRUCT *cwpret = (CWPRETSTRUCT *)((char *)params + lparam_offset); pack_user_message( (char *)params + message_offset, message_size, - cwpret->message, cwpret->wParam, cwpret->lParam, ansi ); + cwpret->message, cwpret->wParam, cwpret->lParam, ansi, &extra_buffer ); } break; } @@ -323,6 +324,8 @@ static LRESULT call_hook( struct win_hook_params *info, const WCHAR *module, siz { WARN("Too many hooks called recursively, skipping call.\n"); if (params != info) free( params ); + if (extra_buffer) + NtFreeVirtualMemory( GetCurrentProcess(), &extra_buffer, &extra_buffer_size, MEM_RELEASE ); return 0; } @@ -346,6 +349,8 @@ static LRESULT call_hook( struct win_hook_params *info, const WCHAR *module, siz thread_info->hook_call_depth--; if (params != info) free( params ); + if (extra_buffer) + NtFreeVirtualMemory( GetCurrentProcess(), &extra_buffer, &extra_buffer_size, MEM_RELEASE ); } return ret; diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index 2f1e14c82c8..0f075a87675 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -1688,7 +1688,11 @@ size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, case WM_COPYDATA: { const COPYDATASTRUCT *cds = lparam_ptr; - size = sizeof(*cds) + cds->cbData; + /* If cbData <= 2048 bytes, pack the data at the end of the message. Otherwise, pack the data + * in an extra user buffer to avoid potential stack overflows when calling KeUserModeCallback() + * because cbData can be very large. Manual tests of KiUserCallbackDispatcher() when receiving + * WM_COPYDATA messages show that Windows does similar things */ + size = cds->cbData <= 2048 ? sizeof(*cds) + cds->cbData : sizeof(*cds); break; } case WM_HELP: @@ -1777,15 +1781,20 @@ size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, * pack_user_message * * Copy message to a buffer for passing to client. + * + * ret_extra_buffer returns an extra user buffer allocated to store large message data, for example, + * for WM_COPYDATA. Call NtFreeVirtualMemory() for *ret_extra_buffer when done using it if it's not NULL. */ void pack_user_message( void *buffer, size_t size, UINT message, - WPARAM wparam, LPARAM lparam, BOOL ansi ) + WPARAM wparam, LPARAM lparam, BOOL ansi, void **ret_extra_buffer ) { const void *lparam_ptr = (const void *)lparam; void const *inline_ptr = (void *)0xffffffff; if (!size) return; + *ret_extra_buffer = NULL; + switch (message) { case WM_NCCREATE: @@ -1822,9 +1831,49 @@ void pack_user_message( void *buffer, size_t size, UINT message, case WM_COPYDATA: { const COPYDATASTRUCT *cds = lparam_ptr; - if (cds->lpData && cds->cbData) - memcpy( (char *)buffer + sizeof(*cds), cds->lpData, cds->cbData ); - size = sizeof(*cds); + + if (!cds->lpData) + { + size = sizeof(*cds); + } + else + { + /* If cbData <= 2048 bytes, pack the data at the end of the message. Otherwise, pack the data + * in an extra user buffer to avoid potential stack overflow when calling KeUserModeCallback() + * because cbData can be very large. Manual tests of KiUserCallbackDispatcher() when receiving + * WM_COPYDATA messages show that Windows does similar things */ + if (cds->cbData <= 2048) + { + memcpy( (char *)buffer + sizeof(*cds), cds->lpData, cds->cbData ); + size = sizeof(*cds); + } + else + { + COPYDATASTRUCT *tmp_cds = buffer; + SIZE_T extra_buffer_size; + unsigned int status; + + memcpy( tmp_cds, cds, sizeof(*cds) ); + + extra_buffer_size = cds->cbData; + status = NtAllocateVirtualMemory( GetCurrentProcess(), ret_extra_buffer, 0, + &extra_buffer_size, MEM_RESERVE | MEM_COMMIT, + PAGE_READWRITE ); + if (!status) + { + memcpy( *ret_extra_buffer, cds->lpData, cds->cbData ); + tmp_cds->lpData = *ret_extra_buffer; + return; + } + else + { + ERR( "Failed to allocate memory for packing WM_COPYDATA, status %#x.\n", status); + tmp_cds->lpData = NULL; + tmp_cds->cbData = 0; + return; + } + } + } break; } case EM_GETSEL: @@ -2216,10 +2265,11 @@ static LRESULT call_window_proc( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lpar struct win_proc_params p, *params = &p; BOOL ansi = ansi_dst && type == MSG_ASCII; size_t packed_size = 0, offset = sizeof(*params), reply_size; + void *ret_ptr, *extra_buffer = NULL; + SIZE_T extra_buffer_size = 0; LRESULT result = 0; CWPSTRUCT cwp; CWPRETSTRUCT cwpret; - void *ret_ptr; size_t ret_len = 0; if (msg & 0x80000000) @@ -2251,10 +2301,12 @@ static LRESULT call_window_proc( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lpar if (type == MSG_OTHER_PROCESS) params->ansi = FALSE; if (packed_size) - pack_user_message( (char *)params + offset, packed_size, msg, wparam, lparam, ansi ); + pack_user_message( (char *)params + offset, packed_size, msg, wparam, lparam, ansi, &extra_buffer ); result = dispatch_win_proc_params( params, offset + packed_size, &ret_ptr, &ret_len ); if (params != &p) free( params ); + if (extra_buffer) + NtFreeVirtualMemory( GetCurrentProcess(), &extra_buffer, &extra_buffer_size, MEM_RELEASE ); copy_user_result( ret_ptr, min( ret_len, reply_size ), result, msg, wparam, lparam, ansi ); diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index f1e286b74e7..6f0cfca5724 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -145,7 +145,7 @@ extern LRESULT send_message_timeout( HWND hwnd, UINT msg, WPARAM wparam, LPARAM extern size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, BOOL other_process, BOOL ansi, size_t *reply_size ); extern void pack_user_message( void *buffer, size_t size, UINT message, - WPARAM wparam, LPARAM lparam, BOOL ansi ); + WPARAM wparam, LPARAM lparam, BOOL ansi, void **extra_buffer ); /* rawinput.c */ extern BOOL process_rawinput_message( MSG *msg, UINT hw_id, const struct hardware_msg_data *msg_data ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7189
From: Zhiyi Zhang <zzhang(a)codeweavers.com> --- dlls/user32/tests/msg.c | 146 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 139 insertions(+), 7 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index baad938b6c7..33962efc790 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -2329,7 +2329,7 @@ static const struct message WmTrackPopupMenuAbort[] = { { 0 } }; -static BOOL after_end_dialog, test_def_id, paint_loop_done; +static BOOL after_end_dialog, test_def_id, paint_loop_done, wm_copydata_done; static int sequence_cnt, sequence_size; static struct recvd_message* sequence; static int log_all_parent_messages; @@ -11244,6 +11244,51 @@ static LRESULT WINAPI HotkeyMsgCheckProcA(HWND hwnd, UINT message, WPARAM wParam return ret; } +static LRESULT WINAPI WmCopyDataProcA(HWND hwnd, UINT msg, WPARAM wp, LPARAM lp) +{ + switch (msg) + { + case WM_COPYDATA: + { + static const DWORD expected_data_sizes[3] = {0, 64, 64 * 1024 * 1024}; + static ULONG_PTR expected_dwdata = 0; + COPYDATASTRUCT *cds = (COPYDATASTRUCT *)lp; + unsigned char *ptr; + unsigned int i; + BOOL matched; + + if (cds->dwData > 2) + return FALSE; + + ok(!wm_copydata_done, "Got unexpected wm_copydata_done.\n"); + ok(wp == (WPARAM)GetDesktopWindow(), "Got unexpected wp.\n"); + ok(cds->dwData == expected_dwdata, "Got unexpected dwData %Id.\n", cds->dwData); + expected_dwdata++; + ok(cds->cbData == expected_data_sizes[cds->dwData], "Got unexpected cbData %#lx.\n", cds->cbData); + + if (cds->dwData) + ok(!!cds->lpData, "Got unexpected lpData %p.\n", cds->lpData); + else + ok(!cds->lpData, "Got unexpected lpData %p.\n", cds->lpData); + + matched = TRUE; + for (i = 0, ptr = cds->lpData; i < cds->cbData; i++, ptr++) + { + if (*ptr != i % 0xff) + { + matched = FALSE; + break; + } + } + ok(matched, "Got unexpected content.\n"); + if (cds->dwData == 2) + wm_copydata_done = TRUE; + return TRUE; + } + } + return DefWindowProcA(hwnd,msg,wp,lp); +} + static void register_classes(void) { WNDCLASSA cls; @@ -11289,6 +11334,10 @@ static void register_classes(void) cls.lpszClassName = "PaintLoopWindowClass"; register_class(&cls); + cls.lpfnWndProc = WmCopyDataProcA; + cls.lpszClassName = "WmCopyDataWindowClass"; + register_class(&cls); + cls.style = CS_NOCLOSE; cls.lpszClassName = "NoCloseWindowClass"; register_class(&cls); @@ -18729,7 +18778,7 @@ static void test_WaitForInputIdle( char *argv0 ) { ResetEvent( start_event ); ResetEvent( end_event ); - sprintf( path, "%s msg %u", argv0, i ); + sprintf( path, "%s msg do_wait_idle_child %u", argv0, i ); ret = CreateProcessA( NULL, path, NULL, NULL, TRUE, 0, NULL, NULL, &startup, &pi ); ok( ret, "CreateProcess '%s' failed err %lu.\n", path, GetLastError() ); if (ret) @@ -20721,6 +20770,83 @@ static void test_hook_cleanup(void) DestroyWindow( d.hwnd ); } +static void test_WM_COPYDATA_child(void) +{ + HWND hwnd; + MSG msg; + + wm_copydata_done = FALSE; + hwnd = CreateWindowA("WmCopyDataWindowClass", "WmCopyDataWindow", + WS_OVERLAPPEDWINDOW | WS_VISIBLE, 100, 100, 100, 100, 0, 0, 0, NULL); + ok(!!hwnd, "CreateWindowA failed, error %lu\n", GetLastError()); + while (!wm_copydata_done) + { + if (PeekMessageA(&msg, 0, 0, 0, 1)) + { + TranslateMessage(&msg); + DispatchMessageA(&msg); + } + } + DestroyWindow(hwnd); +} + +static void test_WM_COPYDATA(char **argv) +{ + static const int LARGE_DATA_SIZE = 64 * 1024 * 1024; + unsigned char *ptr, *buffer; + unsigned int timeout = 0, i; + PROCESS_INFORMATION pi; + char cmdline[MAX_PATH]; + STARTUPINFOA si = {0}; + COPYDATASTRUCT cds; + HWND hwnd = NULL; + BOOL ret; + + sprintf(cmdline, "%s %s test_WM_COPYDATA_child", argv[0], argv[1]); + ret = CreateProcessA(NULL, cmdline, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi); + ok(ret, "CreateProcessA failed, error %lu\n", GetLastError()); + + do + { + hwnd = FindWindowA("WmCopyDataWindowClass", "WmCopyDataWindow"); + Sleep(100); + timeout += 100; + } while (!hwnd && timeout < 1000); + ok(!!hwnd, "Failed to find test window.\n"); + + buffer = malloc(LARGE_DATA_SIZE); + ok(!!buffer, "Failed to allocate memory.\n"); + for (i = 0, ptr = buffer; i < LARGE_DATA_SIZE; i++, ptr++) + *ptr = i % 0xff; + + /* Test a WM_COPYDATA message with no data */ + cds.dwData = 0; + cds.cbData = 0; + cds.lpData = NULL; + ret = SendMessageA(hwnd, WM_COPYDATA, (WPARAM)GetDesktopWindow(), (LPARAM)&cds); + ok(ret, "WM_COPYDATA failed.\n"); + + /* Test a WM_COPYDATA message with a small amount of data */ + cds.dwData = 1; + cds.cbData = 64; + cds.lpData = buffer; + ret = SendMessageA(hwnd, WM_COPYDATA, (WPARAM)GetDesktopWindow(), (LPARAM)&cds); + ok(ret, "WM_COPYDATA failed.\n"); + + /* Test a WM_COPYDATA message with a large amount of data */ + cds.dwData = 2; + cds.cbData = LARGE_DATA_SIZE; + cds.lpData = buffer; + ret = SendMessageA(hwnd, WM_COPYDATA, (WPARAM)GetDesktopWindow(), (LPARAM)&cds); + ok(ret, "WM_COPYDATA failed.\n"); + + free(buffer); + ret = WaitForSingleObject(pi.hProcess, 1000); + ok(!ret, "WaitForSingleObject failed, error %ld.\n", GetLastError()); + CloseHandle(pi.hProcess); + CloseHandle(pi.hThread); +} + START_TEST(msg) { char **test_argv; @@ -20728,12 +20854,19 @@ START_TEST(msg) BOOL (WINAPI *pIsWinEventHookInstalled)(DWORD)= 0;/*GetProcAddress(user32, "IsWinEventHookInstalled");*/ int argc; - argc = winetest_get_mainargs( &test_argv ); - if (argc >= 3) + register_classes(); + + argc = winetest_get_mainargs(&test_argv); + if (argc == 3 && !strcmp(test_argv[2], "test_WM_COPYDATA_child")) + { + test_WM_COPYDATA_child(); + return; + } + else if (argc >= 4 && !strcmp(test_argv[2], "do_wait_idle_child")) { unsigned int arg; /* Child process. */ - sscanf (test_argv[2], "%d", (unsigned int *) &arg); + sscanf (test_argv[3], "%d", (unsigned int *) &arg); do_wait_idle_child( arg ); return; } @@ -20742,8 +20875,6 @@ START_TEST(msg) init_procs(); ImmDisableIME(0); - register_classes(); - if (pSetWinEventHook) { hEvent_hook = pSetWinEventHook(EVENT_MIN, EVENT_MAX, @@ -20849,6 +20980,7 @@ START_TEST(msg) * which rely on active/foreground windows being correct. */ test_SetForegroundWindow(); + test_WM_COPYDATA(test_argv); UnhookWindowsHookEx(hCBT_hook); if (pUnhookWinEvent && hEvent_hook) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7189
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=151027 Your paranoid android. === debian11b (32 bit WoW report) === user32: msg.c:20642: Test failed: WM_SETFOCUS on default radiobutton: 2: the msg sequence is not complete: expected msg 0000 - actual winevent_hook 800b === debian11b (64 bit WoW report) === mf: mf.c:6785: Test failed: Unexpected hr 0. user32: input.c:4306: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 00000000007D00FE, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
[0001-ntdll-tests-Test-packed-WM_COPYDATA-message-size.txt](/uploads/28b4081d8b6a77b28596c623c0d31dc5/0001-ntdll-tests-Test-packed-WM_COPYDATA-message-size.txt) Manual tests for WM_COPYDATA passing through KiUserCallbackDispatcher(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7189#note_92701
participants (3)
-
Marvin -
Zhiyi Zhang -
Zhiyi Zhang (＠zhiyi)