[PATCH v2 0/4] MR8191: ntdll: Duplicate handles for thread pool waits.
Windows allows closure of a waitable timer handle while a work item is waiting on it. Also, the current Wine ntdll implementation calls NtWaitForMultipleObjects() on multiple handles if multiple items are pending, and if one handle is not valid, no items will execute. Btw there are occurrences of `INVALID_HANDLE_VALUE` elsewhere, e.g. in `RtlDeleteTimer()`, which I think are incorrect, unless Windows internals are inconsistent with its use. -- v2: ntdll: Duplicate handles for thread pool waits. ntdll: Initialise waitable handles with NULL. ntdll/tests: Test early closure of handles used for threadpool waits. https://gitlab.winehq.org/wine/wine/-/merge_requests/8191
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Windows allows closure of a waitable timer handle while a work item is waiting on it. Also, the current Wine ntdll implementation calls NtWaitForMultipleObjects() on multiple handles if multiple items are pending, and if one handle is not valid, no items will execute. --- dlls/rtworkq/tests/rtworkq.c | 72 +++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/dlls/rtworkq/tests/rtworkq.c b/dlls/rtworkq/tests/rtworkq.c index 7655275d721..440a574aaf3 100644 --- a/dlls/rtworkq/tests/rtworkq.c +++ b/dlls/rtworkq/tests/rtworkq.c @@ -475,11 +475,14 @@ static void test_work_queue(void) static void test_scheduled_items(void) { - struct test_callback *test_callback; + struct test_callback *test_callback, *test_callback2; RTWQWORKITEM_KEY key, key2; - IRtwqAsyncResult *result; + IRtwqAsyncResult *result, *result2, *callback_result; + HANDLE timer, event, event2; + LARGE_INTEGER time; ULONG refcount; HRESULT hr; + DWORD res; test_callback = create_test_callback(); @@ -527,7 +530,72 @@ static void test_scheduled_items(void) refcount = IRtwqAsyncResult_Release(result); ok(refcount == 0, "Unexpected refcount %lu.\n", refcount); + hr = RtwqStartup(); + ok(hr == S_OK, "Failed to start up, hr %#lx.\n", hr); + + test_callback2 = create_test_callback(); + + timer = CreateWaitableTimerW(NULL, TRUE, NULL); + event = CreateEventA(NULL, FALSE, FALSE, NULL); + + hr = RtwqCreateAsyncResult(NULL, &test_callback->IRtwqAsyncCallback_iface, NULL, &result); + ok(hr == S_OK, "Failed to create result, hr %#lx.\n", hr); + hr = RtwqCreateAsyncResult(NULL, &test_callback2->IRtwqAsyncCallback_iface, NULL, &result2); + ok(hr == S_OK, "Failed to create result, hr %#lx.\n", hr); + + time.QuadPart = -1000000LL; + SetWaitableTimer(timer, &time, 0, NULL, NULL, 0); + hr = RtwqPutWaitingWorkItem(timer, 0, result, NULL); + ok(hr == S_OK, "got %#lx\n", hr); + /* Close the timer handle while the item is pending. This should work and + * not cause failure to execute other waiting items.*/ + CloseHandle(timer); + IRtwqAsyncResult_Release(result); + + hr = RtwqPutWaitingWorkItem(event, 0, result2, NULL); + ok(hr == S_OK, "got %#lx\n", hr); + IRtwqAsyncResult_Release(result2); + res = wait_async_callback_result(&test_callback->IRtwqAsyncCallback_iface, 200, &callback_result); + todo_wine + ok(res == 0, "got %#lx\n", res); + + SetEvent(event); + res = wait_async_callback_result(&test_callback2->IRtwqAsyncCallback_iface, 100, &callback_result); + ok(res == 0, "got %#lx\n", res); + + CloseHandle(event); + + event = CreateEventA(NULL, FALSE, FALSE, NULL); + event2 = CreateEventA(NULL, FALSE, FALSE, NULL); + + hr = RtwqCreateAsyncResult(NULL, &test_callback->IRtwqAsyncCallback_iface, NULL, &result); + ok(hr == S_OK, "Failed to create result, hr %#lx.\n", hr); + hr = RtwqCreateAsyncResult(NULL, &test_callback2->IRtwqAsyncCallback_iface, NULL, &result2); + ok(hr == S_OK, "Failed to create result, hr %#lx.\n", hr); + + hr = RtwqPutWaitingWorkItem(event, 0, result, &key); + ok(hr == S_OK, "got %#lx\n", hr); + /* Abandon the waiting item. This should not cause failure to execute other waiting items. */ + CloseHandle(event); + IRtwqAsyncResult_Release(result); + + hr = RtwqPutWaitingWorkItem(event2, 0, result2, NULL); + ok(hr == S_OK, "got %#lx\n", hr); + IRtwqAsyncResult_Release(result2); + SetEvent(event2); + res = wait_async_callback_result(&test_callback2->IRtwqAsyncCallback_iface, 100, &callback_result); + todo_wine + ok(res == 0, "got %#lx\n", res); + + hr = RtwqCancelWorkItem(key); + ok(hr == S_OK, "Failed to cancel item, hr %#lx.\n", hr); + CloseHandle(event2); + + hr = RtwqShutdown(); + ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr); + IRtwqAsyncCallback_Release(&test_callback->IRtwqAsyncCallback_iface); + IRtwqAsyncCallback_Release(&test_callback2->IRtwqAsyncCallback_iface); } static void test_queue_shutdown(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8191
From: Conor McCarthy <cmccarthy(a)codeweavers.com> --- dlls/ntdll/tests/threadpool.c | 91 +++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/dlls/ntdll/tests/threadpool.c b/dlls/ntdll/tests/threadpool.c index f330cf0671e..b903165a4fb 100644 --- a/dlls/ntdll/tests/threadpool.c +++ b/dlls/ntdll/tests/threadpool.c @@ -2387,6 +2387,96 @@ static void test_kernel32_tp_io(void) pTpReleasePool(pool); } +static void test_tp_wait_early_closure(void) +{ + TP_CALLBACK_ENVIRON environment; + TP_WAIT *wait1, *wait2; + struct wait_info info; + HANDLE semaphores[2]; + LARGE_INTEGER when; + HANDLE semaphore; + NTSTATUS status; + TP_POOL *pool; + HANDLE timer; + DWORD result; + + semaphores[0] = CreateSemaphoreW(NULL, 0, 2, NULL); + ok(semaphores[0] != NULL, "failed to create semaphore\n"); + semaphores[1] = CreateSemaphoreW(NULL, 0, 1, NULL); + ok(semaphores[1] != NULL, "failed to create semaphore\n"); + semaphore = CreateSemaphoreW(NULL, 0, 1, NULL); + ok(semaphore != NULL, "failed to create semaphore\n"); + timer = CreateWaitableTimerW(NULL, TRUE, NULL); + ok(timer != NULL, "failed to create waitable timer\n"); + info.semaphore = semaphore; + + /* allocate new threadpool */ + pool = NULL; + status = pTpAllocPool(&pool, NULL); + ok(!status, "TpAllocPool failed with status %lx\n", status); + ok(pool != NULL, "expected pool != NULL\n"); + + /* allocate new wait items */ + memset(&environment, 0, sizeof(environment)); + environment.Version = 1; + environment.Pool = pool; + + wait1 = NULL; + status = pTpAllocWait(&wait1, wait_cb, &info, &environment); + ok(!status, "TpAllocWait failed with status %lx\n", status); + ok(wait1 != NULL, "expected wait1 != NULL\n"); + + wait2 = NULL; + status = pTpAllocWait(&wait2, wait_cb, &info, &environment); + ok(!status, "TpAllocWait failed with status %lx\n", status); + ok(wait2 != NULL, "expected wait2 != NULL\n"); + + /* waitable timer closed immediately, and a semaphore */ + when.QuadPart = (ULONGLONG)100 * -10000; + status = NtSetTimer(timer, &when, NULL, NULL, FALSE, 0, NULL); + ok(!status, "NtSetTimer returned status %lx\n", status); + info.userdata = 0; + pTpSetWait(wait1, timer, NULL); + CloseHandle(timer); + pTpSetWait(wait2, semaphores[0], NULL); + result = WaitForSingleObject(semaphore, 200); + todo_wine + ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); + ok(info.userdata == 1, "expected info.userdata = 1, got %lu\n", info.userdata); + ReleaseSemaphore(semaphores[0], 1, NULL); + result = WaitForSingleObject(semaphore, 200); + ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); + todo_wine + ok(info.userdata == 2, "expected info.userdata = 2, got %lu\n", info.userdata); + result = WaitForSingleObject(semaphores[0], 0); + ok(result == WAIT_TIMEOUT, "WaitForSingleObject returned %#lx\n", result); + + /* two semaphores, the first closed immediately */ + info.userdata = 0; + when.QuadPart = (ULONGLONG)200 * -10000; + pTpSetWait(wait1, semaphores[0], &when); + CloseHandle(semaphores[0]); + pTpSetWait(wait2, semaphores[1], NULL); + ReleaseSemaphore(semaphores[1], 1, NULL); + result = WaitForSingleObject(semaphore, 100); + ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); + todo_wine + ok(info.userdata == 1, "expected info.userdata = 1, got %lu\n", info.userdata); + result = WaitForSingleObject(semaphores[1], 0); + ok(result == WAIT_TIMEOUT, "WaitForSingleObject returned %#lx\n", result); + result = WaitForSingleObject(semaphore, 300); + ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); + todo_wine + ok(info.userdata == 0x10001, "expected info.userdata = 0x10001, got %lu\n", info.userdata); + + /* cleanup */ + pTpReleaseWait(wait1); + pTpReleaseWait(wait2); + pTpReleasePool(pool); + CloseHandle(semaphores[1]); + CloseHandle(semaphore); +} + START_TEST(threadpool) { test_RtlQueueWorkItem(); @@ -2408,4 +2498,5 @@ START_TEST(threadpool) test_tp_multi_wait(); test_tp_io(); test_kernel32_tp_io(); + test_tp_wait_early_closure(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8191
From: Conor McCarthy <cmccarthy(a)codeweavers.com> All waitable objects use NULL as their invalid value. --- dlls/ntdll/threadpool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index 81cf894d943..6448477a5cb 100644 --- a/dlls/ntdll/threadpool.c +++ b/dlls/ntdll/threadpool.c @@ -1436,7 +1436,7 @@ static NTSTATUS tp_waitqueue_lock( struct threadpool_object *wait ) wait->u.wait.bucket = NULL; wait->u.wait.wait_pending = FALSE; wait->u.wait.timeout = 0; - wait->u.wait.handle = INVALID_HANDLE_VALUE; + wait->u.wait.handle = NULL; RtlEnterCriticalSection( &waitqueue.cs ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8191
From: Conor McCarthy <cmccarthy(a)codeweavers.com> Supports waitable timer closure while pending, and we must not wait on invalid handles generally. Details are in code comments. --- dlls/ntdll/tests/threadpool.c | 4 ---- dlls/ntdll/threadpool.c | 25 ++++++++++++++++++++++++- dlls/rtworkq/tests/rtworkq.c | 2 -- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/dlls/ntdll/tests/threadpool.c b/dlls/ntdll/tests/threadpool.c index b903165a4fb..55d59d50d86 100644 --- a/dlls/ntdll/tests/threadpool.c +++ b/dlls/ntdll/tests/threadpool.c @@ -2440,13 +2440,11 @@ static void test_tp_wait_early_closure(void) CloseHandle(timer); pTpSetWait(wait2, semaphores[0], NULL); result = WaitForSingleObject(semaphore, 200); - todo_wine ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); ok(info.userdata == 1, "expected info.userdata = 1, got %lu\n", info.userdata); ReleaseSemaphore(semaphores[0], 1, NULL); result = WaitForSingleObject(semaphore, 200); ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); - todo_wine ok(info.userdata == 2, "expected info.userdata = 2, got %lu\n", info.userdata); result = WaitForSingleObject(semaphores[0], 0); ok(result == WAIT_TIMEOUT, "WaitForSingleObject returned %#lx\n", result); @@ -2460,13 +2458,11 @@ static void test_tp_wait_early_closure(void) ReleaseSemaphore(semaphores[1], 1, NULL); result = WaitForSingleObject(semaphore, 100); ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); - todo_wine ok(info.userdata == 1, "expected info.userdata = 1, got %lu\n", info.userdata); result = WaitForSingleObject(semaphores[1], 0); ok(result == WAIT_TIMEOUT, "WaitForSingleObject returned %#lx\n", result); result = WaitForSingleObject(semaphore, 300); ok(result == WAIT_OBJECT_0, "WaitForSingleObject returned %#lx\n", result); - todo_wine ok(info.userdata == 0x10001, "expected info.userdata = 0x10001, got %lu\n", info.userdata); /* cleanup */ diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index 6448477a5cb..5fb11c35698 100644 --- a/dlls/ntdll/threadpool.c +++ b/dlls/ntdll/threadpool.c @@ -194,6 +194,7 @@ struct threadpool_object struct list wait_entry; ULONGLONG timeout; HANDLE handle; + HANDLE duped_handle; DWORD flags; RTL_WAITORTIMERCALLBACKFUNC rtl_callback; } wait; @@ -1296,7 +1297,10 @@ static void CALLBACK waitqueue_thread_proc( void *param ) assert( num_handles < MAXIMUM_WAITQUEUE_OBJECTS ); InterlockedIncrement( &wait->refcount ); objects[num_handles] = wait; - handles[num_handles] = wait->u.wait.handle; + /* NtWaitForMultipleObjects() fails if any invalid handles are passed, and one invalid handle + * should not affect other waiting items. The calling app is allowed to close waitable timer + * handles immediately after submission, so we need a duplicate for those in particular. */ + handles[num_handles] = wait->u.wait.duped_handle ? wait->u.wait.duped_handle : wait->u.wait.handle; update_serials[num_handles] = wait->update_serial; num_handles++; } @@ -1437,6 +1441,7 @@ static NTSTATUS tp_waitqueue_lock( struct threadpool_object *wait ) wait->u.wait.wait_pending = FALSE; wait->u.wait.timeout = 0; wait->u.wait.handle = NULL; + wait->u.wait.duped_handle = NULL; RtlEnterCriticalSection( &waitqueue.cs ); @@ -2117,6 +2122,15 @@ static void tp_object_prepare_shutdown( struct threadpool_object *object ) tp_ioqueue_unlock( object ); } +static void tp_wait_close_duped_handle( struct threadpool_object *wait ) +{ + if (wait->u.wait.duped_handle) + { + NtClose( wait->u.wait.duped_handle ); + wait->u.wait.duped_handle = NULL; + } +} + /*********************************************************************** * tp_object_release (internal) * @@ -2150,6 +2164,9 @@ static BOOL tp_object_release( struct threadpool_object *object ) tp_group_release( group ); } + if (object->type == TP_OBJECT_TYPE_WAIT) + tp_wait_close_duped_handle( object ); + tp_threadpool_unlock( object->pool ); if (object->race_dll) @@ -3067,6 +3084,12 @@ VOID WINAPI TpSetWait( TP_WAIT *wait, HANDLE handle, LARGE_INTEGER *timeout ) assert( this->u.wait.bucket ); same_handle = this->u.wait.handle == handle; + tp_wait_close_duped_handle( this ); + if (handle && NtDuplicateObject( NtCurrentProcess(), handle, NtCurrentProcess(), + &this->u.wait.duped_handle, 0, 0, DUPLICATE_SAME_ACCESS ) != STATUS_SUCCESS) + { + WARN( "Failed to duplicate handle.\n" ); + } this->u.wait.handle = handle; if (handle || this->u.wait.wait_pending) diff --git a/dlls/rtworkq/tests/rtworkq.c b/dlls/rtworkq/tests/rtworkq.c index 440a574aaf3..812c1f03c1e 100644 --- a/dlls/rtworkq/tests/rtworkq.c +++ b/dlls/rtworkq/tests/rtworkq.c @@ -556,7 +556,6 @@ static void test_scheduled_items(void) ok(hr == S_OK, "got %#lx\n", hr); IRtwqAsyncResult_Release(result2); res = wait_async_callback_result(&test_callback->IRtwqAsyncCallback_iface, 200, &callback_result); - todo_wine ok(res == 0, "got %#lx\n", res); SetEvent(event); @@ -584,7 +583,6 @@ static void test_scheduled_items(void) IRtwqAsyncResult_Release(result2); SetEvent(event2); res = wait_async_callback_result(&test_callback2->IRtwqAsyncCallback_iface, 100, &callback_result); - todo_wine ok(res == 0, "got %#lx\n", res); hr = RtwqCancelWorkItem(key); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8191
On Wed Jun 4 05:24:20 2025 +0000, Conor McCarthy wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/8191/diffs?diff_id=182619&start_sha=d825e8c5e5cb5bd28ba11a49dba754aefbcdb5ac#efc6febb3be7a57b52f19637ee05baee9943d824_3086_3086) Yes, this is necessary to pass the last of the new threadpool tests, `userdata == 0x10001`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8191#note_105311
As a sidenote, the reason Windows can deal with handle rugpull is due to the use of wait completion packets, as hinted in https://devblogs.microsoft.com/oldnewthing/20220406-00/?p=106434. In fact we already have some basic WaitCompletionPacket implementation in works: !6911. A "right" direction would involve rewriting TP logic around completion packets. However this patch seems arguably a reasonable compromise, so I'm a bit split here. Hopefully someone more knowledgeable could step in and have a say on what would constitute the right direction forward. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8191#note_105366
participants (3)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Jinoh Kang (＠iamahuman)