[PATCH v2 0/2] MR4621: win32u: Provide extra spaces in buffers used to dispatch string getter messages.
This should fix recent CI failures. While investigating it, I noticed that larger buffers should not use stack. This part is not yet implemented. -- v2: user32: Remove unused argument for unpack_message. win32u: Provide extra space in buffers used to dispatch string getter messages. https://gitlab.winehq.org/wine/wine/-/merge_requests/4621
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/win32u/hook.c | 3 ++- dlls/win32u/message.c | 38 ++++++++++++++++++++++++------------ dlls/win32u/tests/win32u.c | 21 +++++++++++++++++++- dlls/win32u/win32u_private.h | 2 +- 4 files changed, 48 insertions(+), 16 deletions(-) diff --git a/dlls/win32u/hook.c b/dlls/win32u/hook.c index e9b3ae7d70d..378eda40abb 100644 --- a/dlls/win32u/hook.c +++ b/dlls/win32u/hook.c @@ -241,6 +241,7 @@ 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; + size_t reply_size; void *ret_ptr; ULONG ret_len; @@ -252,7 +253,7 @@ static LRESULT call_hook( struct win_hook_params *info, const WCHAR *module, siz { CBT_CREATEWNDW *cbtc = (CBT_CREATEWNDW *)params->lparam; message_size = user_message_size( (HWND)params->wparam, WM_NCCREATE, - 0, (LPARAM)cbtc->lpcs, TRUE, FALSE ); + 0, (LPARAM)cbtc->lpcs, TRUE, FALSE, &reply_size ); lparam_size = lparam_ret_size = 0; } diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index 56391977eae..91f836a60bd 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -1624,11 +1624,15 @@ static size_t copy_string( void *ptr, const void *str, BOOL ansi ) * Calculate size of packed message buffer. */ size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, - BOOL other_process, BOOL ansi ) + BOOL other_process, BOOL ansi, size_t *reply_size ) { const void *lparam_ptr = (const void *)lparam; size_t size = 0; + /* Windows provices space for at least 2048 bytes for string getters, which + * mitigates problems with buffer overflows. */ + static const size_t min_string_buffer_size = 2048; + switch (message) { case WM_NCCREATE: @@ -1645,8 +1649,8 @@ size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, break; case WM_GETTEXT: case WM_ASKCBFORMATNAME: - size = wparam * char_size( ansi ); - break; + *reply_size = wparam * char_size( ansi ); + return max( *reply_size, min_string_buffer_size ); case WM_WININICHANGE: case WM_SETTEXT: case WM_DEVMODECHANGE: @@ -1730,13 +1734,17 @@ size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, size = wparam * sizeof(UINT); break; case CB_GETLBTEXT: - size = send_message_timeout( hwnd, CB_GETLBTEXTLEN, wparam, 0, SMTO_NORMAL, 0, ansi ); - size = (size + 1) * char_size( ansi ); - break; + { + size_t len = send_message_timeout( hwnd, CB_GETLBTEXTLEN, wparam, 0, SMTO_NORMAL, 0, ansi ); + *reply_size = (len + 1) * char_size( ansi ); + return max( *reply_size, min_string_buffer_size ); + } case LB_GETTEXT: - size = send_message_timeout( hwnd, LB_GETTEXTLEN, wparam, 0, SMTO_NORMAL, 0, ansi ); - size = (size + 1) * char_size( ansi ); - break; + { + size_t len = send_message_timeout( hwnd, LB_GETTEXTLEN, wparam, 0, SMTO_NORMAL, 0, ansi ); + *reply_size = (len + 1) * char_size( ansi ); + return max( *reply_size, min_string_buffer_size ); + } case LB_GETSELITEMS: size = wparam * sizeof(UINT); break; @@ -1766,7 +1774,7 @@ size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, break; } - return size; + return *reply_size = size; } /*********************************************************************** @@ -1859,6 +1867,10 @@ void pack_user_message( void *buffer, size_t size, UINT message, return; } break; + case CB_GETLBTEXT: + case LB_GETTEXT: + if (size) memset( buffer, 0, size ); + return; } if (size) memcpy( buffer, lparam_ptr, size ); @@ -2167,7 +2179,7 @@ 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); + size_t packed_size = 0, offset = sizeof(*params), reply_size; LRESULT result = 0; CWPSTRUCT cwp; CWPRETSTRUCT cwpret; @@ -2179,7 +2191,7 @@ static LRESULT call_window_proc( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lpar if (!is_current_thread_window( hwnd )) return 0; - packed_size = user_message_size( hwnd, msg, wparam, lparam, type == MSG_OTHER_PROCESS, ansi ); + packed_size = user_message_size( hwnd, msg, wparam, lparam, type == MSG_OTHER_PROCESS, ansi, &reply_size ); /* first the WH_CALLWNDPROC hook */ cwp.lParam = lparam; @@ -2208,7 +2220,7 @@ static LRESULT call_window_proc( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lpar result = dispatch_win_proc_params( params, offset + packed_size, &ret_ptr, &ret_len ); if (params != &p) free( params ); - copy_user_result( ret_ptr, min( ret_len, packed_size ), result, msg, wparam, lparam, ansi ); + copy_user_result( ret_ptr, min( ret_len, reply_size ), result, msg, wparam, lparam, ansi ); /* and finally the WH_CALLWNDPROCRET hook */ cwpret.lResult = result; diff --git a/dlls/win32u/tests/win32u.c b/dlls/win32u/tests/win32u.c index e118d081ead..f7fe74ed34c 100644 --- a/dlls/win32u/tests/win32u.c +++ b/dlls/win32u/tests/win32u.c @@ -1382,6 +1382,19 @@ static LPARAM callwnd_hook_lparam, callwnd_hook_lparam2, retwnd_hook_lparam, ret static LPARAM wndproc_lparam; static char lparam_buffer[521]; +static void check_zero_memory( const char *mem, size_t size ) +{ + size_t i; + for (i = 0; i < size; i++) + { + if (mem[i]) + { + ok( 0, "non-zero byte %x at offset %Iu\n", mem[i], i ); + return; + } + } +} + static void check_params( const struct lparam_hook_test *test, UINT message, WPARAM wparam, LPARAM lparam, BOOL is_ret ) { @@ -1428,8 +1441,14 @@ static void check_params( const struct lparam_hook_test *test, UINT message, } break; + case CB_GETLBTEXT: + case LB_GETTEXT: + check_zero_memory( (const char *)lparam, 2048 ); + break; + default: - if (test->check_size) { + if (test->check_size) + { const void *expected; if (is_ret && test->check_lparam) expected = test->check_lparam; diff --git a/dlls/win32u/win32u_private.h b/dlls/win32u/win32u_private.h index 65105c4c1a6..b08c0ceb716 100644 --- a/dlls/win32u/win32u_private.h +++ b/dlls/win32u/win32u_private.h @@ -141,7 +141,7 @@ extern BOOL send_notify_message( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lpar extern LRESULT send_message_timeout( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam, UINT flags, UINT timeout, BOOL ansi ); extern size_t user_message_size( HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam, - BOOL other_process, BOOL ansi ); + 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 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4621
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/user32/hook.c | 6 +++--- dlls/user32/user_private.h | 2 +- dlls/user32/winproc.c | 5 ++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/dlls/user32/hook.c b/dlls/user32/hook.c index b2e213f42fe..024bb5b7116 100644 --- a/dlls/user32/hook.c +++ b/dlls/user32/hook.c @@ -478,7 +478,7 @@ BOOL WINAPI User32CallWindowsHook( struct win_hook_params *params, ULONG size ) { cbtc.hwndInsertAfter = HWND_TOP; unpack_message( (HWND)params->wparam, WM_CREATE, NULL, (LPARAM *)&cbtc.lpcs, - ret_ptr, ret_size, FALSE ); + ret_ptr, FALSE ); params->lparam = (LPARAM)&cbtc; ret_size = sizeof(*cbtc.lpcs); } @@ -490,7 +490,7 @@ BOOL WINAPI User32CallWindowsHook( struct win_hook_params *params, ULONG size ) size_t offset = (lparam_offset + sizeof(*cwp) + 15) & ~15; unpack_message( cwp->hwnd, cwp->message, &cwp->wParam, &cwp->lParam, - (char *)params + offset, size - offset, !params->prev_unicode ); + (char *)params + offset, !params->prev_unicode ); ret_size = 0; break; } @@ -501,7 +501,7 @@ BOOL WINAPI User32CallWindowsHook( struct win_hook_params *params, ULONG size ) size_t offset = (lparam_offset + sizeof(*cwpret) + 15) & ~15; unpack_message( cwpret->hwnd, cwpret->message, &cwpret->wParam, &cwpret->lParam, - (char *)params + offset, size - offset, !params->prev_unicode ); + (char *)params + offset, !params->prev_unicode ); ret_size = 0; break; } diff --git a/dlls/user32/user_private.h b/dlls/user32/user_private.h index a9bbdd16fb0..50d4005d973 100644 --- a/dlls/user32/user_private.h +++ b/dlls/user32/user_private.h @@ -52,7 +52,7 @@ extern BOOL unpack_dde_message( HWND hwnd, UINT message, WPARAM *wparam, LPARAM extern void free_cached_data( UINT format, HANDLE handle ); extern HANDLE render_synthesized_format( UINT format, UINT from ); extern void unpack_message( HWND hwnd, UINT message, WPARAM *wparam, LPARAM *lparam, - void *buffer, size_t size, BOOL ansi ); + void *buffer, BOOL ansi ); extern void CLIPBOARD_ReleaseOwner( HWND hwnd ); extern HDC get_display_dc(void); diff --git a/dlls/user32/winproc.c b/dlls/user32/winproc.c index 8804199aed1..d2e4b29fc44 100644 --- a/dlls/user32/winproc.c +++ b/dlls/user32/winproc.c @@ -738,8 +738,7 @@ static size_t string_size( const void *str, BOOL ansi ) * * Unpack a message received from win32u. */ -void unpack_message( HWND hwnd, UINT message, WPARAM *wparam, LPARAM *lparam, - void *buffer, size_t size, BOOL ansi ) +void unpack_message( HWND hwnd, UINT message, WPARAM *wparam, LPARAM *lparam, void *buffer, BOOL ansi ) { switch(message) { @@ -816,7 +815,7 @@ BOOL WINAPI User32CallWindowProc( struct win_proc_params *params, ULONG size ) buffer = (char *)params + offset; unpack_message( params->hwnd, params->msg, ¶ms->wparam, ¶ms->lparam, - buffer, packed_size, params->ansi ); + buffer, params->ansi ); } result = dispatch_win_proc_params( params ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4621
On Wed Dec 6 21:30:01 2023 +0000, Jacek Caban wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/4621/diffs?diff_id=88498&start_sha=dc547331f47cfbc4b8ed4262d864d104b891abae#bddee5ea9d993e9a0987591fc4e612261c3a87a3_1632_1632) Yes, good catch, thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4621#note_55263
participants (2)
-
Jacek Caban -
Jacek Caban (＠jacek)