[PATCH v2 0/3] MR4243: rtworkq: free cancelled work items
-- v2: rtworkq: Release cancelled work items. 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index 15b8da47639..0242cd0d61b 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -737,9 +737,9 @@ static void queue_release_pending_item(struct work_item *item) { list_remove(&item->entry); item->key = 0; - IUnknown_Release(&item->IUnknown_iface); } LeaveCriticalSection(&item->queue->cs); + 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 0242cd0d61b..8e34341c621 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -876,32 +876,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
Zhiyi Zhang (@zhiyi) commented about dlls/rtworkq/queue.c:
{ list_remove(&item->entry); item->key = 0; - IUnknown_Release(&item->IUnknown_iface); } LeaveCriticalSection(&item->queue->cs); + IUnknown_Release(&item->IUnknown_iface);
This releases &item->IUnknown_iface even though item->key is NULL, which is a behavior. Are you sure this is correct? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50486
On Wed Nov 1 14:12:42 2023 +0000, Zhiyi Zhang wrote:
This releases &item->IUnknown_iface even though item->key is NULL, which is a behavior change. Are you sure this is correct? yeah you are right. i just had a quick check and it looks like all the callsites would have `item->key != 0`, but i should definitely be careful here.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_50487
participants (3)
-
Yuxuan Shui -
Yuxuan Shui (＠yshui)
-
Zhiyi Zhang (＠zhiyi)