[PATCH 0/1] MR8538: server: Release mutexes upon their destruction.

[Tim Clem (＠tclem)]

Fixes a regression with 672c3a561f5.

---

A few applications (Quicken, some Wine Mono tests) hit this server assertion after the 10.11 merge:

```
Assertion failed: (mutex->owner == thread), function abandon_mutexes, file mutex.c, line 244.
```

I'm not sure that this patch is correct, exactly. But the current situation is that `mutex_destroy` frees its mutex's sync object, but leaves it in its owning thread's list of `mutex_sync`s. So later, `abandon_mutexes` will enumerate it, which is a UAF. If the memory got stomped on in the meantime (or zeroed, as macOS' `free` does recently), it will hit the above assertion.

Arguably I suppose this could call `mutex_sync_destroy` instead of the direct `do_release`?

Also should this do a `release_object` on the `struct mutex *` itself?

[PATCH 1/1] server: Release mutexes upon their destruction.

[Tim Clem]

From: Tim Clem

Fixes a regression with 672c3a561f5.
---
 server/mutex.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/server/mutex.c b/server/mutex.c
index 573fcda1083..f92ecb6610e 100644
--- a/server/mutex.c
+++ b/server/mutex.c
@@ -267,7 +267,11 @@ static void mutex_destroy( struct object *obj )
 {
     struct mutex *mutex = (struct mutex *)obj;
     assert( obj->ops == &mutex_ops );
-    if (mutex->sync) release_object( mutex->sync );
+    if (mutex->sync)
+    {
+        if (mutex->sync->count) do_release( mutex->sync, current, mutex->sync->count );
+        release_object( mutex->sync );
+    }
 }

/* create a mutex */

[Rémi Bernon (＠rbernon)]

`mutex_sync_destroy` should release it already, and it's called when the last reference is released. The question might be why is the sync still referenced?

[Tim Clem (＠tclem)]

On Thu Jul 10 16:33:51 2025 +0000, Rémi Bernon wrote:
> `mutex_sync_destroy` should release it already, and it's called when the
> last reference is released. The question might be why is the sync still
> referenced when the object isn't anymore?

The trouble is that mutex_destroy inherently assumes that the calling thread owns the sync object. Consider this flow:

1. There's a static HANDLE to a mutex
2. Thread A calls WaitForSingleObject on that mutex
3. Thread B closes the handle

That will hit mutex_destroy, which will call release_object on the mutex's sync object. And that will hit mutex_sync_destroy, which calls do_release with the current thread. However, since thread B doesn't own the mutex, do_release will hit the STATUS_MUTANT_NOT_OWNED case and do nothing. But release_object will continue and free the mutex_sync, even though it remains in the thread's list of mutexes.

See [this demo program](/uploads/c8f96dcfcf3315cd967cd420652ad596/test.c)

In light of that I'm actually unsure how my patch was fixing anything, since it ought to hit the same situation where `current` is not actually the owner of the mutex.

I'm not sure what the correct behavior is in this case. Probably mutex_destroy should just release the sync regardless of its owner? That seems to have been the old behavior.

[PATCH 0/1] MR8538: server: Release mutexes upon their destruction. - closed

[Tim Clem (＠tclem)]

This merge request was closed by Tim Clem.

[Tim Clem (＠tclem)]

Closing in favor of !8541.