[PATCH v7 0/3] MR4243: rtworkq: free cancelled work items
-- v7: 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 | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 6897036e9dd..363c240d13d 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -3598,6 +3598,7 @@ static void test_scheduled_items(void) IMFAsyncResult *result; MFWORKITEM_KEY key, key2; HRESULT hr; + ULONG refcount; callback = create_test_callback(NULL); @@ -3610,6 +3611,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 +3623,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> Usually the threadpool holds a reference to the work_item, which is released when the work_item's callback is invoked (see `scheduled_item_cancelable_callback`). However if an item is cancelled by `queue_cancel_item`, that reference is not released. This commit fixes this leak in `queue_cancel_item`, and also makes sure this reference is not double released if the callback happens to be running while `queue_cancel_item` is called. --- dlls/mfplat/tests/mfplat.c | 2 +- dlls/rtworkq/queue.c | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/mfplat/tests/mfplat.c b/dlls/mfplat/tests/mfplat.c index 363c240d13d..1e751b425fa 100644 --- a/dlls/mfplat/tests/mfplat.c +++ b/dlls/mfplat/tests/mfplat.c @@ -3612,7 +3612,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); diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c index 00b77bf6953..97d1cdc9f12 100644 --- a/dlls/rtworkq/queue.c +++ b/dlls/rtworkq/queue.c @@ -792,9 +792,10 @@ static void CALLBACK scheduled_item_cancelable_callback(TP_CALLBACK_INSTANCE *in TRACE("result object %p.\n", item->result); if (queue_release_pending_item(item)) + { invoke_async_callback(item->result); - - IUnknown_Release(&item->IUnknown_iface); + IUnknown_Release(&item->IUnknown_iface); + } } static void CALLBACK periodic_item_callback(TP_CALLBACK_INSTANCE *instance, void *context, TP_TIMER *timer) @@ -907,6 +908,7 @@ static HRESULT queue_cancel_item(struct queue *queue, RTWQWORKITEM_KEY key) else WARN("Unknown item key mask %#I64x.\n", key); queue_release_pending_item(item); + IUnknown_Release(&item->IUnknown_iface); hr = S_OK; break; } -- 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=143737 Your paranoid android. === debian11b (64 bit WoW report) === mfplat: Unhandled exception: page fault on execute access to 0x00000000011b7070 in 64-bit code (0x000000011b7070).
On Wed Mar 6 06:18:22 2024 +0000, Yuxuan Shui wrote:
@zhiyi's fix for the race in !5158 is much much nicer than mine. I updated this MR to build on top of that and also fix the memory leak. hopefully it's clearer now. Hmm, i think the clearer version does have a use-after-free problem. If the item is freed in `queue_cancel_item` while `scheduled_item_cancelable_callback`/`waiting_item_cancelable_callback` is running, the latter will use-after-free.
I am afraid my original version is still the only way to do this. @zhiyi do you have thoughts? since you have worked on this part of code just recently? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_63641
On Wed Mar 6 06:18:22 2024 +0000, Yuxuan Shui wrote:
Hmm, i think the clearer version does have a use-after-free problem. If the item is freed in `queue_cancel_item` while `scheduled_item_cancelable_callback`/`waiting_item_cancelable_callback` is running, the latter will use-after-free. I am afraid my original version is still the only way to do this. @zhiyi do you have thoughts? since you have worked on this part of code just recently? ~~Or maybe we can put the whole callback into critical section~~ I don't think this would help.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4243#note_63642
participants (3)
-
Marvin -
Yuxuan Shui -
Yuxuan Shui (＠yshui)