[PATCH v14 0/3] MR4243: rtworkq: free cancelled work items
-- v14: 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 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index eebb096ad31..00b77bf6953 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -734,9 +734,10 @@ static HRESULT invoke_async_callback(IRtwqAsyncResult *result) * removed from pending items when it got canceled. */ static BOOL queue_release_pending_item(struct work_item *item) { + struct queue *queue = item->queue; BOOL ret = FALSE; - EnterCriticalSection(&item->queue->cs); + EnterCriticalSection(&queue->cs); if (item->key) { list_remove(&item->entry); @@ -744,7 +745,7 @@ static BOOL queue_release_pending_item(struct work_item *item) item->key = 0; IUnknown_Release(&item->IUnknown_iface); } - LeaveCriticalSection(&item->queue->cs); + LeaveCriticalSection(&queue->cs); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4243
From: Yuxuan Shui <yshui(a)codeweavers.com> --- dlls/mfplat/tests/mfplat.c | 63 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 6897036e9dd..8298b140897 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -584,6 +584,51 @@ static const IMFAsyncCallbackVtbl test_async_callback_result_vtbl = test_async_callback_result_Invoke, }; +/* Test context for MFAddPeriodCallback. */ +struct test_context +{ + IUnknown IUnknown_iface; + LONG refcount; +}; + +static struct test_context *test_context_from_IUnknown(IUnknown *iface) +{ + return CONTAINING_RECORD(iface, struct test_context, IUnknown_iface); +} + +static HRESULT WINAPI test_context_QueryInterface(IUnknown *iface, REFIID riid, void **obj) +{ + if (IsEqualIID(riid, &IID_IUnknown)) + { + *obj = iface; + IUnknown_AddRef(iface); + return S_OK; + } + + *obj = NULL; + return E_NOINTERFACE; +} + +static ULONG WINAPI test_context_AddRef(IUnknown *iface) +{ + struct test_context *context = test_context_from_IUnknown(iface); + return InterlockedIncrement(&context->refcount); +} + +static ULONG WINAPI test_context_Release(IUnknown *iface) +{ + struct test_context *context = test_context_from_IUnknown(iface); + ULONG refcount = InterlockedDecrement(&context->refcount); + return refcount; +} + +static const IUnknownVtbl test_context_vtbl = +{ + test_context_QueryInterface, + test_context_AddRef, + test_context_Release, +}; + static DWORD wait_async_callback_result(IMFAsyncCallback *iface, DWORD timeout, IMFAsyncResult **result) { struct test_callback *callback = impl_from_IMFAsyncCallback(iface); @@ -3598,6 +3643,7 @@ static void test_scheduled_items(void) IMFAsyncResult *result; MFWORKITEM_KEY key, key2; HRESULT hr; + ULONG refcount; callback = create_test_callback(NULL); @@ -3610,6 +3656,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); + todo_wine 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); @@ -3619,6 +3668,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); @@ -3715,6 +3766,10 @@ static void test_periodic_callback(void) { DWORD period, key; HRESULT hr; + struct test_context context = { + .IUnknown_iface = { &test_context_vtbl }, + .refcount = 1, + }; hr = MFStartup(MF_VERSION, MFSTARTUP_FULL); ok(hr == S_OK, "Failed to start up, hr %#lx.\n", hr); @@ -3745,6 +3800,14 @@ static void test_periodic_callback(void) ok(periodic_counter > 0, "Unexpected counter value %lu.\n", periodic_counter); + hr= pMFAddPeriodicCallback(periodic_callback, &context.IUnknown_iface, &key); + ok(hr == S_OK, "Failed to add periodic callback, hr %#lx.\n", hr); + ok(context.refcount == 2, "Unexpected refcount %ld.\n", context.refcount); + + hr = pMFRemovePeriodicCallback(key); + ok(hr == S_OK, "Failed to remove callback, hr %#lx.\n", hr); + todo_wine ok(context.refcount == 1, "Unexpected refcount %ld.\n", context.refcount); + hr = MFShutdown(); ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4243
From: Yuxuan Shui <yshui(a)codeweavers.com> Usually the threadpool holds a reference to the `work_item`, which is released when the `work_item`'s callback is invoked. On the other hand, `queue_cancel_item` closes the threadpool object without releasing the `work_item`. So if the callbacks don't get a chance to run - which is not guaranteed - the `work_item` will be leaked. The fix is not as simple as adding a `IUnknown_Release` to `queue_cancel_item`, 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 the callback will access freed memory if `queue_cancel_item` frees the `work_item`. We have to stop any further callbacks to be queued, wait for any currently running callbacks to finish, then finally we can release the `work_item` if it hasn't already been freed during the wait. --- dlls/mfplat/tests/mfplat.c | 4 ++-- dlls/rtworkq/queue.c | 43 ++++++++++++++++++++++++++++++++------ 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 8298b140897..2b63f1da14c 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -3657,7 +3657,7 @@ static void test_scheduled_items(void) ok(hr == S_OK, "Failed to cancel item, hr %#lx.\n", hr); refcount = IMFAsyncCallback_Release(&callback->IMFAsyncCallback_iface); - todo_wine ok(refcount == 0, "Unexpected refcount %lu.\n", refcount); + 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); @@ -3806,7 +3806,7 @@ static void test_periodic_callback(void) hr = pMFRemovePeriodicCallback(key); ok(hr == S_OK, "Failed to remove callback, hr %#lx.\n", hr); - todo_wine ok(context.refcount == 1, "Unexpected refcount %ld.\n", context.refcount); + ok(context.refcount == 1, "Unexpected refcount %ld.\n", context.refcount); hr = MFShutdown(); ok(hr == S_OK, "Failed to shut down, hr %#lx.\n", hr); diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index 00b77bf6953..8450a5180e8 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -884,6 +884,8 @@ static HRESULT queue_submit_timer(struct queue *queue, IRtwqAsyncResult *result, static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) { HRESULT hr = RTWQ_E_NOT_FOUND; + TP_WAIT *wait_object; + TP_TIMER *timer_object; struct work_item *item; EnterCriticalSection(&queue->cs); @@ -891,27 +893,56 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) { if (item->key == key) { + /* We can't immediately release the item here, because the callback could already be + * running somewhere else. And if we release it here, the callback will access freed memory. + * So instead we have to make sure the callback is really stopped, or has really finished + * running before we do that. And we can't do that in this critical section, which would be a + * deadlock. So we first keep an extra reference to it, then leave the critical section to + * wait for the thread-pool objects, finally we re-enter critical section to release it. */ key >>= 32; + IUnknown_AddRef(&item->IUnknown_iface); if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) { - IRtwqAsyncResult_SetStatus(item->result, RTWQ_E_OPERATION_CANCELLED); - invoke_async_callback(item->result); - CloseThreadpoolWait(item->u.wait_object); + wait_object = item->u.wait_object; item->u.wait_object = NULL; + LeaveCriticalSection(&queue->cs); + + SetThreadpoolWait(wait_object, NULL, NULL); + WaitForThreadpoolWaitCallbacks(wait_object, TRUE); + CloseThreadpoolWait(wait_object); } else if ((key & SCHEDULED_ITEM_KEY_MASK) == SCHEDULED_ITEM_KEY_MASK) { - CloseThreadpoolTimer(item->u.timer_object); + timer_object = item->u.timer_object; item->u.timer_object = NULL; + LeaveCriticalSection(&queue->cs); + + SetThreadpoolTimer(timer_object, NULL, 0, 0); + WaitForThreadpoolTimerCallbacks(timer_object, TRUE); + CloseThreadpoolTimer(timer_object); } else + { WARN("Unknown item key mask %#I64x.\n", key); - queue_release_pending_item(item); + LeaveCriticalSection(&queue->cs); + } + + if (queue_release_pending_item(item)) + { + /* This means the callback wasn't run during our wait, so we can invoke the + * callback with a canceled status, and release the work item. */ + if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) + { + IRtwqAsyncResult_SetStatus(item->result, RTWQ_E_OPERATION_CANCELLED); + invoke_async_callback(item->result); + } + IUnknown_Release(&item->IUnknown_iface); + } + IUnknown_Release(&item->IUnknown_iface); hr = S_OK; break; } } - LeaveCriticalSection(&queue->cs); return hr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4243
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=144021 Your paranoid android. === w7u_2qxl (32 bit report) === mfplat: mfplat.c:3809: Test failed: Unexpected refcount 2. === w7u_el (32 bit report) === mfplat: mfplat.c:3809: Test failed: Unexpected refcount 2. === w8 (32 bit report) === mfplat: mfplat.c:3809: Test failed: Unexpected refcount 2. === w8adm (32 bit report) === mfplat: mfplat.c:3809: Test failed: Unexpected refcount 2. === w864 (32 bit report) === mfplat: mfplat.c:3809: Test failed: Unexpected refcount 2. === w7pro64 (64 bit report) === mfplat: mfplat.c:3809: Test failed: Unexpected refcount 2. === w864 (64 bit report) === mfplat: mfplat.c:3809: Test failed: Unexpected refcount 2. === debian11b (64 bit WoW report) === d3dx10_34: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011B60B0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011D5590. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011D1940. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011D1B10. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011C3580. d3dx10_35: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001239C40. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 0000000001239E10. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011D3EC0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001239E10. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 0000000001239E10. d3dx10_36: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011C37C0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011B6000. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011B6000. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B5D20. d3dx10_37: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011BFF80. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 0000000001195A10. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011BFF80. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195A10. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011BFEB0. d3dx10_38: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011C37C0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011B6000. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011B6000. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011A4750. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B5D20. d3dx10_39: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001195A50. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011A49E0. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011B6010. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011E3A60. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011E3C30. d3dx10_40: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011C3850. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 0000000001195C00. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011C34C0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195C00. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011BBC10. d3dx10_41: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011BB560. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011B6050. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 0000000001239D30. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011946B0. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 0000000001194880. d3dx10_42: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011BBB50. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011E3BA0. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011E3C60. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195C60. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011D4950. d3dx10_43: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011C37C0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011B6000. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011B6000. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011A4750. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B5D20. mfmediaengine: mfmediaengine.c:1358: Test failed: Unexpected 56% diff mfmediaengine: Timeout mf: mf: Timeout
update: removed unnecessary re-entering of critical sections; removed unnecessary changes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_64596
In that case neither windows or wine blocks.
So that means MFCancelWorkItem() doesn't wait for work items to finish then. And WaitForThreadpoolWaitCallbacks() waits for work items to finish. So there is a difference. And please add these tests as well. Also, you said wine doesn't block, is that TRUE after your MR? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_64597
On Wed Mar 13 15:06:15 2024 +0000, Zhiyi Zhang wrote:
In that case neither windows or wine blocks. So that means MFCancelWorkItem() doesn't wait for work items to finish then. And WaitForThreadpoolWaitCallbacks() waits for work items to finish. So there is a difference. And please add these tests as well. Also, you said wine doesn't block, is that TRUE after your MR? (copying from IRC)
Also, you said wine doesn't block, is that TRUE after your MR?
yes, it is still true. because `scheduled_item_cancelable_callback` removes the item from the list of items, so `queue_cancel_item` won't find it, except in race conditions -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_64601
Zhiyi Zhang (@zhiyi) commented about dlls/rtworkq/queue.c:
+ { + /* This means the callback wasn't run during our wait, so we can invoke the + * callback with a canceled status, and release the work item. */ + if ((key & WAIT_ITEM_KEY_MASK) == WAIT_ITEM_KEY_MASK) + { + IRtwqAsyncResult_SetStatus(item->result, RTWQ_E_OPERATION_CANCELLED); + invoke_async_callback(item->result); + } + IUnknown_Release(&item->IUnknown_iface); + } + IUnknown_Release(&item->IUnknown_iface); hr = S_OK; break; } } - LeaveCriticalSection(&queue->cs); If key is not found, queue->cs is not released.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_64606
participants (4)
-
Marvin -
Yuxuan Shui -
Yuxuan Shui (＠yshui)
-
Zhiyi Zhang (＠zhiyi)