[PATCH v3 0/6] MR9305: Reject pseudo-handles in NtWaitForMultipleObjects().
Some applications rely on this behavior. -- v3: kernel32/tests: Add more tests for waits on pseudo-handles. include: Add more NT pseudo-handle definitions. ntdll: Reject pseudo-handles in NtWaitForMultipleObjects. ntdll: Reimplement NtWaitForSingleObject without NtWaitForMultipleObjects. kernelbase: Reimplement WaitForSingleObject[Ex] on top of NtWaitForSingleObject. kernelbase: Add set_ntwaitstatus() helper. https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernelbase/kernelbase.h | 10 ++++++++++ dlls/kernelbase/sync.c | 11 ++--------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/dlls/kernelbase/kernelbase.h b/dlls/kernelbase/kernelbase.h index 2209939a8a2..dad8b3e7d2c 100644 --- a/dlls/kernelbase/kernelbase.h +++ b/dlls/kernelbase/kernelbase.h @@ -50,6 +50,16 @@ static inline BOOL set_ntstatus( NTSTATUS status ) return !status; } +static inline DWORD set_ntwaitstatus( NTSTATUS status ) +{ + if (NT_ERROR(status)) + { + SetLastError( RtlNtStatusToDosError( status )); + return WAIT_FAILED; + } + return (DWORD)status; +} + /* make the kernel32 names available */ #define HeapAlloc(heap, flags, size) RtlAllocateHeap(heap, flags, size) #define HeapReAlloc(heap, flags, ptr, size) RtlReAllocateHeap(heap, flags, ptr, size) diff --git a/dlls/kernelbase/sync.c b/dlls/kernelbase/sync.c index 40aec41e5c8..09a880fe2c8 100644 --- a/dlls/kernelbase/sync.c +++ b/dlls/kernelbase/sync.c @@ -424,7 +424,6 @@ DWORD WINAPI DECLSPEC_HOTPATCH WaitForMultipleObjects( DWORD count, const HANDLE DWORD WINAPI DECLSPEC_HOTPATCH WaitForMultipleObjectsEx( DWORD count, const HANDLE *handles, BOOL wait_all, DWORD timeout, BOOL alertable ) { - NTSTATUS status; HANDLE hloc[MAXIMUM_WAIT_OBJECTS]; LARGE_INTEGER time; unsigned int i; @@ -436,14 +435,8 @@ DWORD WINAPI DECLSPEC_HOTPATCH WaitForMultipleObjectsEx( DWORD count, const HAND } for (i = 0; i < count; i++) hloc[i] = normalize_std_handle( handles[i] ); - status = NtWaitForMultipleObjects( count, hloc, wait_all ? WaitAll : WaitAny, alertable, - get_nt_timeout( &time, timeout ) ); - if (HIWORD(status)) /* is it an error code? */ - { - SetLastError( RtlNtStatusToDosError(status) ); - status = WAIT_FAILED; - } - return status; + return set_ntwaitstatus( NtWaitForMultipleObjects( count, hloc, wait_all ? WaitAll : WaitAny, alertable, + get_nt_timeout( &time, timeout ) ) ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernelbase/sync.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/dlls/kernelbase/sync.c b/dlls/kernelbase/sync.c index 09a880fe2c8..ecb68be77da 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,10 @@ 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 ); + LARGE_INTEGER time; + + return set_ntwaitstatus( NtWaitForSingleObject( normalize_std_handle( handle ), alertable, + get_nt_timeout( &time, timeout ) ) ); } -- 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..6cf0fa4369a 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
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- include/winbase.h | 36 +++++++++++++++++++++--------------- include/winternl.h | 8 ++++++-- 2 files changed, 27 insertions(+), 17 deletions(-) diff --git a/include/winbase.h b/include/winbase.h index c96f58a61b3..4ae800bfc46 100644 --- a/include/winbase.h +++ b/include/winbase.h @@ -2657,6 +2657,9 @@ extern WCHAR * CDECL wine_get_dos_file_name( const char * ) __WINE_DEALLOC(HeapF #define GetCurrentProcess() NtCurrentProcess() #define GetCurrentThread() NtCurrentThread() +#define GetCurrentProcessToken() NtCurrentProcessToken() +#define GetCurrentThreadToken() NtCurrentThreadToken() +#define GetCurrentThreadEffectiveToken() NtCurrentEffectiveToken() #define GetCurrentProcessId() HandleToULong(NtCurrentTeb()->ClientId.UniqueProcess) #define GetCurrentThreadId() HandleToULong(NtCurrentTeb()->ClientId.UniqueThread) @@ -2682,6 +2685,21 @@ static FORCEINLINE DWORD WINAPI GetCurrentThreadId(void) return HandleToULong( ((HANDLE *)NtCurrentTeb())[9] ); } +static FORCEINLINE HANDLE WINAPI GetCurrentProcessToken(void) +{ + return (HANDLE)~(ULONG_PTR)3; +} + +static FORCEINLINE HANDLE WINAPI GetCurrentThreadToken(void) +{ + return (HANDLE)~(ULONG_PTR)4; +} + +static FORCEINLINE HANDLE WINAPI GetCurrentThreadEffectiveToken(void) +{ + return (HANDLE)~(ULONG_PTR)5; +} + static FORCEINLINE DWORD WINAPI GetLastError(void) { return *(DWORD *)((void **)NtCurrentTeb() + 13); @@ -2703,27 +2721,15 @@ WINBASEAPI HANDLE WINAPI GetCurrentProcess(void); WINBASEAPI DWORD WINAPI GetCurrentProcessId(void); WINBASEAPI HANDLE WINAPI GetCurrentThread(void); WINBASEAPI DWORD WINAPI GetCurrentThreadId(void); +WINBASEAPI HANDLE WINAPI GetCurrentProcessToken(void); +WINBASEAPI HANDLE WINAPI GetCurrentThreadToken(void); +WINBASEAPI HANDLE WINAPI GetCurrentThreadEffectiveToken(void); WINBASEAPI DWORD WINAPI GetLastError(void); WINBASEAPI HANDLE WINAPI GetProcessHeap(void); WINBASEAPI VOID WINAPI SetLastError(DWORD); #endif /* __WINESRC__ */ -static FORCEINLINE HANDLE WINAPI GetCurrentProcessToken(void) -{ - return (HANDLE)~(ULONG_PTR)3; -} - -static FORCEINLINE HANDLE WINAPI GetCurrentThreadToken(void) -{ - return (HANDLE)~(ULONG_PTR)4; -} - -static FORCEINLINE HANDLE WINAPI GetCurrentThreadEffectiveToken(void) -{ - return (HANDLE)~(ULONG_PTR)5; -} - /* WinMain(entry point) must be declared in winbase.h. */ /* If this is not declared, we cannot compile many sources written with C++. */ int WINAPI WinMain(HINSTANCE,HINSTANCE,LPSTR,int); diff --git a/include/winternl.h b/include/winternl.h index d94b9911c74..7dea51d2f93 100644 --- a/include/winternl.h +++ b/include/winternl.h @@ -5284,8 +5284,12 @@ NTSYSAPI NTSTATUS WINAPI RtlLargeIntegerToChar(const ULONGLONG *,ULONG,ULONG,PC * Inline functions */ -#define NtCurrentProcess() ((HANDLE)~(ULONG_PTR)0) -#define NtCurrentThread() ((HANDLE)~(ULONG_PTR)1) +#define NtCurrentProcess() ((HANDLE)~(ULONG_PTR)0) +#define NtCurrentThread() ((HANDLE)~(ULONG_PTR)1) +#define NtCurrentSession() ((HANDLE)~(ULONG_PTR)2) +#define NtCurrentProcessToken() ((HANDLE)~(ULONG_PTR)3) +#define NtCurrentThreadToken() ((HANDLE)~(ULONG_PTR)4) +#define NtCurrentEffectiveToken() ((HANDLE)~(ULONG_PTR)5) #define RtlFillMemory(Destination,Length,Fill) memset((Destination),(Fill),(Length)) #define RtlMoveMemory(Destination,Source,Length) memmove((Destination),(Source),(Length)) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/kernel32/tests/sync.c | 167 +++++++++++++++++++++++++++++-------- 1 file changed, 133 insertions(+), 34 deletions(-) diff --git a/dlls/kernel32/tests/sync.c b/dlls/kernel32/tests/sync.c index f2fc0196626..8787d720010 100644 --- a/dlls/kernel32/tests/sync.c +++ b/dlls/kernel32/tests/sync.c @@ -1291,6 +1291,22 @@ static void test_WaitForSingleObject(void) LARGE_INTEGER timeout; NTSTATUS status; DWORD ret; + int i; + HANDLE waitable_pseudohandles[] = { + NtCurrentProcess(), + NtCurrentThread() + }; + HANDLE non_waitable_pseudohandles[] = { + NtCurrentSession(), + NtCurrentProcessToken(), + NtCurrentThreadToken(), + NtCurrentEffectiveToken() + }; + HANDLE std_handles[] = { + (HANDLE)STD_INPUT_HANDLE, + (HANDLE)STD_OUTPUT_HANDLE, + (HANDLE)STD_ERROR_HANDLE + }; signaled = CreateEventW(NULL, TRUE, TRUE, NULL); nonsignaled = CreateEventW(NULL, TRUE, FALSE, NULL); @@ -1359,20 +1375,43 @@ static void test_WaitForSingleObject(void) ok(ret == WAIT_OBJECT_0, "expected WAIT_OBJECT_0, got %ld\n", ret); ok(GetLastError() == 0xdeadbeef, "expected 0xdeadbeef, got %ld\n", GetLastError()); - /* pseudo handles are allowed in WaitForSingleObject and NtWaitForSingleObject */ - ret = WaitForSingleObject(GetCurrentProcess(), 100); - ok(ret == WAIT_TIMEOUT, "expected WAIT_TIMEOUT, got %lu\n", ret); + /* waitable pseudo-handles are allowed in WaitForSingleObject and NtWaitForSingleObject, + * but not in NtWaitForMultipleObjects, see test_WaitForMultipleObjects */ + for (i = 0; i < ARRAY_SIZE(waitable_pseudohandles); i++) + { + ret = WaitForSingleObject(waitable_pseudohandles[i], 100); + ok(ret == WAIT_TIMEOUT, "expected WAIT_TIMEOUT, got %lu\n", ret); - ret = WaitForSingleObject(GetCurrentThread(), 100); - ok(ret == WAIT_TIMEOUT, "expected WAIT_TIMEOUT, got %lu\n", ret); + timeout.QuadPart = -1000000; + status = pNtWaitForSingleObject(waitable_pseudohandles[i], FALSE, &timeout); + ok(status == STATUS_TIMEOUT, "expected STATUS_TIMEOUT, got %08lx\n", status); + } - timeout.QuadPart = -1000000; - status = pNtWaitForSingleObject(GetCurrentProcess(), FALSE, &timeout); - ok(status == STATUS_TIMEOUT, "expected STATUS_TIMEOUT, got %08lx\n", status); + /* non-waitable pseudo-handles return STATUS_INVALID_HANDLE */ + for (i = 0; i < ARRAY_SIZE(non_waitable_pseudohandles); i++) + { + SetLastError(0xdeadbeef); + ret = WaitForSingleObject(non_waitable_pseudohandles[i], 100); + ok(ret == WAIT_FAILED, "expected WAIT_FAILED, got %ld\n", ret); + ok(GetLastError() == ERROR_INVALID_HANDLE, "expected ERROR_INVALID_HANDLE, got %ld\n", GetLastError()); + + timeout.QuadPart = -1000000; + status = pNtWaitForSingleObject(non_waitable_pseudohandles[i], FALSE, &timeout); + todo_wine_if(non_waitable_pseudohandles[i] == NtCurrentProcessToken() || + non_waitable_pseudohandles[i] == NtCurrentEffectiveToken()) + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + } - timeout.QuadPart = -1000000; - status = pNtWaitForSingleObject(GetCurrentThread(), FALSE, &timeout); - ok(status == STATUS_TIMEOUT, "expected STATUS_TIMEOUT, got %08lx\n", status); + /* std handles are only allowed in WaitForSingleObject and not NtWaitForSingleObject */ + for (i = 0; i < ARRAY_SIZE(std_handles); i++) + { + ret = WaitForSingleObject(std_handles[i], 100); + ok(ret != WAIT_FAILED, "expected non-fail result, got %lu (GetLastError()=%lu)\n", ret, GetLastError()); + + timeout.QuadPart = -1000000; + status = pNtWaitForSingleObject(std_handles[i], FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + } CloseHandle(signaled); CloseHandle(nonsignaled); @@ -1385,6 +1424,19 @@ static void test_WaitForMultipleObjects(void) DWORD r; int i; HANDLE maxevents[MAXIMUM_WAIT_OBJECTS]; + HANDLE pseudohandles[] = { + NtCurrentProcess(), + NtCurrentThread(), + NtCurrentSession(), + NtCurrentProcessToken(), + NtCurrentThreadToken(), + NtCurrentEffectiveToken() + }; + HANDLE std_handles[] = { + (HANDLE)STD_INPUT_HANDLE, + (HANDLE)STD_OUTPUT_HANDLE, + (HANDLE)STD_ERROR_HANDLE + }; /* create the maximum number of events and make sure * we can wait on that many */ @@ -1424,34 +1476,81 @@ static void test_WaitForMultipleObjects(void) ok(status == STATUS_WAIT_0 + i, "should signal handle #%d first, got %08lx\n", i, status); } - for (i=0; i<MAXIMUM_WAIT_OBJECTS; i++) - if (maxevents[i]) CloseHandle(maxevents[i]); + CloseHandle(maxevents[0]); - /* in contrast to WaitForSingleObject, pseudo handles are not allowed in + /* in contrast to WaitForSingleObject, all pseudo-handles are not allowed in * WaitForMultipleObjects and NtWaitForMultipleObjects */ - maxevents[0] = GetCurrentProcess(); - SetLastError(0xdeadbeef); - r = WaitForMultipleObjects(1, maxevents, FALSE, 100); - ok(r == WAIT_FAILED, "expected WAIT_FAILED, got %lu\n", r); - ok(GetLastError() == ERROR_INVALID_HANDLE, - "expected ERROR_INVALID_HANDLE, got %lu\n", GetLastError()); + for (i = 0; i < ARRAY_SIZE(pseudohandles); i++) + { + maxevents[0] = pseudohandles[i]; + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(1, maxevents, FALSE, 100); + 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); - 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; + status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); - timeout.QuadPart = -1000000; - maxevents[0] = GetCurrentProcess(); - status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); - ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + /* also not allowed with a wait count greater than 1 with both WaitAny and WaitAll */ + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, FALSE, 100); + ok(r == WAIT_FAILED, "expected WAIT_FAILED, got %lu\n", r); + ok(GetLastError() == ERROR_INVALID_HANDLE, + "expected ERROR_INVALID_HANDLE, got %lu\n", GetLastError()); + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, TRUE, 100); + 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; + status = pNtWaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, WaitAny, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + timeout.QuadPart = -1000000; + status = pNtWaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, WaitAll, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + + /* irrespective of the index that contains the pseudo handle for both wait types */ + maxevents[0] = maxevents[MAXIMUM_WAIT_OBJECTS - 1]; + maxevents[MAXIMUM_WAIT_OBJECTS - 1] = pseudohandles[i]; + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, FALSE, 100); + ok(r == WAIT_FAILED, "expected WAIT_FAILED, got %lu\n", r); + ok(GetLastError() == ERROR_INVALID_HANDLE, + "expected ERROR_INVALID_HANDLE, got %lu\n", GetLastError()); + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, TRUE, 100); + 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; + status = pNtWaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, WaitAny, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + timeout.QuadPart = -1000000; + status = pNtWaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, WaitAll, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + maxevents[MAXIMUM_WAIT_OBJECTS - 1] = maxevents[0]; + } + + /* Similar to WaitForSingleObject std handles are only allowed in WaitForMultipleObjects and + * not NtWaitForMultipleObjects (for brevity we only test single count WaitAny here) */ + for (i = 0; i < ARRAY_SIZE(std_handles); i++) + { + maxevents[0] = std_handles[i]; + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(1, maxevents, FALSE, 100); + ok(r != WAIT_FAILED, "expected non-fail result, got %lu (GetLastError()=%lu)\n", r, GetLastError()); - timeout.QuadPart = -1000000; - maxevents[0] = GetCurrentThread(); - status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); - ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + timeout.QuadPart = -1000000; + status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + } + + for (i=1; i<MAXIMUM_WAIT_OBJECTS; i++) + if (maxevents[i]) CloseHandle(maxevents[i]); } static BOOL g_initcallback_ret, g_initcallback_called; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9305
On Thu Oct 30 15:40:37 2025 +0000, Marc-Aurel Zent wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/9305/diffs?diff_id=219832&start_sha=792178f0af4c418a0ab07d41ea0f423212548d88#99217b65edfcb4ed70a0da76f6b08330e9661ab1_414_410) I added a helper similar to set_ntstatus(), just for wait functions, so there is a bit less code duplication in v2 (with v3 fixing the unused variable I forgot to remove because of that)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120138
I added all kinds of tests for everything that is relevant to this MR (and also added a few missing NT pseudo-handle definitions)... Of note is that there is still a todo in the single wait case for `NtCurrentProcessToken()` and `NtCurrentEffectiveToken()`, but not `NtCurrentSession()` and `NtCurrentThreadToken()`. This seems to be to me due to difference in how server handles these handles, but compared to native Windows there are bigger problems than just that. For starters the pseudo-handles that are failing on Wine can actually be duplicated, whereas on Windows they cannot, so this seems to me at least a much bigger architectural issue than server-side than what the scope of this MR is. In the inproc_sync case, server returns `STATUS_NOT_IMPLEMENTED`, when trying to get the wait fd for these 2 pseudo handles, which causes ntdll to delegate to server wait and then return the same `STATUS_OBJECT_TYPE_MISMATCH` as with normal server-wait, instead of `STATUS_INVALID_HANDLE`, like for the other non-waitable pseudo-handles. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120139
On Wed Oct 29 22:28:44 2025 +0000, Paul Gofman wrote:
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? For std handles, it seems to be exactly the same (also part of the tests now).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120140
Paul Gofman (@gofman) commented about dlls/ntdll/unix/sync.c:
- 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;
same here, let's get rid of label and jump only meant to reuse a TRACE and better keep TRACE duplicated. That's also two lines FWIW. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120164
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)
just return STATUS_INVALID_HANDLE here I guess, no need to add a label and jump (even if we want to necessarily keep extra trace that is probably best without label and jump in this case). Also, probably '> 0xfffffffa' as in cache_inproc_sync()? In this specific case I'd probably suggest to add 'static BOOL is_pseudo_handle()' helper and use it here and in cache_inproc_sync(), for clarity. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120163
Paul Gofman (@gofman) commented about include/winbase.h:
#define GetCurrentProcess() NtCurrentProcess() #define GetCurrentThread() NtCurrentThread() +#define GetCurrentProcessToken() NtCurrentProcessToken()
Do we need changes in this patch for anything? I am not finding, e. g., NtCurrentProcessToken() definition in Windows SDK. Can't we just use existing GetCurrentProcessToken() in the added tests? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120165
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
LARGE_INTEGER timeout; NTSTATUS status; DWORD ret; + int i; + HANDLE waitable_pseudohandles[] = {
Formatting, probably more consistent to put '{' on the separate line (here and below). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120166
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
LARGE_INTEGER timeout; NTSTATUS status; DWORD ret; + int i; + HANDLE waitable_pseudohandles[] = { + NtCurrentProcess(), + NtCurrentThread()
While we are here, might put ',' on the last element in list too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120167
Paul Gofman (@gofman) commented about dlls/kernelbase/kernelbase.h:
return !status; }
+static inline DWORD set_ntwaitstatus( NTSTATUS status ) +{ + if (NT_ERROR(status)) + { + SetLastError( RtlNtStatusToDosError( status )); + return WAIT_FAILED; + } + return (DWORD)status; +} +
I doubt we need to factor out such a helper for just two wait functions, if we cannot use existing one it is probably better to duplicate this part. Probably changing to NT_ERROR() as Dmirty initially suggested. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120161
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
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());
Indentation (also in other places), I think it should either align to GetLastError() start or have two tabs from 'ok' start. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120162
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
ok(ret == WAIT_OBJECT_0, "expected WAIT_OBJECT_0, got %ld\n", ret); ok(GetLastError() == 0xdeadbeef, "expected 0xdeadbeef, got %ld\n", GetLastError());
- /* pseudo handles are allowed in WaitForSingleObject and NtWaitForSingleObject */ - ret = WaitForSingleObject(GetCurrentProcess(), 100); - ok(ret == WAIT_TIMEOUT, "expected WAIT_TIMEOUT, got %lu\n", ret); + /* waitable pseudo-handles are allowed in WaitForSingleObject and NtWaitForSingleObject, + * but not in NtWaitForMultipleObjects, see test_WaitForMultipleObjects */ + for (i = 0; i < ARRAY_SIZE(waitable_pseudohandles); i++) + { + ret = WaitForSingleObject(waitable_pseudohandles[i], 100); + ok(ret == WAIT_TIMEOUT, "expected WAIT_TIMEOUT, got %lu\n", ret);
Here and throughout the rest of the test, whenever we expect timeout to happen, and when in this case it is not subject to interaction with any other place, we can just wait with 0 timeout (both for kerbelbase and Nt functions). Currently the added test increases the test run time greatly without good reason. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120168
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
+ ok(status == STATUS_TIMEOUT, "expected STATUS_TIMEOUT, got %08lx\n", status); + }
- timeout.QuadPart = -1000000; - status = pNtWaitForSingleObject(GetCurrentProcess(), FALSE, &timeout); - ok(status == STATUS_TIMEOUT, "expected STATUS_TIMEOUT, got %08lx\n", status); + /* non-waitable pseudo-handles return STATUS_INVALID_HANDLE */ + for (i = 0; i < ARRAY_SIZE(non_waitable_pseudohandles); i++) + { + SetLastError(0xdeadbeef); + ret = WaitForSingleObject(non_waitable_pseudohandles[i], 100); + ok(ret == WAIT_FAILED, "expected WAIT_FAILED, got %ld\n", ret); + ok(GetLastError() == ERROR_INVALID_HANDLE, "expected ERROR_INVALID_HANDLE, got %ld\n", GetLastError()); + + timeout.QuadPart = -1000000; + status = pNtWaitForSingleObject(non_waitable_pseudohandles[i], FALSE, &timeout); When we, instead, expect the wait call to fail immediately with an error we can just pass NULL instead of timeout, sparing a bunch of unneeded lines with 'timeout.QuadPart = -1000000;' throughout the test.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120169
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
- timeout.QuadPart = -1000000; - status = pNtWaitForSingleObject(GetCurrentProcess(), FALSE, &timeout); - ok(status == STATUS_TIMEOUT, "expected STATUS_TIMEOUT, got %08lx\n", status); + /* non-waitable pseudo-handles return STATUS_INVALID_HANDLE */ + for (i = 0; i < ARRAY_SIZE(non_waitable_pseudohandles); i++) + { + SetLastError(0xdeadbeef); + ret = WaitForSingleObject(non_waitable_pseudohandles[i], 100); + ok(ret == WAIT_FAILED, "expected WAIT_FAILED, got %ld\n", ret); + ok(GetLastError() == ERROR_INVALID_HANDLE, "expected ERROR_INVALID_HANDLE, got %ld\n", GetLastError()); + + timeout.QuadPart = -1000000; + status = pNtWaitForSingleObject(non_waitable_pseudohandles[i], FALSE, &timeout); + todo_wine_if(non_waitable_pseudohandles[i] == NtCurrentProcessToken() || + non_waitable_pseudohandles[i] == NtCurrentEffectiveToken()) + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); It is best to add '&& status == <wine_error_code>' to todo_wine_if, to test and illustrate clearly that Wine wait doesn't succeed in this special case but returns a different error code.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120170
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
+ maxevents[0] = pseudohandles[i]; + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(1, maxevents, FALSE, 100); + 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); - 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; + status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); For completeness, I'd also add the same test with WaitAll after that (I checked locally that it works the same way at least on Win11).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120171
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
+ status = pNtWaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, WaitAny, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + timeout.QuadPart = -1000000; + status = pNtWaitForMultipleObjects(MAXIMUM_WAIT_OBJECTS, maxevents, WaitAll, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + maxevents[MAXIMUM_WAIT_OBJECTS - 1] = maxevents[0]; + } + + /* Similar to WaitForSingleObject std handles are only allowed in WaitForMultipleObjects and + * not NtWaitForMultipleObjects (for brevity we only test single count WaitAny here) */ + for (i = 0; i < ARRAY_SIZE(std_handles); i++) + { + maxevents[0] = std_handles[i]; + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(1, maxevents, FALSE, 100); + ok(r != WAIT_FAILED, "expected non-fail result, got %lu (GetLastError()=%lu)\n", r, GetLastError()); Is it possible to be specific in wait result here, maybe depending on the pseudo handle type? Or, if for some std handles it may timeout or succeed, accept any of those results but not generic '!= WAIT_FAILED'.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120172
Paul Gofman (@gofman) commented about dlls/kernel32/tests/sync.c:
+ { + maxevents[0] = std_handles[i]; + SetLastError(0xdeadbeef); + r = WaitForMultipleObjects(1, maxevents, FALSE, 100); + ok(r != WAIT_FAILED, "expected non-fail result, got %lu (GetLastError()=%lu)\n", r, GetLastError());
- timeout.QuadPart = -1000000; - maxevents[0] = GetCurrentThread(); - status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); - ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + timeout.QuadPart = -1000000; + status = pNtWaitForMultipleObjects(1, maxevents, WaitAny, FALSE, &timeout); + ok(status == STATUS_INVALID_HANDLE, "expected STATUS_INVALID_HANDLE, got %08lx\n", status); + } + + for (i=1; i<MAXIMUM_WAIT_OBJECTS; i++) Here and possibly elsewhere, spaces around '=' and '<'. If there is a bit of similar formatting in the existing test code I'd suggest to maybe change that too while there.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120173
Paul Gofman (@gofman) commented about dlls/ntdll/unix/sync.c:
*/ 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; Same as in NtWaitForMultipleObjects(), I don't think a label and goto are needed here, just return STATUS_INVALID_HANDLE or duplicate TRACE() before that (that will be the same two lines).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9305#note_120174
participants (3)
-
Marc-Aurel Zent -
Marc-Aurel Zent (＠mzent)
-
Paul Gofman (＠gofman)