[PATCH 0/3] MR9305: Reject pseudo-handles in NtWaitForMultipleObjects().
Some applications rely on this behavior. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernelbase/sync.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/dlls/kernelbase/sync.c b/dlls/kernelbase/sync.c index 40aec41e5c8..7ebd711e468 100644 --- a/dlls/kernelbase/sync.c +++ b/dlls/kernelbase/sync.c @@ -395,7 +395,7 @@ BOOL WINAPI DECLSPEC_HOTPATCH UnregisterWaitEx( HANDLE handle, HANDLE event ) */ DWORD WINAPI DECLSPEC_HOTPATCH WaitForSingleObject( HANDLE handle, DWORD timeout ) { - return WaitForMultipleObjectsEx( 1, &handle, FALSE, timeout, FALSE ); + return WaitForSingleObjectEx( handle, timeout, FALSE ); } @@ -404,7 +404,17 @@ DWORD WINAPI DECLSPEC_HOTPATCH WaitForSingleObject( HANDLE handle, DWORD timeout */ DWORD WINAPI DECLSPEC_HOTPATCH WaitForSingleObjectEx( HANDLE handle, DWORD timeout, BOOL alertable ) { - return WaitForMultipleObjectsEx( 1, &handle, FALSE, timeout, alertable ); + NTSTATUS status; + LARGE_INTEGER time; + + status = NtWaitForSingleObject( normalize_std_handle( handle ), alertable, + get_nt_timeout( &time, timeout ) ); + if (HIWORD(status)) /* is it an error code? */ + { + SetLastError( RtlNtStatusToDosError(status) ); + status = WAIT_FAILED; + } + return status; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/ntdll/unix/sync.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 07ffe854104..6557bc3bf28 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -2333,7 +2333,22 @@ NTSTATUS WINAPI NtWaitForMultipleObjects( DWORD count, const HANDLE *handles, WA */ NTSTATUS WINAPI NtWaitForSingleObject( HANDLE handle, BOOLEAN alertable, const LARGE_INTEGER *timeout ) { - return NtWaitForMultipleObjects( 1, &handle, FALSE, alertable, timeout ); + union select_op select_op; + UINT flags = SELECT_INTERRUPTIBLE; + unsigned int ret; + + TRACE( "handle %p, alertable %u, timeout %s\n", handle, alertable, debugstr_timeout(timeout) ); + + if ((ret = inproc_wait( 1, &handle, WaitAny, alertable, timeout )) != STATUS_NOT_IMPLEMENTED) + goto out; + + if (alertable) flags |= SELECT_ALERTABLE; + select_op.wait.op = SELECT_WAIT; + select_op.wait.handles[0] = wine_server_obj_handle( handle ); + ret = server_wait( &select_op, offsetof( union select_op, wait.handles[1] ), flags, timeout ); +out: + TRACE( "-> %#x\n", ret ); + return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernel32/tests/sync.c | 12 ++++++------ dlls/ntdll/unix/sync.c | 14 +++++++++++--- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c index 466bbc8908b..f2fc0196626 100644 --- a/dlls/kernel32/tests/sync.c +++ b/dlls/kernel32/tests/sync.c @@ -1432,26 +1432,26 @@ static void test_WaitForMultipleObjects(void) maxevents[0] = GetCurrentProcess(); SetLastError(0xdeadbeef); r = WaitForMultipleObjects(1, maxevents, FALSE, 100); - todo_wine ok(r == WAIT_FAILED, "expected WAIT_FAILED, got %lu\n", r); - todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, + ok(r == WAIT_FAILED, "expected WAIT_FAILED, got %lu\n", r); + ok(GetLastError() == ERROR_INVALID_HANDLE, "expected ERROR_INVALID_HANDLE, got %lu\n", GetLastError()); maxevents[0] = GetCurrentThread(); SetLastError(0xdeadbeef); r = WaitForMultipleObjects(1, maxevents, FALSE, 100); - todo_wine ok(r == WAIT_FAILED, "expected WAIT_FAILED, got %lu\n", r); - todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, + ok(r == WAIT_FAILED, "expected WAIT_FAILED, got %lu\n", r); + ok(GetLastError() == ERROR_INVALID_HANDLE, "expected ERROR_INVALID_HANDLE, got %lu\n", GetLastError()); timeout.QuadPart = -1000000; maxevents[0] = GetCurrentProcess(); status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); - todo_wine ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); timeout.QuadPart = -1000000; maxevents[0] = GetCurrentThread(); status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); - todo_wine ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); } static BOOL g_initcallback_ret, g_initcallback_called; diff --git a/dlls/ntdll/unix/sync.c b/dlls/ntdll/unix/sync.c index 6557bc3bf28..6565216f144 100644 --- a/dlls/ntdll/unix/sync.c +++ b/dlls/ntdll/unix/sync.c @@ -2313,16 +2313,24 @@ NTSTATUS WINAPI NtWaitForMultipleObjects( DWORD count, const HANDLE *handles, WA TRACE( "}, timeout %s\n", debugstr_timeout(timeout) ); } - if ((ret = inproc_wait( count, handles, type, alertable, timeout )) != STATUS_NOT_IMPLEMENTED) + /* Reject pseudo-handles up front. These are not valid for multi-object waits. */ + for (i = 0; i < count; i++) { - TRACE( "-> %#x\n", ret ); - return ret; + if ((ULONG)(ULONG_PTR)handles[i] > 0xfffffffa) + { + ret = STATUS_INVALID_HANDLE; + goto out; + } } + if ((ret = inproc_wait( count, handles, type, alertable, timeout )) != STATUS_NOT_IMPLEMENTED) + goto out; + if (alertable) flags |= SELECT_ALERTABLE; select_op.wait.op = type == WaitAll ? SELECT_WAIT_ALL : SELECT_WAIT; for (i = 0; i < count; i++) select_op.wait.handles[i] = wine_server_obj_handle( handles[i] ); ret = server_wait( &select_op, offsetof( union select_op, wait.handles[count] ), flags, timeout ); +out: TRACE( "-> %#x\n", ret ); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
Dmitry Timoshkov (@dmitry) commented about dlls/kernelbase/sync.c:
*/ DWORD WINAPI DECLSPEC_HOTPATCH WaitForSingleObjectEx( HANDLE handle, DWORD timeout, BOOL alertable ) { - return WaitForMultipleObjectsEx( 1, &handle, FALSE, timeout, alertable ); + NTSTATUS status; + LARGE_INTEGER time; + + status = NtWaitForSingleObject( normalize_std_handle( handle ), alertable, + get_nt_timeout( &time, timeout ) ); + if (HIWORD(status)) /* is it an error code? */ Shouldn't the check use something like NT_SUCCESS()/NT_ERROR() macros instead of HIWORD()?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_119960
On Wed Oct 29 11:00:11 2025 +0000, Dmitry Timoshkov wrote:
Shouldn't the check use something like NT_SUCCESS()/NT_ERROR() macros instead of HIWORD()? This was adapted to be similar to the existing implementation in `WaitForMultipleObjectsEx`, but I am open to changing it if this is preferred in general.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_119962
On Wed Oct 29 11:02:51 2025 +0000, Marc-Aurel Zent wrote:
This was adapted to be similar to the existing implementation in `WaitForMultipleObjectsEx`, but I am open to changing it if this is preferred in general. I see, thanks for the clarification.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_119979
Paul Gofman (@gofman) commented about dlls/kernelbase/sync.c:
*/ DWORD WINAPI DECLSPEC_HOTPATCH WaitForSingleObjectEx( HANDLE handle, DWORD timeout, BOOL alertable ) { - return WaitForMultipleObjectsEx( 1, &handle, FALSE, timeout, alertable ); + NTSTATUS status; + LARGE_INTEGER time; + + status = NtWaitForSingleObject( normalize_std_handle( handle ), alertable, It would probably need a test for waiting on stdio handles to see if introducing normalize_std_handle is needed here, for either wait for single object or both.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120014
Paul Gofman (@gofman) commented about dlls/ntdll/unix/sync.c:
TRACE( "}, timeout %s\n", debugstr_timeout(timeout) ); }
- if ((ret = inproc_wait( count, handles, type, alertable, timeout )) != STATUS_NOT_IMPLEMENTED) + /* Reject pseudo-handles up front. These are not valid for multi-object waits. */ + for (i = 0; i < count; i++) { - TRACE( "-> %#x\n", ret ); - return ret; + if ((ULONG)(ULONG_PTR)handles[i] > 0xfffffffa)
This would need tests for other special handles besides the current process. Also, do we have any tests showing that the behaviour is different for the single and multiple objects wait, wait for all and for any? Maybe I missed something but I only spotted the patches fixing todos in the test for multiple objects. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120016
Paul Gofman (@gofman) commented about dlls/kernelbase/sync.c:
*/ DWORD WINAPI DECLSPEC_HOTPATCH WaitForSingleObjectEx( HANDLE handle, DWORD timeout, BOOL alertable ) { - return WaitForMultipleObjectsEx( 1, &handle, FALSE, timeout, alertable ); + NTSTATUS status; + LARGE_INTEGER time; + + status = NtWaitForSingleObject( normalize_std_handle( handle ), alertable, + get_nt_timeout( &time, timeout ) ); + if (HIWORD(status)) /* is it an error code? */ + { + SetLastError( RtlNtStatusToDosError(status) ); There is set_ntstatus() for this, does it not work here for some reason? But I think it can use more tests besides the case for GetCurrentProcess() handle.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120015
Before reasoning on gory details, it seems to me we could use more test clearly showing: - the difference between wait single and multiple (multiple as in multiple wait function but with one handle only, with multiple handles, what all and wait any: if there is a difference between waiting for single and multiple, what if handle count or wait all / wait any also matters?); - what happens with std handles (especially because the MR changes behaviour for those); - what happens with other special handles like current thread or token. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120017
Also WRT setting last error I think it could also use some tests for other cases besides invalid handle due to special handle. Like, invalid handle which is a valid handle but not waitable, when there is STATUS_ACCESS_DENIED for wait, when there is timeout (maybe we already have some for the latter, but probably not for other error statuses as those would have been fixed todos). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120020
Also, do we have any tests showing that the behaviour is different for the single and multiple objects wait, wait for all and for any?
For single waits we do: https://gitlab.winehq.org/wine/wine/-/commit/46a208d6faf5668e88a98fc3a14733b... , but this does not test wait any vs wait all, but IIRC this does not matter (would still be good to have tested out though).
This would need tests for other special handles besides the current process.
All other pseudo handles apart from `NtCurrentProcess()` and `NtCurrentThread()` are non-waitable and also return `STATUS_INVALID_HANDLE` on any possible path that I tested, but I can also add tests for that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120040
On Wed Oct 29 17:30:20 2025 +0000, Paul Gofman wrote:
There is set_ntstatus() for this, does it not work here for some reason? But I think it can use more tests besides the case for GetCurrentProcess() handle. We can't use `set_ntstatus()` since we are returning non-truthy values, which are not errors, but just need to be passed through, but as @dmitry noted `NT_ERROR()` could be used here and is probably more elegant.
I think the `WaitForMultipleObjectsEx` implementation should stay in sync with whatever is done though. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120042
On Wed Oct 29 21:39:08 2025 +0000, Paul Gofman wrote:
Before reasoning on gory details, it seems to me we could use more test clearly showing: - the difference between wait single and multiple (multiple as in multiple wait function but with one handle only, with multiple handles, what all and wait any: if there is a difference between waiting for single and multiple, what if handle count or wait all / wait any also matters?); - what happens with std handles (especially because the MR changes behaviour for those); - what happens with other special handles like current thread or token. I'll try to add tests for all of these.
multiple as in multiple wait function but with one handle only, with multiple handles,
multiple as in takes the `NtWaitForMultipleObjects()` path irrespective of number of handles. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120043
On Wed Oct 29 21:47:26 2025 +0000, Paul Gofman wrote:
Also WRT setting last error I think it could also use some tests for other cases besides invalid handle due to special handle. Like, invalid handle which is a valid handle but not waitable, when there is STATUS_ACCESS_DENIED for wait, when there is timeout (maybe we already have some for the latter, but probably not for other error statuses as those would have been fixed todos). IIRC `STATUS_ACCESS_DENIED` is already correctly handled (both with inproc and server-side waits) and depends on if the opened handle has `SYNCHRONIZE` access.
I believe `STATUS_INVALID_HANDLE` is also returned for valid handles that are not waitable, but this could indeed use a bit more testing too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120044
what happens with std handles (especially because the MR changes behaviour for those);
I am not sure I can see how it is changing behaviour here for std handles... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120045
On Wed Oct 29 21:52:54 2025 +0000, Marc-Aurel Zent wrote:
what happens with std handles (especially because the MR changes behaviour for those); I am not sure I can see how it is changing behaviour here for std handles... Well, in the last patch:
- return WaitForMultipleObjectsEx( 1, &handle, FALSE, timeout, alertable );
+ NTSTATUS status;
+ LARGE_INTEGER time;
+
+ status = NtWaitForSingleObject( normalize_std_handle( handle ), alertable,
+ get_nt_timeout( &time, timeout ) );
Previously there wasn't normalize_std_handle call, now there is (which replaces special values like STD_INPUT_HANDLE with the handles fetched from TEB), so looks it changes the behaviour? Am I missing something? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120046
On Wed Oct 29 22:06:11 2025 +0000, Paul Gofman wrote:
Well, in the last patch: ``` - return WaitForMultipleObjectsEx( 1, &handle, FALSE, timeout, alertable ); + NTSTATUS status; + LARGE_INTEGER time; + + status = NtWaitForSingleObject( normalize_std_handle( handle ), alertable, + get_nt_timeout( &time, timeout ) ); ``` Previously there wasn't normalize_std_handle call, now there is (which replaces special values like STD_INPUT_HANDLE with the handles fetched from TEB), so looks it changes the behaviour? Am I missing something? `WaitForMultipleObjectsEx()` would normalize all std handles then previously.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120047
On Wed Oct 29 21:47:26 2025 +0000, Marc-Aurel Zent wrote:
IIRC `STATUS_ACCESS_DENIED` is already correctly handled (both with inproc and server-side waits) and depends on if the opened handle has `SYNCHRONIZE` access. I believe `STATUS_INVALID_HANDLE` is also returned for valid handles that are not waitable, but this could indeed use a bit more testing too. I know some tests might be spread over here and there, but I personally would suggest to see that single vs multiple behaviour tested thorougly in one place, with many handles to one multiple wait, single, all / any etc. Because the difference between wait multiple and wait single call looks so weird.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120048
On Wed Oct 29 22:15:32 2025 +0000, Marc-Aurel Zent wrote:
`WaitForMultipleObjectsEx()` would normalize all std handles then previously. Ah yeah, sorry, indeed. Yet once we are going to do weird things with handles it would probably make sense to test something around std handles. If WaitForMultiple and WaitForSingle behaves different WRT pseudo-handles, is it the same for std handles?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120054
participants (4)
-
Dmitry Timoshkov (＠dmitry) -
Marc-Aurel Zent
-
Marc-Aurel Zent (＠mzent)
-
Paul Gofman (＠gofman)