[PATCH v2 0/2] 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.

--
  v2: rtworkq: Wait for in-flight finalization callbacks on queue shutdown.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/11605

[PATCH v2 1/2] 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 v2 2/2] rtworkq: Wait for in-flight finalization callbacks on queue shutdown.

[Mingze Feng]

From: Mingze Feng <[e-mail]>

A serial work queue can be destroyed while a finalization callback of one of its work items is still running on the target thread pool. The callback unconditionally enters the serial queue's critical section, so deleting the critical section during shutdown crashes 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().

The target queue is not necessarily shut down together with the serial queue: it can be a system queue that stays alive until RtwqShutdown(), or a user pool queue kept alive by other references (a shared target, an explicit RtwqLockWorkQueue(), etc.). In those cases nothing waits for the finalization callbacks before the serial queue's critical section is destroyed.

Track the number of finalization callbacks in flight per queue. Work items that have a finalization callback increment the counter of the queue owning the callback, and a wrapper around the callback decrements it after the callback has returned, signaling an event when the counter reaches zero. The wrapper is only installed for items that actually have a finalization callback, so plain pool queue items are unaffected. shutdown_queue() waits for the counter to reach zero before deleting the critical section; the event is reset before waiting, as it may already be signaled by an earlier transition of the counter to zero. The wait is done outside the global queues lock, since the finalization callbacks may need that lock themselves.

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

diff --git a/dlls/rtworkq/queue.c b/dlls/rtworkq/queue.c
index aadff4312a7..0750b0a5fb2 100644
--- a/dlls/rtworkq/queue.c
+++ b/dlls/rtworkq/queue.c
@@ -239,11 +239,19 @@ static HRESULT unlock_user_queue(DWORD queue)
     {
         if (--entry->refcount == 0)
         {
+            struct queue *obj = entry->obj;
+
             if (shared_mt_queue == queue) shared_mt_queue = 0;
-            shutdown_queue((struct queue *)entry->obj);
-            free(entry->obj);
+            /* Queue shutdown may have to wait for finalization callbacks running on a target
+               thread pool, so drop the lock before that. The slot can be reused right away,
+               the generation counter protects stale handles. */
             entry->obj = next_free_user_queue;
             next_free_user_queue = entry;
+
+            LeaveCriticalSection(&queues_section);
+            shutdown_queue(obj);
+            free(obj);
+            return S_OK;
         }
         hr = S_OK;
     }
@@ -424,14 +432,16 @@ 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 = 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)
+    {
+        /* Worker pool callback will release one reference. Grab one more to keep object alive when
+           we need 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);
+        /* 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. */
+        env.FinalizationCallback = pool_queue_finalization_callback;
+        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);
@@ -484,6 +494,7 @@ static HRESULT serial_queue_init(const struct queue_desc *desc, struct queue *qu
     queue->target_queue = desc->target_queue;
     lock_user_queue(queue->target_queue);
     queue->finalization_callback = serial_queue_finalization_callback;
+    queue->inflight_event = CreateEventW(NULL, TRUE, FALSE, NULL);
 
     return S_OK;
 }
@@ -654,7 +665,6 @@ 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);
     }
 }
 
@@ -721,11 +731,18 @@ 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);
+    if (queue->inflight_event)
+    {
+        /* 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. The event has
+           to be reset first, as it may already be signaled by an earlier transition of the
+           in-flight counter to zero. */
+        ResetEvent(queue->inflight_event);
+        if (queue->inflight)
+            WaitForSingleObject(queue->inflight_event, INFINITE);
+        CloseHandle(queue->inflight_event);
+        queue->inflight_event = NULL;
+    }
 
     DeleteCriticalSection(&queue->cs);
 
-- 
GitLab

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