[PATCH v5 0/1] MR2153: win32u: invalidate all cached keys after input
This patch addresses an issue in Second Life and potentially other multi-threaded applications which process WM_KEYDOWN in one thread and then verify that the key is "still down" with GetAsyncKeyState from another thread. Wine uses a per-thread key cache, resulting in inconsistent views of key status. Caches are now invalidated when an input event is injected by the driver or via SendInput. -- v5: win32u: invalidate all cached keys after input https://gitlab.winehq.org/wine/wine/-/merge_requests/2153
From: Henry Goffin <hgoffin(a)amazon.com> This patch addresses an issue in Second Life and potentially other multi-threaded applications which process WM_KEYDOWN in one thread and then verify that the key is "still down" with GetAsyncKeyState from another thread. Wine uses a per-thread key cache, resulting in inconsistent views of key status. Caches are now invalidated when an input event is injected by the driver or via SendInput. --- dlls/win32u/message.c | 17 ++++++++--------- include/wine/server_protocol.h | 1 - server/queue.c | 1 - server/trace.c | 1 - 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c index de1ca8cefbc..e9e1a0f2ad3 100644 --- a/dlls/win32u/message.c +++ b/dlls/win32u/message.c @@ -2585,13 +2585,12 @@ LRESULT send_internal_message_timeout( DWORD dest_pid, DWORD dest_tid, */ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *rawinput, UINT flags ) { - struct user_key_state_info *key_state_info = get_user_thread_info()->key_state; struct send_message_info info; int prev_x, prev_y, new_x, new_y; - INT counter = global_key_state_counter; USAGE hid_usage_page, hid_usage; NTSTATUS ret; BOOL wait; + BOOL affects_key_state = FALSE; info.type = MSG_HARDWARE; info.dest_tid = 0; @@ -2629,6 +2628,10 @@ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *r req->input.mouse.flags = input->mi.dwFlags; req->input.mouse.time = input->mi.time; req->input.mouse.info = input->mi.dwExtraInfo; + affects_key_state = !!(input->mi.dwFlags & (MOUSEEVENTF_LEFTDOWN | MOUSEEVENTF_LEFTUP | + MOUSEEVENTF_RIGHTDOWN | MOUSEEVENTF_RIGHTUP | + MOUSEEVENTF_MIDDLEDOWN | MOUSEEVENTF_MIDDLEUP | + MOUSEEVENTF_XDOWN | MOUSEEVENTF_XUP)); break; case INPUT_KEYBOARD: req->input.kbd.vkey = input->ki.wVk; @@ -2636,6 +2639,7 @@ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *r req->input.kbd.flags = input->ki.dwFlags; req->input.kbd.time = input->ki.time; req->input.kbd.info = input->ki.dwExtraInfo; + affects_key_state = TRUE; break; case INPUT_HARDWARE: req->input.hw.msg = input->hi.uMsg; @@ -2664,8 +2668,6 @@ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *r } break; } - if (key_state_info) wine_server_set_reply( req, key_state_info->state, - sizeof(key_state_info->state) ); ret = wine_server_call( req ); wait = reply->wait; prev_x = reply->prev_x; @@ -2677,11 +2679,8 @@ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *r if (!ret) { - if (key_state_info) - { - key_state_info->time = NtGetTickCount(); - key_state_info->counter = counter; - } + if (affects_key_state) + InterlockedIncrement( &global_key_state_counter ); /* force refreshing the key state cache */ if ((flags & SEND_HWMSG_INJECTED) && (prev_x != new_x || prev_y != new_y)) user_driver->pSetCursorPos( new_x, new_y ); } diff --git a/include/wine/server_protocol.h b/include/wine/server_protocol.h index c5e0182f42a..9dfe936057b 100644 --- a/include/wine/server_protocol.h +++ b/include/wine/server_protocol.h @@ -2768,7 +2768,6 @@ struct send_hardware_message_reply int prev_y; int new_x; int new_y; - /* VARARG(keystate,bytes); */ char __pad_28[4]; }; #define SEND_HWMSG_INJECTED 0x01 diff --git a/server/queue.c b/server/queue.c index 07a31b47370..d591fb9f847 100644 --- a/server/queue.c +++ b/server/queue.c @@ -2587,7 +2587,6 @@ DECL_HANDLER(send_hardware_message) reply->new_x = desktop->cursor.x; reply->new_y = desktop->cursor.y; - set_reply_data( desktop->keystate, size ); release_object( desktop ); } diff --git a/server/trace.c b/server/trace.c index b63bc386ee8..91bc3ae7af9 100644 --- a/server/trace.c +++ b/server/trace.c @@ -2662,7 +2662,6 @@ static void dump_send_hardware_message_reply( const struct send_hardware_message fprintf( stderr, ", prev_y=%d", req->prev_y ); fprintf( stderr, ", new_x=%d", req->new_x ); fprintf( stderr, ", new_y=%d", req->new_y ); - dump_varargs_bytes( ", keystate=", cur_size ); } static void dump_get_message_request( const struct get_message_request *req ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2153
Rémi Bernon (@rbernon) commented about dlls/win32u/message.c:
*/ NTSTATUS send_hardware_message( HWND hwnd, const INPUT *input, const RAWINPUT *rawinput, UINT flags ) { - struct user_key_state_info *key_state_info = get_user_thread_info()->key_state; struct send_message_info info; int prev_x, prev_y, new_x, new_y; - INT counter = global_key_state_counter; USAGE hid_usage_page, hid_usage; NTSTATUS ret; BOOL wait; + BOOL affects_key_state = FALSE; Nitpick: we usually keep variables with the same type declared on the same line.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_25829
Rémi Bernon (@rbernon) commented about server/queue.c:
struct desktop *desktop; unsigned int origin = (req->flags & SEND_HWMSG_INJECTED ? IMO_INJECTED : IMO_HARDWARE); struct msg_queue *sender = get_current_queue(); data_size_t size = min( 256, get_reply_max_size() );
I think this is unused now, and build will probably fail. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_25830
Also would you mind editing the commit title to match the usual format, which would be `win32u: Invalidate all cached keys after input.` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2153#note_25831
participants (3)
-
Henry Goffin -
Henry Goffin (＠AMZN-hgoffin)
-
Rémi Bernon