[PATCH 0/1] MR8538: server: Release mutexes upon their destruction.
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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8538
From: Tim Clem <tclem(a)codeweavers.com> 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 */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8538
`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? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8538#note_109444
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8538#note_109479
This merge request was closed by Tim Clem. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8538
Closing in favor of !8541. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8538#note_109509
participants (3)
-
Rémi Bernon -
Tim Clem
-
Tim Clem (＠tclem)