RsEventsService: make unregisterEventsHandler() a barrier (fix shutdown UAF)#330
Open
jolavillette wants to merge 1 commit into
Open
Conversation
…wn UAF) handleEvent() copies the registered callbacks under mHandlerMapMtx and then runs them *outside* the lock (on purpose, so a callback may re-enter to send events or unregister itself). As a side effect unregisterEventsHandler() was not a barrier: it removed the handler from the map, but an in-flight dispatch still held a private copy of it. If the handler's owner (a GUI widget) was destroyed on another thread in that window, the copy fired against a dangling object -> use-after-free, typically SIGSEGV in qobject_cast<QThread*> inside RsQThreadUtils::postToObject. This is what tears the process down at shutdown, when many widgets are destroyed while the events thread keeps ticking a flood of peer-disconnect events (the rest of the crash dump is just exit()-in-the- signal-handler unwinding RsServer while other threads are still running). Add a recursive mDispatchMtx held by handleEvent() across the whole dispatch loop. unregisterEventsHandler() erases the handler under mHandlerMapMtx (no future dispatch can pick it up) and then fences on mDispatchMtx (no ongoing dispatch is still running it), so once it returns the handler is guaranteed neither running nor about to start. Widgets that unregister from their destructor are then safe to destroy. Recursive so a callback re-entering on the dispatching thread (self-unregister / synchronous sendEvent) does not deadlock; safe because handlers post asynchronously (Qt QueuedConnection) and never block on the unregistering thread, so there is no lock-order cycle. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RsEventsService: make unregisterEventsHandler() a barrier (fix shutdown UAF)
handleEvent() copies the registered callbacks under mHandlerMapMtx and then runs them outside the lock (on purpose, so a callback may re-enter to send events or unregister itself). As a side effect unregisterEventsHandler() was not a barrier: it removed the handler from the map, but an in-flight dispatch still held a private copy of it. If the handler's owner (a GUI widget) was destroyed on another thread in that window, the copy fired against a dangling object -> use-after-free, typically SIGSEGV in qobject_cast<QThread*> inside RsQThreadUtils::postToObject. This is what tears the process down at shutdown, when many widgets are destroyed while the events thread keeps ticking a flood of peer-disconnect events (the rest of the crash dump is just exit()-in-the- signal-handler unwinding RsServer while other threads are still running).
Add a recursive mDispatchMtx held by handleEvent() across the whole dispatch loop. unregisterEventsHandler() erases the handler under mHandlerMapMtx (no future dispatch can pick it up) and then fences on mDispatchMtx (no ongoing dispatch is still running it), so once it returns the handler is guaranteed neither running nor about to start. Widgets that unregister from their destructor are then safe to destroy. Recursive so a callback re-entering on the dispatching thread (self-unregister / synchronous sendEvent) does not deadlock; safe because handlers post asynchronously (Qt QueuedConnection) and never block on the unregistering thread, so there is no lock-order cycle.