[PATCH v18 0/3] MR4243: rtworkq: free cancelled work items
-- v18: 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 | 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/mf/tests/mf.c | 1 - dlls/mfplat/tests/mfplat.c | 4 ++-- dlls/rtworkq/queue.c | 48 +++++++++++++++++++++++++++++++------- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index c05482778df..4353cd61477 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6368,7 +6368,6 @@ if (SUCCEEDED(hr)) check_sar_rate_support(sink); ref = IMFMediaSink_Release(sink); - todo_wine ok(ref == 0, "Release returned %ld\n", ref); /* Activation */ diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 8298b140897..19a975ff9e2 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 || broken(context.refcount == 2) /* Win 7/8 */, "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..9046a70b359 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -883,7 +883,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,29 +892,58 @@ 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); - hr = S_OK; - break; + 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); + return S_OK; } } LeaveCriticalSection(&queue->cs); - return hr; + return RTWQ_E_NOT_FOUND; } static HRESULT alloc_user_queue(const struct queue_desc *desc, DWORD *queue_id) -- 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=144060 Your paranoid android. === debian11 (32 bit he:IL report) === mf: mf.c:8071: Test failed: Unexpected hr 0. mf.c:8075: Test failed: Unexpected hr 0. === debian11b (64 bit WoW report) === d3dx10_34: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001239E90. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011E3550. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011E3720. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011E3720. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B5D90. d3dx10_35: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011BB9A0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011A4920. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011A4920. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011E3A30. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011E3C00. d3dx10_36: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011D57F0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011A4A20. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011E39B0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011D5720. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B5D90. d3dx10_37: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001239D90. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 0000000001195BC0. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011A4A10. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195BC0. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011C34B0. d3dx10_38: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001230D00. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011D5810. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011D59E0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011958A0. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 0000000001195A70. d3dx10_39: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011B24E0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011D5820. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011D5820. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011B1E90. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B2060. d3dx10_40: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011BB840. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011BB560. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011B6010. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011BB560. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011A49E0. d3dx10_41: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001239DE0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011B6070. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011B6070. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011B5D90. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B1E90. d3dx10_42: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011B6120. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011BBE60. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011BBE60. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011C36A0. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011E3760. d3dx10_43: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001230C20. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 0000000001230C20. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 0000000001239E00. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011B61A0. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 0000000001239E00.
participants (3)
-
Marvin -
Yuxuan Shui -
Yuxuan Shui (＠yshui)