[PATCH 0/2] MR1949: Draft: win32u: Correct GetRawInputBuffer alignment under WoW64.
The existing mitigation for this (memcpy/overhead) leaves the first 8 bytes of RAWINPUT.data in the location of the wParam, and the upper 4 bytes of the wParam in the lower 4 bytes of the hDevice. Marking this as a draft due to code freeze. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1949
From: Tim Clem <tclem(a)codeweavers.com> Exposes an alignment issue under WoW64. --- dlls/user32/tests/input.c | 43 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index bf913b8e6a6..c2b2e4ded80 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -2296,13 +2296,15 @@ static LRESULT CALLBACK rawinputbuffer_wndproc(HWND hwnd, UINT msg, WPARAM wpara static void test_GetRawInputBuffer(void) { - unsigned int size, count, rawinput_size, header_size; + unsigned int size, count, rawinput_size, header_size, scan_code; RAWINPUTDEVICE raw_devices[1]; char buffer[16 * sizeof(RAWINPUT64)]; HWND hwnd; BOOL ret; POINT pt; +#define HEADER_FIELD(field) (is_wow64 ? ((RAWINPUT64 *)buffer)->header.field : ((RAWINPUT *)buffer)->header.field) + if (is_wow64) rawinput_size = sizeof(RAWINPUT64); else rawinput_size = sizeof(RAWINPUT); @@ -2392,6 +2394,8 @@ static void test_GetRawInputBuffer(void) count = GetRawInputBuffer((RAWINPUT*)buffer, &size, sizeof(RAWINPUTHEADER)); ok(count == 1U, "GetRawInputBuffer returned %u\n", count); ok(size == sizeof(buffer), "GetRawInputBuffer returned unexpected size: %u\n", size); + ok(HEADER_FIELD(dwType) == RIM_TYPEMOUSE, "Unexpected rawinput dwType: %ld\n", HEADER_FIELD(dwType)); + ok(HEADER_FIELD(wParam) == 0 || HEADER_FIELD(wParam) == 1, "Expected wparam 0 or 1, got %Iu\n", (WPARAM)HEADER_FIELD(wParam)); ok(rawinput_buffer_mouse_x(buffer, 0) == 5, "Unexpected rawinput data: %d\n", rawinput_buffer_mouse_x(buffer, 0)); @@ -2428,7 +2432,44 @@ static void test_GetRawInputBuffer(void) ok(ret, "RegisterRawInputDevices failed\n"); ok(GetLastError() == 0xdeadbeef, "RegisterRawInputDevices returned %08lx\n", GetLastError()); + + /* some keyboard tests to better check fields under wow64 */ + raw_devices[0].usUsagePage = 0x01; + raw_devices[0].usUsage = 0x06; + raw_devices[0].dwFlags = RIDEV_INPUTSINK; + raw_devices[0].hwndTarget = hwnd; + + SetLastError(0xdeadbeef); + ret = RegisterRawInputDevices(raw_devices, ARRAY_SIZE(raw_devices), sizeof(RAWINPUTDEVICE)); + ok(ret, "RegisterRawInputDevices failed\n"); + ok(GetLastError() == 0xdeadbeef, "RegisterRawInputDevices returned %08lx\n", GetLastError()); + + keybd_event('X', 0x2d, 0, 0); + keybd_event('X', 0x2d, KEYEVENTF_KEYUP, 0); + + size = sizeof(buffer); + memset(buffer, 0, sizeof(buffer)); + count = GetRawInputBuffer((RAWINPUT*)buffer, &size, sizeof(RAWINPUTHEADER)); + ok(count == 2U, "GetRawInputBuffer returned %u\n", count); + ok(size == sizeof(buffer), "GetRawInputBuffer returned unexpected size: %u\n", size); + + ok(HEADER_FIELD(dwType) == RIM_TYPEKEYBOARD, "Unexpected rawinput dwType: %ld\n", HEADER_FIELD(dwType)); + todo_wine_if(is_wow64) + ok(HEADER_FIELD(wParam) == 0 || HEADER_FIELD(wParam) == 1, "Expected wparam 0 or 1, got %Iu\n", (WPARAM)HEADER_FIELD(wParam)); + scan_code = is_wow64 ? ((RAWINPUT64 *)buffer)->data.keyboard.MakeCode : ((RAWINPUT *)buffer)->data.keyboard.MakeCode; + ok(scan_code == 0x2d, "Unexpected rawinput keyboard scan code: %x\n", scan_code); + + raw_devices[0].dwFlags = RIDEV_REMOVE; + raw_devices[0].hwndTarget = 0; + + SetLastError(0xdeadbeef); + ret = RegisterRawInputDevices(raw_devices, ARRAY_SIZE(raw_devices), sizeof(RAWINPUTDEVICE)); + ok(ret, "RegisterRawInputDevices failed\n"); + ok(GetLastError() == 0xdeadbeef, "RegisterRawInputDevices returned %08lx\n", GetLastError()); + DestroyWindow(hwnd); + +#undef HEADER_FIELD } static BOOL rawinput_test_received_legacy; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1949
From: Tim Clem <tclem(a)codeweavers.com> --- dlls/user32/tests/input.c | 1 - dlls/win32u/rawinput.c | 172 ++++++++++++++++++++++++-------------- 2 files changed, 111 insertions(+), 62 deletions(-) diff --git a/dlls/user32/tests/input.c b/dlls/user32/tests/input.c index c2b2e4ded80..95abdbb601b 100644 --- a/dlls/user32/tests/input.c +++ b/dlls/user32/tests/input.c @@ -2454,7 +2454,6 @@ static void test_GetRawInputBuffer(void) ok(size == sizeof(buffer), "GetRawInputBuffer returned unexpected size: %u\n", size); ok(HEADER_FIELD(dwType) == RIM_TYPEKEYBOARD, "Unexpected rawinput dwType: %ld\n", HEADER_FIELD(dwType)); - todo_wine_if(is_wow64) ok(HEADER_FIELD(wParam) == 0 || HEADER_FIELD(wParam) == 1, "Expected wparam 0 or 1, got %Iu\n", (WPARAM)HEADER_FIELD(wParam)); scan_code = is_wow64 ? ((RAWINPUT64 *)buffer)->data.keyboard.MakeCode : ((RAWINPUT *)buffer)->data.keyboard.MakeCode; ok(scan_code == 0x2d, "Unexpected rawinput keyboard scan code: %x\n", scan_code); diff --git a/dlls/win32u/rawinput.c b/dlls/win32u/rawinput.c index ab1252ccea6..f4b5d65b335 100644 --- a/dlls/win32u/rawinput.c +++ b/dlls/win32u/rawinput.c @@ -40,6 +40,13 @@ WINE_DEFAULT_DEBUG_CHANNEL(rawinput); #define WINE_MOUSE_HANDLE ((HANDLE)1) #define WINE_KEYBOARD_HANDLE ((HANDLE)2) +union rawinput_data +{ + RAWMOUSE mouse; + RAWKEYBOARD keyboard; + RAWHID hid; +}; + #ifdef _WIN64 typedef RAWINPUTHEADER RAWINPUTHEADER64; typedef RAWINPUT RAWINPUT64; @@ -55,12 +62,7 @@ typedef struct typedef struct { RAWINPUTHEADER64 header; - union - { - RAWMOUSE mouse; - RAWKEYBOARD keyboard; - RAWHID hid; - } data; + union rawinput_data data; } RAWINPUT64; #endif @@ -73,11 +75,57 @@ static struct rawinput_thread_data *get_rawinput_thread_data(void) return data; } -static bool rawinput_from_hardware_message( RAWINPUT *rawinput, const struct hardware_msg_data *msg_data ) +static void set_rawinput_header( void *rawinput_buffer, bool force_64_bit, DWORD type, DWORD size, HANDLE device, WPARAM wParam ) { - SIZE_T size; + if (force_64_bit) + { + RAWINPUTHEADER64 *header = (RAWINPUTHEADER64 *)rawinput_buffer; + header->dwType = type; + header->dwSize = size; + +#ifdef _WIN64 + header->hDevice = device; + header->wParam = wParam; +#else + header->hDevice = HandleToULong(device); + header->wParam = (ULONGLONG)wParam; +#endif + } + else + { + RAWINPUTHEADER *header = (RAWINPUTHEADER *)rawinput_buffer; + header->dwType = type; + header->dwSize = size; + header->hDevice = device; + header->wParam = wParam; + } +} + +static bool rawinput_from_hardware_message( void *rawinput_buffer, const struct hardware_msg_data *msg_data, bool force_64_bit ) +{ + /* Under WoW64, GetRawInputBuffer always gives 64-bit RAWINPUT structs. + * GetRawInputData, however, gives the 32-bit version. If force_64_bit + * is true, this function will treat rawinput_buffer as a RAWINPUT64. + * Otherwise, it's treated as a (platform-correct) RAWINPUT. */ + + union rawinput_data *data; + SIZE_T rawinput_size, incoming_size; + +#define RAWINPUT_FIELD_OFFSET(field) (force_64_bit ? FIELD_OFFSET(RAWINPUT64, field) : FIELD_OFFSET(RAWINPUT, field)) + + if (force_64_bit) + { + rawinput_size = sizeof(RAWINPUT64); + incoming_size = ((RAWINPUT64 *)rawinput_buffer)->header.dwSize; + data = (union rawinput_data *)&((RAWINPUT64 *)rawinput_buffer)->data; + } + else + { + rawinput_size = sizeof(RAWINPUT); + incoming_size = ((RAWINPUT *)rawinput_buffer)->header.dwSize; + data = (union rawinput_data *)&((RAWINPUT *)rawinput_buffer)->data; + } - rawinput->header.dwType = msg_data->rawinput.type; if (msg_data->rawinput.type == RIM_TYPEMOUSE) { static const unsigned int button_flags[] = @@ -92,98 +140,104 @@ static bool rawinput_from_hardware_message( RAWINPUT *rawinput, const struct har }; unsigned int i; - rawinput->header.dwSize = FIELD_OFFSET(RAWINPUT, data) + sizeof(RAWMOUSE); - rawinput->header.hDevice = WINE_MOUSE_HANDLE; - rawinput->header.wParam = 0; + set_rawinput_header( rawinput_buffer, force_64_bit, + RIM_TYPEMOUSE, + RAWINPUT_FIELD_OFFSET(data) + sizeof(RAWMOUSE), + WINE_MOUSE_HANDLE, + 0 ); - rawinput->data.mouse.usFlags = MOUSE_MOVE_RELATIVE; - rawinput->data.mouse.usButtonFlags = 0; - rawinput->data.mouse.usButtonData = 0; + data->mouse.usFlags = MOUSE_MOVE_RELATIVE; + data->mouse.usButtonFlags = 0; + data->mouse.usButtonData = 0; for (i = 1; i < ARRAY_SIZE(button_flags); ++i) { if (msg_data->flags & (1 << i)) - rawinput->data.mouse.usButtonFlags |= button_flags[i]; + data->mouse.usButtonFlags |= button_flags[i]; } if (msg_data->flags & MOUSEEVENTF_WHEEL) { - rawinput->data.mouse.usButtonFlags |= RI_MOUSE_WHEEL; - rawinput->data.mouse.usButtonData = msg_data->rawinput.mouse.data; + data->mouse.usButtonFlags |= RI_MOUSE_WHEEL; + data->mouse.usButtonData = msg_data->rawinput.mouse.data; } if (msg_data->flags & MOUSEEVENTF_HWHEEL) { - rawinput->data.mouse.usButtonFlags |= RI_MOUSE_HORIZONTAL_WHEEL; - rawinput->data.mouse.usButtonData = msg_data->rawinput.mouse.data; + data->mouse.usButtonFlags |= RI_MOUSE_HORIZONTAL_WHEEL; + data->mouse.usButtonData = msg_data->rawinput.mouse.data; } if (msg_data->flags & MOUSEEVENTF_XDOWN) { if (msg_data->rawinput.mouse.data == XBUTTON1) - rawinput->data.mouse.usButtonFlags |= RI_MOUSE_BUTTON_4_DOWN; + data->mouse.usButtonFlags |= RI_MOUSE_BUTTON_4_DOWN; else if (msg_data->rawinput.mouse.data == XBUTTON2) - rawinput->data.mouse.usButtonFlags |= RI_MOUSE_BUTTON_5_DOWN; + data->mouse.usButtonFlags |= RI_MOUSE_BUTTON_5_DOWN; } if (msg_data->flags & MOUSEEVENTF_XUP) { if (msg_data->rawinput.mouse.data == XBUTTON1) - rawinput->data.mouse.usButtonFlags |= RI_MOUSE_BUTTON_4_UP; + data->mouse.usButtonFlags |= RI_MOUSE_BUTTON_4_UP; else if (msg_data->rawinput.mouse.data == XBUTTON2) - rawinput->data.mouse.usButtonFlags |= RI_MOUSE_BUTTON_5_UP; + data->mouse.usButtonFlags |= RI_MOUSE_BUTTON_5_UP; } - rawinput->data.mouse.ulRawButtons = 0; - rawinput->data.mouse.lLastX = msg_data->rawinput.mouse.x; - rawinput->data.mouse.lLastY = msg_data->rawinput.mouse.y; - rawinput->data.mouse.ulExtraInformation = msg_data->info; + data->mouse.ulRawButtons = 0; + data->mouse.lLastX = msg_data->rawinput.mouse.x; + data->mouse.lLastY = msg_data->rawinput.mouse.y; + data->mouse.ulExtraInformation = msg_data->info; } else if (msg_data->rawinput.type == RIM_TYPEKEYBOARD) { - rawinput->header.dwSize = FIELD_OFFSET(RAWINPUT, data) + sizeof(RAWKEYBOARD); - rawinput->header.hDevice = WINE_KEYBOARD_HANDLE; - rawinput->header.wParam = 0; + set_rawinput_header( rawinput_buffer, force_64_bit, + RIM_TYPEKEYBOARD, + RAWINPUT_FIELD_OFFSET(data) + sizeof(RAWKEYBOARD), + WINE_KEYBOARD_HANDLE, + 0 ); - rawinput->data.keyboard.MakeCode = msg_data->rawinput.kbd.scan; - rawinput->data.keyboard.Flags = (msg_data->flags & KEYEVENTF_KEYUP) ? RI_KEY_BREAK : RI_KEY_MAKE; + data->keyboard.MakeCode = msg_data->rawinput.kbd.scan; + data->keyboard.Flags = (msg_data->flags & KEYEVENTF_KEYUP) ? RI_KEY_BREAK : RI_KEY_MAKE; if (msg_data->flags & KEYEVENTF_EXTENDEDKEY) - rawinput->data.keyboard.Flags |= RI_KEY_E0; - rawinput->data.keyboard.Reserved = 0; + data->keyboard.Flags |= RI_KEY_E0; + data->keyboard.Reserved = 0; switch (msg_data->rawinput.kbd.vkey) { case VK_LSHIFT: case VK_RSHIFT: - rawinput->data.keyboard.VKey = VK_SHIFT; - rawinput->data.keyboard.Flags &= ~RI_KEY_E0; + data->keyboard.VKey = VK_SHIFT; + data->keyboard.Flags &= ~RI_KEY_E0; break; case VK_LCONTROL: case VK_RCONTROL: - rawinput->data.keyboard.VKey = VK_CONTROL; + data->keyboard.VKey = VK_CONTROL; break; case VK_LMENU: case VK_RMENU: - rawinput->data.keyboard.VKey = VK_MENU; + data->keyboard.VKey = VK_MENU; break; default: - rawinput->data.keyboard.VKey = msg_data->rawinput.kbd.vkey; + data->keyboard.VKey = msg_data->rawinput.kbd.vkey; break; } - rawinput->data.keyboard.Message = msg_data->rawinput.kbd.message; - rawinput->data.keyboard.ExtraInformation = msg_data->info; + data->keyboard.Message = msg_data->rawinput.kbd.message; + data->keyboard.ExtraInformation = msg_data->info; } else if (msg_data->rawinput.type == RIM_TYPEHID) { - size = msg_data->size - sizeof(*msg_data); - if (size > rawinput->header.dwSize - sizeof(*rawinput)) return false; + SIZE_T data_size = msg_data->size - sizeof(*msg_data); + if (data_size > incoming_size - rawinput_size) return false; - rawinput->header.dwSize = FIELD_OFFSET( RAWINPUT, data.hid.bRawData ) + size; - rawinput->header.hDevice = ULongToHandle( msg_data->rawinput.hid.device ); - rawinput->header.wParam = 0; + set_rawinput_header( rawinput_buffer, force_64_bit, + RIM_TYPEHID, + RAWINPUT_FIELD_OFFSET(data.hid.bRawData) + data_size, + ULongToHandle( msg_data->rawinput.hid.device ), + 0 ); - rawinput->data.hid.dwCount = msg_data->rawinput.hid.count; - rawinput->data.hid.dwSizeHid = msg_data->rawinput.hid.length; - memcpy( rawinput->data.hid.bRawData, msg_data + 1, size ); + data->hid.dwCount = msg_data->rawinput.hid.count; + data->hid.dwSizeHid = msg_data->rawinput.hid.length; + memcpy( data->hid.bRawData, msg_data + 1, data_size ); } else { @@ -192,6 +246,8 @@ static bool rawinput_from_hardware_message( RAWINPUT *rawinput, const struct har } return true; + +#undef RAWINPUT_FIELD_OFFSET } struct device @@ -609,17 +665,17 @@ UINT WINAPI NtUserGetRawInputDeviceInfo( HANDLE handle, UINT command, void *data */ UINT WINAPI NtUserGetRawInputBuffer( RAWINPUT *data, UINT *data_size, UINT header_size ) { - unsigned int count = 0, remaining, rawinput_size, next_size, overhead; + unsigned int count = 0, remaining, rawinput_size, next_size; struct rawinput_thread_data *thread_data; struct hardware_msg_data *msg_data; RAWINPUT *rawinput; int i; + BOOL is_wow = NtCurrentTeb()->WowTebOffset != 0; - if (NtCurrentTeb()->WowTebOffset) + if (is_wow) rawinput_size = sizeof(RAWINPUT64); else rawinput_size = sizeof(RAWINPUT); - overhead = rawinput_size - sizeof(RAWINPUT); if (header_size != sizeof(RAWINPUTHEADER)) { @@ -668,13 +724,7 @@ UINT WINAPI NtUserGetRawInputBuffer( RAWINPUT *data, UINT *data_size, UINT heade for (i = 0; i < count; ++i) { data->header.dwSize = remaining; - if (!rawinput_from_hardware_message( data, msg_data )) break; - if (overhead) - { - memmove( (char *)&data->data + overhead, &data->data, - data->header.dwSize - sizeof(RAWINPUTHEADER) ); - } - data->header.dwSize += overhead; + if (!rawinput_from_hardware_message( data, msg_data, is_wow )) break; remaining -= data->header.dwSize; data = NEXTRAWINPUTBLOCK(data); msg_data = (struct hardware_msg_data *)((char *)msg_data + msg_data->size); @@ -776,7 +826,7 @@ BOOL process_rawinput_message( MSG *msg, UINT hw_id, const struct hardware_msg_d else { thread_data->buffer->header.dwSize = RAWINPUT_BUFFER_SIZE; - if (!rawinput_from_hardware_message( thread_data->buffer, msg_data )) return FALSE; + if (!rawinput_from_hardware_message( thread_data->buffer, msg_data, FALSE )) return FALSE; thread_data->hw_id = hw_id; msg->lParam = (LPARAM)hw_id; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1949
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=128355 Your paranoid android. === w7u_el (32 bit report) === user32: input.c:4583: Test failed: SendInput triggered unexpected message 0xc042
Are all these changes necessary? Why not simply fixup the existing fixup (which imho keeps this wow64 oddity localized and avoids having specific handling elsewhere where it's not needed) like this: ```diff diff --git a/dlls/win32u/rawinput.c b/dlls/win32u/rawinput.c index ab1252ccea6..f0be80924af 100644 --- a/dlls/win32u/rawinput.c +++ b/dlls/win32u/rawinput.c @@ -48,7 +48,11 @@ typedef struct { DWORD dwType; DWORD dwSize; - ULONGLONG hDevice; + union + { + HANDLE hDevice; + ULONGLONG __align; + }; ULONGLONG wParam; } RAWINPUTHEADER64; @@ -671,10 +675,14 @@ UINT WINAPI NtUserGetRawInputBuffer( RAWINPUT *data, UINT *data_size, UINT heade if (!rawinput_from_hardware_message( data, msg_data )) break; if (overhead) { + /* Under WoW64, GetRawInputBuffer always gives 64-bit RAWINPUT structs. */ + RAWINPUT64 *ri64 = (RAWINPUT64 *)data; memmove( (char *)&data->data + overhead, &data->data, data->header.dwSize - sizeof(RAWINPUTHEADER) ); + ri64->header.dwSize += overhead; + ri64->header.hDevice = data->header.hDevice; + ri64->header.wParam = data->header.wParam; } - data->header.dwSize += overhead; remaining -= data->header.dwSize; data = NEXTRAWINPUTBLOCK(data); msg_data = (struct hardware_msg_data *)((char *)msg_data + msg_data->size); ``` With a simple fix like that I think it'd even be acceptable to merge in 8.0. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1949#note_20838
On Fri Jan 13 07:11:10 2023 +0000, Rémi Bernon wrote:
Are all these changes necessary? Why not simply fixup the existing fixup (which imho keeps this wow64 oddity localized and avoids having specific handling elsewhere where it's not needed) like this: ```diff diff --git a/dlls/win32u/rawinput.c b/dlls/win32u/rawinput.c index ab1252ccea6..f0be80924af 100644 --- a/dlls/win32u/rawinput.c +++ b/dlls/win32u/rawinput.c @@ -48,7 +48,11 @@ typedef struct { DWORD dwType; DWORD dwSize; - ULONGLONG hDevice; + union + { + HANDLE hDevice; + ULONGLONG __align; + }; ULONGLONG wParam; } RAWINPUTHEADER64;
@@ -671,10 +675,14 @@ UINT WINAPI NtUserGetRawInputBuffer( RAWINPUT *data, UINT *data_size, UINT heade if (!rawinput_from_hardware_message( data, msg_data )) break; if (overhead) { + /* Under WoW64, GetRawInputBuffer always gives 64-bit RAWINPUT structs. */ + RAWINPUT64 *ri64 = (RAWINPUT64 *)data; memmove( (char *)&data->data + overhead, &data->data, data->header.dwSize - sizeof(RAWINPUTHEADER) ); + ri64->header.dwSize += overhead; + ri64->header.hDevice = data->header.hDevice; + ri64->header.wParam = data->header.wParam; } - data->header.dwSize += overhead; remaining -= data->header.dwSize; data = NEXTRAWINPUTBLOCK(data); msg_data = (struct hardware_msg_data *)((char *)msg_data + msg_data->size); ``` With a simple fix like that I think it'd even be acceptable to merge in 8.0. (Well, I wanted to make it simpler, but I realize that __align high bits are left untouched here, probably better to keep the #ifdef and HandleToUlong)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1949#note_20839
participants (4)
-
Marvin -
Rémi Bernon -
Tim Clem
-
Tim Clem (＠tclem)