[PATCH 0/3] 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. -- 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> 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/threadpool.c | 29 +++++++++++++++++++++++++++-- dlls/rtworkq/tests/rtworkq.c | 2 -- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/dlls/ntdll/threadpool.c b/dlls/ntdll/threadpool.c index 6448477a5cb..c32ce33d819 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) @@ -3066,7 +3083,15 @@ VOID WINAPI TpSetWait( TP_WAIT *wait, HANDLE handle, LARGE_INTEGER *timeout ) assert( this->u.wait.bucket ); - same_handle = this->u.wait.handle == handle; + if (!(same_handle = NT_SUCCESS(NtCompareObjects( 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
We should test this with threadpool API directly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8191#note_105222
Jinoh Kang (@iamahuman) commented about dlls/ntdll/threadpool.c:
assert( this->u.wait.bucket );
- same_handle = this->u.wait.handle == handle; + if (!(same_handle = NT_SUCCESS(NtCompareObjects( this->u.wait.handle, handle ))))
Same object does not mean same handle. Let's keep this as is. ```suggestion:-0+0 same_handle = this->u.wait.handle == handle; if (!same_handle) ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8191#note_105227
On Tue Jun 3 10:16:31 2025 +0000, Jinoh Kang wrote:
Same object does not mean same handle. Let's keep this as is. ```suggestion:-0+0 same_handle = this->u.wait.handle == handle; if (!same_handle) ``` I'll change it to set `same_handle` the existing way, but duplication needs `NtCompareObjects()` or an alternative, because `this->u.wait.handle` may have been closed and the handle value reused for a new handle passed as the `handle` parameter.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8191#note_105249
On Tue Jun 3 14:27:52 2025 +0000, Conor McCarthy wrote:
I'll change it to set `same_handle` the existing way, but duplication needs `NtCompareObjects()` or an alternative, because `this->u.wait.handle` may have been closed and the handle value reused for a new handle passed as the `handle` parameter. Shouldn't we just always duplicate the handle? That sounds simpler.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8191#note_105257
participants (4)
-
Conor McCarthy -
Conor McCarthy (＠cmccarthy)
-
Jinoh Kang (＠iamahuman)
-
Nikolay Sivov (＠nsivov)