[PATCH v3 0/3] MR4243: rtworkq: free cancelled work items
-- v3: rtworkq: Release cancelled work items. mfplat: tests: Validate MFCancelWorkItem releases the callback. rtworkq: Avoid use-after-free. https://gitlab.winehq.org/wine/wine/-/merge_requests/4243
From: Yuxuan Shui <yshui(a)codeweavers.com> queue_release_pending_item releases the work_item reference but later accesses `item->queue`, which is a potential use-after-free. --- dlls/rtworkq/queue.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index 15b8da47639..27d715a9bf9 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -732,14 +732,18 @@ static HRESULT invoke_async_callback(IRtwqAsyncResult *result) static void queue_release_pending_item(struct work_item *item) { + BOOL should_release = FALSE; EnterCriticalSection(&item->queue->cs); if (item->key) { list_remove(&item->entry); item->key = 0; - IUnknown_Release(&item->IUnknown_iface); + should_release = TRUE; } LeaveCriticalSection(&item->queue->cs); + + if (should_release) + IUnknown_Release(&item->IUnknown_iface); } static void CALLBACK waiting_item_callback(TP_CALLBACK_INSTANCE *instance, void *context, TP_WAIT *wait, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4243
From: Yuxuan Shui <yshui(a)codeweavers.com> --- dlls/mfplat/tests/mfplat.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 5f9460655ec..7f0d721392d 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -3560,6 +3560,7 @@ static void test_scheduled_items(void) IMFAsyncResult *result; MFWORKITEM_KEY key, key2; HRESULT hr; + ULONG refcount; callback = create_test_callback(NULL); @@ -3572,6 +3573,9 @@ static void test_scheduled_items(void) hr = MFCancelWorkItem(key); ok(hr == S_OK, "Failed to cancel item, hr %#lx.\n", hr); + refcount = IMFAsyncCallback_Release(&callback->IMFAsyncCallback_iface); + ok(refcount == 0, "Unexpected refcount %lu.\n", refcount); + hr = MFCancelWorkItem(key); ok(hr == MF_E_NOT_FOUND || broken(hr == S_OK) /* < win10 */, "Unexpected hr %#lx.\n", hr); @@ -3581,6 +3585,8 @@ static void test_scheduled_items(void) return; } + callback = create_test_callback(NULL); + hr = MFCreateAsyncResult(NULL, &callback->IMFAsyncCallback_iface, NULL, &result); ok(hr == S_OK, "Failed to create result, hr %#lx.\n", hr); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4243
From: Yuxuan Shui <yshui(a)codeweavers.com> And avoiding race condition while doing so. Usually the threadpool holds a reference to the work_item, which is released when the work_item's callback is invoked. However queue_cancel_item closes the threadpool object without releasing it, leading to a leak. The fix is not as simple as adding a IUnknown_Release, because the work_item's callback can still be called after CloseThreadpoolTimer/Wait has returned. In fact its callback might currently be running. In which case we would double release the reference. We have to stop any further callbacks to be queued, wait for any currently running callbacks to finish, then finally we can close the threadpool object. After that, if the callback wasn't called, we can safely release the work_item reference. --- dlls/rtworkq/queue.c | 73 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 64 insertions(+), 9 deletions(-) diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index 27d715a9bf9..ee7b8ad4d2e 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -880,32 +880,87 @@ static HRESULT queue_submit_timer(struct queue *queue, IRtwqAsyncResult *result, return S_OK; } -static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) +static HRESULT queue_cancel_item(struct queue *queue, const RTWQWORKITEM_KEY key) { HRESULT hr = RTWQ_E_NOT_FOUND; + union { TP_WAIT *wait_object; TP_TIMER *timer_object; } work_object; + enum work_item_type work_object_type; struct work_item *item; + const UINT64 mask = key >> 32; EnterCriticalSection(&queue->cs); LIST_FOR_EACH_ENTRY(item, &queue->pending_items, struct work_item, entry) { if (item->key == key) { - key >>= 32; - if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) + hr = S_OK; + if ((mask & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) + { + if (item->type != WORK_ITEM_WAIT) + WARN("Item %p is not a wait item, but its key has wait item mask.\n", item); + + work_object_type = WORK_ITEM_WAIT; + work_object.wait_object = item->u.wait_object; + item->u.wait_object = NULL; + } + else if ((mask & SCHEDULED_ITEM_KEY_MASK) == SCHEDULED_ITEM_KEY_MASK) + { + if (item->type != WORK_ITEM_TIMER) + WARN("Item %p is not a timer item, but its key has timer item mask.\n", item); + + work_object_type = WORK_ITEM_TIMER; + work_object.timer_object = item->u.timer_object; + item->u.timer_object = NULL; + } + else + { + WARN("Unknown item key mask %#I64x.\n", mask); + queue_release_pending_item(item); + goto out; + } + break; + } + } + + if (FAILED(hr)) + goto out; + + LeaveCriticalSection(&queue->cs); + + // Safely either stop the thread pool object, or if the callback is already running, wait for it to finish. + // This way, we can safely release the reference to the work item. + if (work_object_type == WORK_ITEM_WAIT) + { + SetThreadpoolWait(work_object.wait_object, NULL, NULL); + WaitForThreadpoolWaitCallbacks(work_object.wait_object, TRUE); + CloseThreadpoolWait(work_object.wait_object); + } + else if (work_object_type == WORK_ITEM_TIMER) + { + SetThreadpoolTimer(work_object.timer_object, NULL, 0, 0); + WaitForThreadpoolTimerCallbacks(work_object.timer_object, TRUE); + CloseThreadpoolTimer(work_object.timer_object); + } + + // If the work item is still in pending items, its callback hasn't been invoked yet; + // we remove it. Otherwise its callback would have already released it. + EnterCriticalSection(&queue->cs); + LIST_FOR_EACH_ENTRY(item, &queue->pending_items, struct work_item, entry) + { + if (item->key == key) + { + if (work_object_type == WORK_ITEM_WAIT) { IRtwqAsyncResult_SetStatus(item->result, RTWQ_E_OPERATION_CANCELLED); invoke_async_callback(item->result); - CloseThreadpoolWait(item->u.wait_object); } - else if ((key & SCHEDULED_ITEM_KEY_MASK) == SCHEDULED_ITEM_KEY_MASK) - CloseThreadpoolTimer(item->u.timer_object); - else - WARN("Unknown item key mask %#I64x.\n", key); queue_release_pending_item(item); - hr = S_OK; + IUnknown_Release(&item->IUnknown_iface); break; } } + +out: LeaveCriticalSection(&queue->cs); return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4243
On Wed Nov 1 14:13:04 2023 +0000, Yuxuan Shui wrote:
changed this line in [version 3 of the diff](/wine/wine/-/merge_requests/4243/diffs?diff_id=80306&start_sha=79fd34d568c5c5f93a29cda5741467a81042de26#7c1e37217b904b3c36290ceb09bec98217131c4a_742_744) addressed
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50490
Nikolay Sivov (@nsivov) commented about dlls/rtworkq/queue.c:
static void queue_release_pending_item(struct work_item *item) { + BOOL should_release = FALSE; EnterCriticalSection(&item->queue->cs); if (item->key) { list_remove(&item->entry); item->key = 0; - IUnknown_Release(&item->IUnknown_iface); + should_release = TRUE; } LeaveCriticalSection(&item->queue->cs); + + if (should_release) + IUnknown_Release(&item->IUnknown_iface); } Just add a temp "struct queue *", and keep the rest as is.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50493
Nikolay Sivov (@nsivov) commented about dlls/mfplat/tests/mfplat.c:
hr = MFCancelWorkItem(key); ok(hr == S_OK, "Failed to cancel item, hr %#lx.\n", hr);
+ refcount = IMFAsyncCallback_Release(&callback->IMFAsyncCallback_iface); + ok(refcount == 0, "Unexpected refcount %lu.\n", refcount); +
I don't see why that would be a concern, but checking this is fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50494
Nikolay Sivov (@nsivov) commented about dlls/rtworkq/queue.c:
return S_OK; }
-static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) +static HRESULT queue_cancel_item(struct queue *queue, const RTWQWORKITEM_KEY key)
What's the purpose of this 'const'? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50495
Nikolay Sivov (@nsivov) commented about dlls/rtworkq/queue.c:
+ + // Safely either stop the thread pool object, or if the callback is already running, wait for it to finish. + // This way, we can safely release the reference to the work item. + if (work_object_type == WORK_ITEM_WAIT) + { + SetThreadpoolWait(work_object.wait_object, NULL, NULL); + WaitForThreadpoolWaitCallbacks(work_object.wait_object, TRUE); + CloseThreadpoolWait(work_object.wait_object); + } + else if (work_object_type == WORK_ITEM_TIMER) + { + SetThreadpoolTimer(work_object.timer_object, NULL, 0, 0); + WaitForThreadpoolTimerCallbacks(work_object.timer_object, TRUE); + CloseThreadpoolTimer(work_object.timer_object); + } + I haven't read through properly yet, but isn't it possible TP objects might already be freed at this point, or at any point really.
Is this fix purely theoretical, or it's possible to demonstrate the issue reliably? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50496
On Wed Nov 1 14:24:51 2023 +0000, Nikolay Sivov wrote:
I don't see why that would be a concern, but checking this is fine. the concern is that this is causing a memory leak.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50497
On Wed Nov 1 14:24:52 2023 +0000, Nikolay Sivov wrote:
What's the purpose of this 'const'? to make sure i didn't accidentally change `key`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50498
On Wed Nov 1 14:24:52 2023 +0000, Nikolay Sivov wrote:
I haven't read through properly yet, but isn't it possible TP objects might already be freed at this point, or at any point really. Is this fix purely theoretical, or it's possible to demonstrate the issue reliably? the only place the TP objects are freed is in this function or in `work_item_Release`. we set the pointers to the TP objects to `NULL` while holding the queue lock (which means no other threads can remove the item from `queue->pending_items`, which in turn means the work item will have at least 1 reference), so `work_item_Release` won't release it. therefore we can safely release it here.
i haven't tried to demonstrate this issue but it's clearly there -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50500
participants (3)
-
Nikolay Sivov (＠nsivov) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)