[PATCH 0/1] MR11605: rtworkq: Wait for in-flight finalization callbacks on queue shutdown.

[Mingze Feng (＠Zeeze)]

## Summary

`shutdown_queue()` in `dlls/rtworkq/queue.c` can call `DeleteCriticalSection(&queue->cs)` while a finalization callback is still executing in the thread pool. The callback then calls `EnterCriticalSection()` on the deleted critical section and the process crashes with `STATUS_INVALID_HANDLE` (0xc0000008).

## Evidence

Observed while running a Unity 2021.3 game (Media Foundation video pipeline) under Proton (GE-Proton11-3). The game crashes intermittently during normal gameplay; video playback itself usually succeeds.

Seven collected minidumps were parsed (ExceptionStream):

- Exception code `0xc0000008` (`STATUS_INVALID_HANDLE`) in 6 dumps, `0xc0000022` in 1
- The crashing thread differs every time
- The crash offset is **identical in all 7 dumps**: inside `serial_queue_finalization_callback()`, at the `EnterCriticalSection(&queue->cs)` call at the top of the function

## Analysis

When a serial work queue is destroyed (last handle released via `unlock_user_queue()` -> `shutdown_queue()`), `pool_queue_shutdown()` calls `CloseThreadpoolCleanupGroupMembers()` and then `shutdown_queue()` deletes the queue's critical section. However, a work item dispatched to the target thread pool may have completed its main callback while its finalization callback (`serial_queue_finalization_callback`) is still running (or about to run) on a pool thread. That callback unconditionally enters the (now deleted) serial queue's critical section.

!5158 ("rtworkq: Avoid closing a thread pool object while its callbacks are running") fixed a related race for the cancel path (`queue_cancel_item()` waits for thread pool callbacks), but the shutdown path remains unprotected.

## Fix

Track in-flight finalization callbacks per queue: `pool_queue_submit()` increments the counter of the queue owning the finalization callback (`item->queue`; for serial queues this is the serial queue whose critical section the callback will enter), and a wrapper (`pool_queue_finalization_callback`) decrements it after the callback has returned and signals an event when the counter reaches zero. `shutdown_queue()` waits for the counter to reach zero before `DeleteCriticalSection()`.

The wrapper snapshots `item->queue` before calling the finalization callback, because that callback may release the last reference to the work item.

The fix was verified against the affected game: crashes stopped after deploying a build with this change, and normal video playback / queue progression is unaffected since the hot path only gains two atomic operations per submitted work item.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11605

[PATCH 1/1] rtworkq: Wait for in-flight finalization callbacks on queue shutdown.

[Mingze Feng]

From: Mingze Feng <[e-mail]>

A finalization callback may still be executing when shutdown_queue() runs:
CloseThreadpoolCleanupGroupMembers() does not guarantee that finalization
callbacks have completed. Such a callback then calls EnterCriticalSection()
on a critical section that shutdown_queue() has just deleted, crashing with
STATUS_INVALID_HANDLE (0xc0000008). This was observed with a Unity game
using the Media Foundation video pipeline, where minidumps from repeated
crashes all pointed at the same offset in serial_queue_finalization_callback().

Track the number of finalization callbacks in flight per queue. Each work
item submitted to a thread pool increments the counter of the queue owning
the finalization callback, and a wrapper around the callback decrements it
when the callback has returned. shutdown_queue() then waits for the counter
to reach zero before deleting the critical section.

Signed-off-by: Mingze Feng <[e-mail]>
---
 dlls/rtworkq/queue.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c
index 144feb98c55..aadff4312a7 100644
--- a/dlls/rtworkq/queue.c
+++ b/dlls/rtworkq/queue.c
@@ -198,6 +198,10 @@ struct queue
     /* Data used for serial queues only. */
     PTP_SIMPLE_CALLBACK finalization_callback;
     DWORD target_queue;
+    /* Number of finalization callbacks that are still in flight. Shutdown has to wait
+     * for them to finish before the critical section can be deleted. */
+    LONG inflight;
+    HANDLE inflight_event;
 };
 
 static void shutdown_queue(struct queue *queue);
@@ -393,6 +397,20 @@ static void CALLBACK standard_queue_worker(TP_CALLBACK_INSTANCE *instance, void
     IUnknown_Release(&item->IUnknown_iface);
 }
 
+static void CALLBACK pool_queue_finalization_callback(PTP_CALLBACK_INSTANCE instance, void *context)
+{
+    struct work_item *item = context;
+    /* The finalization callback below may release the last reference to the item,
+     * so snapshot the queue before calling it. */
+    struct queue *queue = item->queue;
+
+    if (item->finalization_callback)
+        item->finalization_callback(instance, context);
+
+    if (!InterlockedDecrement(&queue->inflight) && queue->inflight_event)
+        SetEvent(queue->inflight_event);
+}
+
 static void pool_queue_submit(struct queue *queue, struct work_item *item)
 {
     TP_CALLBACK_PRIORITY callback_priority;
@@ -406,11 +424,14 @@ static void pool_queue_submit(struct queue *queue, struct work_item *item)
         callback_priority = TP_CALLBACK_PRIORITY_HIGH;
 
     env = queue->envs[callback_priority];
-    env.FinalizationCallback = item->finalization_callback;
+    env.FinalizationCallback = pool_queue_finalization_callback;
     /* Worker pool callback will release one reference. Grab one more to keep object alive when
        we need finalization callback. */
     if (item->finalization_callback)
         IUnknown_AddRef(&item->IUnknown_iface);
+    /* Count the finalization callback as in flight, so that queue shutdown can wait
+     * for it before deleting the critical section the callback is going to enter. */
+    InterlockedIncrement(&item->queue->inflight);
     item->u.work_object = CreateThreadpoolWork(standard_queue_worker, item, (TP_CALLBACK_ENVIRON *)&env);
     item->type = WORK_ITEM_WORK;
     SubmitThreadpoolWork(item->u.work_object);
@@ -633,6 +654,7 @@ static void init_work_queue(const struct queue_desc *desc, struct queue *queue)
     {
         list_init(&queue->pending_items);
         InitializeCriticalSection(&queue->cs);
+        queue->inflight_event = CreateEventW(NULL, TRUE, FALSE, NULL);
     }
 }
 
@@ -699,6 +721,12 @@ static void shutdown_queue(struct queue *queue)
     }
     LeaveCriticalSection(&queue->cs);
 
+    /* A finalization callback could still be executing at this point, waiting for it to
+     * complete before deleting the critical section it is going to enter. */
+    if (queue->inflight)
+        WaitForSingleObject(queue->inflight_event, INFINITE);
+    CloseHandle(queue->inflight_event);
+
     DeleteCriticalSection(&queue->cs);
 
     memset(queue, 0, sizeof(*queue));
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/11605

[PATCH 0/1] MR11605: rtworkq: Wait for in-flight finalization callbacks on queue shutdown. - closed

[Mingze Feng (＠Zeeze)]

This merge request was closed by Mingze Feng.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11605

[Mingze Feng (＠Zeeze)]

Closing this MR in favor of a new one opened from the fork's master branch after CI validation. The fix has been reworked: the finalization-callback wrapper is now only installed for work items that actually have a finalization callback (fixing a use-after-free for plain pool items), the wait event is reset before waiting (fixing a stale-signal race), the wait was moved out of the global queues lock, and the root-cause analysis was corrected (CloseThreadpoolCleanupGroupMembers in Wine does wait for finalization callbacks; the race is with target queues that outlive the serial queue).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11605#note_148239