[PATCH v2 0/1] MR2067: wine32u: fix off-by-one bugs in MsgWaitForMultipleObjectsEx drivers

[Ivan Chikish (＠nekotekina)]

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

--
  v2: wine32u: move count fixup out of MsgWaitForMultipleObjectsEx drivers
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2067

[PATCH v2 1/1] wine32u: move count fixup out of MsgWaitForMultipleObjectsEx drivers

[Ivan Chikish]

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. When -1 is
returned, it may cause SetLastError to -1 as well which is not a
valid error code AFAIK. Also PeekMessage, which calls this driver
indirectly, may be not supposed to set last error at all according
to MSDN.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54405
---
 dlls/win32u/message.c         | 9 ++++++---
 dlls/wineandroid.drv/window.c | 2 +-
 dlls/winemac.drv/event.c      | 2 +-
 dlls/winex11.drv/event.c      | 2 +-
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/dlls/win32u/message.c b/dlls/win32u/message.c
index 2c320377e34..fe4cd909d89 100644
--- a/dlls/win32u/message.c
+++ b/dlls/win32u/message.c
@@ -2187,7 +2187,7 @@ DWORD WINAPI NtUserMsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handl
                                                 DWORD timeout, DWORD mask, DWORD flags )
 {
     HANDLE wait_handles[MAXIMUM_WAIT_OBJECTS];
-    DWORD i;
+    DWORD i, ret;
 
     if (count > MAXIMUM_WAIT_OBJECTS-1)
     {
@@ -2199,8 +2199,11 @@ DWORD WINAPI NtUserMsgWaitForMultipleObjectsEx( DWORD count, const HANDLE *handl
     for (i = 0; i < count; i++) wait_handles[i] = normalize_std_handle( handles[i] );
     wait_handles[count] = get_server_queue_handle();
 
-    return wait_objects( count+1, wait_handles, timeout,
-                         (flags & MWMO_INPUTAVAILABLE) ? mask : 0, mask, flags );
+    ret = wait_objects( count+1, wait_handles, timeout,
+                        (flags & MWMO_INPUTAVAILABLE) ? mask : 0, mask, flags );
+    if (ret == count + 1)
+        ret = count;
+    return ret;
 }
 
 /***********************************************************************
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

[Alexandros Frantzis (＠afrantzis)]

> 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.

OK, so the actual problem is the new SetLastError behavior, and I think this is a reasonable concern. About how a proper fix should look like, I will have to defer to someone with more experience in the win32u internals design.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_22345

[Jinoh Kang (＠iamahuman)]

> Is PeekMessage supposed to SetLastError at all?

It already does in some cases.  You use `RtlSetLastWin32Error` for that.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_22487

[PATCH v2 0/1] MR2067: wine32u: fix integer underflow in MsgWaitForMultipleObjectsEx drivers - closed

[Ivan Chikish (＠nekotekina)]

This merge request was closed by Ivan Chikish.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2067

[PATCH v2 0/1] MR2067: wine32u: fix integer underflow in MsgWaitForMultipleObjectsEx drivers

[Ivan Chikish (＠nekotekina)]

Superseded by !2242

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/2067#note_25300