[PATCH 0/1] MR2067: wine32u: fix off-by-one bugs in MsgWaitForMultipleObjectsEx drivers
When count = 0, returned value may become -1 which seems wrong in the case of successful process_events() call. Also MSDN suggests that `count` may be a valid return value in this case. When -1 is returned, it may cause SetLastError to -1 as well which is not a valid error code AFAIK. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54405 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2067
From: Ivan Chikish <nekotekina(a)gmail.com> When count = 0, returned value may become -1 which seems wrong in the case of successful process_events() call. Also MSDN suggests that `count` may be a valid return value in this case. When -1 is returned, it may cause SetLastError to -1 as well which is not a valid error code AFAIK. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54405 --- dlls/wineandroid.drv/window.c | 2 +- dlls/winemac.drv/event.c | 2 +- dlls/winex11.drv/event.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/wineandroid.drv/window.c b/dlls/wineandroid.drv/window.c index a339c20ceda..04cc88653cc 100644 --- a/dlls/wineandroid.drv/window.c +++ b/dlls/wineandroid.drv/window.c @@ -1210,7 +1210,7 @@ NTSTATUS ANDROID_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles { /* don't process nested events */ if (current_event) mask = 0; - if (process_events( mask )) return count - 1; + if (process_events( mask )) return count; } return NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), !!(flags & MWMO_ALERTABLE), timeout ); diff --git a/dlls/winemac.drv/event.c b/dlls/winemac.drv/event.c index e4f76e2b0e1..34e4e1e9700 100644 --- a/dlls/winemac.drv/event.c +++ b/dlls/winemac.drv/event.c @@ -535,7 +535,7 @@ NTSTATUS macdrv_MsgWaitForMultipleObjectsEx(DWORD count, const HANDLE *handles, data->current_event->type != WINDOW_DRAG_BEGIN) event_mask = 0; /* don't process nested events */ - if (process_events(data->queue, event_mask)) ret = count - 1; + if (process_events(data->queue, event_mask)) ret = count; else if (count || !timeout || timeout->QuadPart) { ret = NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 86edf66b820..b519d1fef17 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -489,7 +489,7 @@ NTSTATUS X11DRV_MsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, if (data->current_event) mask = 0; /* don't process nested events */ - if (process_events( data->display, filter_event, mask )) ret = count - 1; + if (process_events( data->display, filter_event, mask )) ret = count; else if (count || !timeout || timeout->QuadPart) { ret = NtWaitForMultipleObjects( count, handles, !(flags & MWMO_WAITALL), -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2067
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 full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=128903 Your paranoid android. === debian11 (build log) === WineRunWineTest.pl:error: The task timed out
Hi Ivan! The MsgWaitForMultipleObjectsEx **driver** function is expected to be called in two ways: 1. data == NULL, count == 0 2. data != NULL, count >= 1, with the handle at index `count - 1` being the server queue handle. In the second case the server queue handle is appended internally by Wine and the count adjusted (see `win32u/message.c`). So, the `count - 1` value returned by the driver function ends up corresponding to `count` in the original user invocation (e.g., from (NtUser)MsgWaitForMultipleObjectsEx): ``` NtUserMsgWaitForMultipleObjectsEx(count = 2, pHandles = [h1, h2], ...) driver->pMsgWaitForMultipleObjectsEx(count' = 3, pHandles' = [h1, h2, server_queue_handle], ...) => 2, which is == count' - 1 => 2, which is == count (`WAIT_OBJECT_0 + nCount` as per the MSDN docs) ``` All of this to explain why the proposed change is not correct, since with this change (NtUser)MsgWaitForMultipleObjectsEx would end up returning the wrong value in this scenario (`WAIT_OBJECT_0 + nCount + 1` which is not, AFAIK, a meaningful value). Driver implementations (exception: Android, see below) have a separate code path for the `data == NULL` case which doesn't involve `process_events()`, and since if `data != NULL` all the Wine code paths give us `count >= 1`, I don't see how we can get into the situation you are trying to fix. Perhaps I am missing something, but are you sure this particular scenario is the reason for the failures you are seeing? FWIW, the Android driver doesn't seem to handle the `data == NULL, count == 0` scenario properly (i.e., it still calls ` if (process_events( mask )) return count - 1;`) but perhaps there is something else at play here which I am not familiar with. In any case, the Android driver is not relevant to your scenario. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_22319
On Mon Jan 30 17:16:06 2023 +0000, Alexandros Frantzis wrote:
Hi Ivan! The MsgWaitForMultipleObjectsEx **driver** function is expected to be called in two ways: 1. data == NULL, count == 0 2. data != NULL, count >= 1, with the handle at index `count - 1` being the server queue handle. In the second case the server queue handle is appended internally by Wine and the count adjusted (see `win32u/message.c`). So, the `count - 1` value returned by the driver function ends up corresponding to `count` in the original user invocation (e.g., from (NtUser)MsgWaitForMultipleObjectsEx): ``` NtUserMsgWaitForMultipleObjectsEx(count = 2, pHandles = [h1, h2], ...) driver->pMsgWaitForMultipleObjectsEx(count' = 3, pHandles' = [h1, h2, server_queue_handle], ...) => 2, which is == count' - 1 => 2, which is == count (`WAIT_OBJECT_0 + nCount` as per the MSDN docs) ``` All of this to explain why the proposed change is not correct, since with this change (NtUser)MsgWaitForMultipleObjectsEx would end up returning the wrong value in this scenario (`WAIT_OBJECT_0 + nCount + 1` which is not, AFAIK, a meaningful value). Driver implementations (exception: Android, see below) have a separate code path for the `data == NULL` case which doesn't involve `process_events()`, and since if `data != NULL` all the Wine code paths give us `count >= 1`, I don't see how we can get into the situation you are trying to fix. Perhaps I am missing something, but are you sure this particular scenario is the reason for the failures you are seeing? FWIW, the Android driver doesn't seem to handle the `data == NULL, count == 0` scenario properly (i.e., it still calls ` if (process_events( mask )) return count - 1;`) but perhaps there is something else at play here which I am not familiar with. In any case, the Android driver is not relevant to your scenario. Hello, I experienced occasional hits of the code path after `process_events()` with `count == 0` in the game I was trying to fix ( https://bugs.winehq.org/show_bug.cgi?id=54405 ). It resulted in `-1` from the driver function, without a mistake: I inserted a special check for it. What if I add a special case in `NtUserMsgWaitForMultipleObjectsEx` to clamp `nCount + 1` to `nCount` results instead of doing this in the driver functions?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_22320
On Mon Jan 30 17:16:06 2023 +0000, Ivan Chikish wrote:
Hello, I experienced occasional hits of the code path after `process_events()` with `count == 0` in the game I was trying to fix ( https://bugs.winehq.org/show_bug.cgi?id=54405 ). It resulted in `-1` from the driver function, without a mistake: I inserted a special check for it. What if I add a special case in `NtUserMsgWaitForMultipleObjectsEx` to clamp `nCount + 1` to `nCount` results instead of doing this in the driver functions? UPD: callstack of the "failure" I'm experiencing:
=>0 0x0000007e13d0e7 X11DRV_MsgWaitForMultipleObjectsEx+0xb7(count=0, handles=00
00000000000000, timeout=000000000069FBB8, mask=0x1cff, flags=0)
1 0x0000007e7ac1f0 wait_message+0x60(count=0, handles=0000000000000000, timeou
t=0, mask=0x1cff, flags=0)
2 0x0000007e7b14b4 NtUserPeekMessage+0xb4(msg_out=<couldn't compute location>,
hwnd=<couldn't compute location>, first=<couldn't compute location>, last=<coul
dn't compute location>, flags=<couldn't compute location>)
3 0x000000f7c55469 in ntdll.so (+0x37469) (0x0000000069fcb4)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_22321
On Mon Jan 30 17:32:02 2023 +0000, Ivan Chikish wrote:
UPD: callstack of the "failure" I'm experiencing: ``` =>0 0x0000007e13d0e7 X11DRV_MsgWaitForMultipleObjectsEx+0xb7(count=0, handles=00 00000000000000, timeout=000000000069FBB8, mask=0x1cff, flags=0) 1 0x0000007e7ac1f0 wait_message+0x60(count=0, handles=0000000000000000, timeou t=0, mask=0x1cff, flags=0) 2 0x0000007e7b14b4 NtUserPeekMessage+0xb4(msg_out=<couldn't compute location>, hwnd=<couldn't compute location>, first=<couldn't compute location>, last=<coul dn't compute location>, flags=<couldn't compute location>) 3 0x000000f7c55469 in ntdll.so (+0x37469) (0x0000000069fcb4) ``` @nekotekina Thanks, I understand the issue better now (in my last paragraphs of my original message I mixed up `data` with `handles` when thinking about code paths leading to this, so please ignore those).
However, I now wonder if what you describe is a problem at all. It seems to me that everything is working as expected, and the 0xffffffff GetLastError you see is in fact not relevant: ``` NtUserPeekMessage wait_message count=0 handles=NULL X11DRV_MsgWaitForMultipleObjectsEx count=0 handles=NULL => -1 (0xffffffff) => WAIT_FAILED (0xffffffff) => FALSE, since wait_message() returned != WAIT_TIMEOUT (indicating that we received driver events) the code calls peek_message() again, finds nothing and returns FALSE, due to: ret = wait_message( 0, NULL, 0, QS_ALLINPUT, 0 ); /* if we received driver events, check again for a pending message */ if (ret == WAIT_TIMEOUT || peek_message( &msg, hwnd, first, last, flags, 0 ) <= 0) return FALSE; ``` So, this sequence seems that's just part of normal operation, and a red herring for your investigation? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_22322
On Mon Jan 30 18:27:02 2023 +0000, Alexandros Frantzis wrote:
@nekotekina Thanks, I understand the issue better now (in my last paragraphs of my original message I mixed up `data` with `handles` when thinking about code paths leading to this, so please ignore those). However, I now wonder if what you describe is a problem at all. It seems to me that everything is working as expected, and the 0xffffffff GetLastError you see is in fact not relevant: ``` NtUserPeekMessage wait_message count=0 handles=NULL X11DRV_MsgWaitForMultipleObjectsEx count=0 handles=NULL => -1 (0xffffffff) => WAIT_FAILED (0xffffffff) => FALSE, since wait_message() returned != WAIT_TIMEOUT (indicating that we received driver events) the code calls peek_message() again, finds nothing and returns FALSE, due to: ret = wait_message( 0, NULL, 0, QS_ALLINPUT, 0 ); /* if we received driver events, check again for a pending message */ if (ret == WAIT_TIMEOUT || peek_message( &msg, hwnd, first, last, flags, 0 ) <= 0) return FALSE; ``` So, this sequence seems that's just part of normal operation, and a red herring for your investigation? Is PeekMessage supposed to SetLastError at all? Because that's what happens in the game, and the value 0xffffffff is read much later in an unrelated code, causing the crash. I think the regression was introduced relatively recently by commit ac9b63580018354c5a7c9164bb5c781b9e2b6cbf. MSDN doesn't seem to say anything about GetLastError in PeekMessageW.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_22327
participants (4)
-
Alexandros Frantzis (＠afrantzis) -
Ivan Chikish
-
Ivan Chikish (＠nekotekina)
-
Marvin