-- v6: rtworkq: Release cancelled work items. mfplat: tests: Validate MFCancelWorkItem releases the callback. rtworkq: Avoid use-after-free.
From: Yuxuan Shui yshui@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; }
From: Yuxuan Shui yshui@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);
From: Yuxuan Shui yshui@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/rtworkq/queue.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
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; }
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=143736
Your paranoid android.
=== debian11 (32 bit report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11 (32 bit ar:MA report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11 (32 bit de report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11 (32 bit fr report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11 (32 bit he:IL report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11 (32 bit hi:IN report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11 (32 bit ja:JP report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11 (32 bit zh:CN report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11b (32 bit WoW report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
=== debian11b (64 bit WoW report) ===
mfplat: mfplat.c:3615: Test succeeded inside todo block: Unexpected refcount 0.
@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.
-
Marvin -
Yuxuan Shui -
Yuxuan Shui (＠yshui)