[PATCH v19 0/3] MR4243: rtworkq: free cancelled work items
-- v19: 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 | 5 ++-- dlls/rtworkq/queue.c | 48 +++++++++++++++++++++++++++++++------- 3 files changed, 42 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..8601789be78 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,8 @@ 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); + Sleep(500); + 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=144061 Your paranoid android. === w7u_el (32 bit report) === mf: mf: Timeout === debian11b (64 bit WoW report) === d3dx10_34: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011BB900. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011B5D20. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011BB6A0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011B5D20. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011D18E0. d3dx10_35: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001239C80. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011BCAB0. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 0000000001239E50. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011E3400. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B5D90. d3dx10_36: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001195A50. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011D56F0. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011B60E0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195A50. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011B6010. d3dx10_37: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011E3AD0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011E3AD0. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011BE6C0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011BBC10. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011BE6C0. d3dx10_38: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011BBA60. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011BB950. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011BBA10. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195B10. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 0000000001195B10. d3dx10_39: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011A4B30. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011E3D80. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011E3D80. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 00000000011D4980. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011E3D80. d3dx10_40: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001239D80. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 0000000001239D80. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011936A0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001193870. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 0000000001193870. d3dx10_41: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011E3AA0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011A4A80. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011E37C0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195B10. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011E37C0. d3dx10_42: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 00000000011E3AD0. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 00000000011A4A80. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011E37F0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001195B10. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 00000000011E37F0. d3dx10_43: d3dx10.c:4380: Test succeeded inside todo block: Got unexpected effect 0000000001239D80. d3dx10.c:4470: Test succeeded inside todo block: Got unexpected effect 0000000001239D80. d3dx10.c:4480: Test succeeded inside todo block: Got unexpected effect 00000000011936A0. d3dx10.c:4589: Test succeeded inside todo block: Got unexpected effect 0000000001193870. d3dx10.c:4599: Test succeeded inside todo block: Got unexpected effect 0000000001193870.
Ah, no, the callback is invoked as another work item! We can't just wait for the tp object in `work_item`, we also need to wait for the `invoke_async_result` tp object. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_64740
On Thu Mar 14 14:38:17 2024 +0000, Yuxuan Shui wrote:
Ah, no, the callback is invoked as another work item! We can't just wait for the tp object in `work_item`, we also need to wait for the `invoke_async_result` tp object. OK, this is getting too complex, too many work_items to chase down. I am going with the other option.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_64741
On Thu Mar 14 14:41:12 2024 +0000, Yuxuan Shui wrote:
~~OK, this is getting too complex, too many work_items to chase down. I am going with the other option.~~ On second thought, this is completely fine. The work item created by `invoke_async_result` is not the one we free in `queue_cancel_item`. This also means `MFCancelWorkItem` is just not going to wait for the user callback _at all_. It waits for `scheduled_item_callback`/`periodic_item_callback`, etc., but those callbacks _**do not**_ call user callback directly.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_64745
@zhiyi anything more i need to do to get this merged? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_65394
Zhiyi Zhang (@zhiyi) commented about dlls/mfplat/tests/mfplat.c:
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); + Sleep(500);
These added Sleep() and broken() should be in patch 2/3. Otherwise, I think 2/3 is going to have failures on win7/8. You need to keep each patch passing the tests. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_65960
participants (4)
-
Marvin -
Yuxuan Shui -
Yuxuan Shui (＠yshui)
-
Zhiyi Zhang (＠zhiyi)