Skip to content

RsEventsService: replace global dispatch barrier with a per-handler one (follow-up to #330) - #335

Open
jolavillette wants to merge 1 commit into
RetroShare:masterfrom
jolavillette:fix/rsevents-per-handler-barrier
Open

jolavillette wants to merge 1 commit into
RetroShare:masterfrom
jolavillette:fix/rsevents-per-handler-barrier

Conversation

@jolavillette

@jolavillette jolavillette commented Jul 19, 2026 •

Copy link
Copy Markdown
Contributor

RsEventsService: replace global dispatch barrier with a per-handler one (follow-up to #330)

To fix the shutdown use-after-free, #330 makes handleEvent() hold a single recursive mutex (mDispatchMtx) for the whole dispatch, and makes unregisterEventsHandler() wait on it. So that mutex is now held while the event callbacks run - and two of those callbacks are synchronous and blocking (the passphrase request and the plugin-confirmation dialog). Two threads then end up waiting on each other:

  1. a background thread requests the PGP passphrase → sendEvent() runs the handler on that thread holding mDispatchMtx, and blocks on the GUI thread via Qt::BlockingQueuedConnection;
  2. the GUI thread destroys any widget → its destructor's unregisterEventsHandler() blocks acquiring mDispatchMtx;
  3. background waits for the GUI thread, GUI waits for the lock it holds → hard hang.

Before #330, step 2 returned immediately, so no cycle existed.

This PR keeps #330's UAF fix but makes the unregister barrier per-handler instead of global, so callbacks run with no events-service lock held again and a blocking handler only ever delays unregister of itself, without adding work to the dispatch path:

  • each registered callback lives in a shared_ptr<Handler> carrying an atomic inFlight counter; handleEvent() snapshots the shared_ptrs (no more std::function copies) and bumps the counter under mHandlerMapMtx, so an unregister either erases a handler before the snapshot or sees it in flight;
  • after each callback the counter is decremented; the condition variable is only touched when an unregister is actually waiting (atomic waiter counter), so the common path takes no extra lock and does no allocation;
  • unregisterEventsHandler(h) erases h, then waits until its counter drops to the number of times the current thread itself is running it (thread-local frame stack), so a callback unregistering itself, or one re-entered through a nested sendEvent(), never waits on itself.

Exception-safe (not-yet-run handlers are released if a callback throws). Touches only rseventsservice.{cc,h}.

@jolavillette
jolavillette force-pushed the fix/rsevents-per-handler-barrier branch 5 times, most recently from 81663a3 to 5290449 Compare July 27, 2026 20:05
@jolavillette
jolavillette force-pushed the fix/rsevents-per-handler-barrier branch 6 times, most recently from 71bd272 to e0c4e69 Compare August 6, 2026 23:17
@jolavillette
jolavillette force-pushed the fix/rsevents-per-handler-barrier branch 4 times, most recently from 578bb44 to c5445e1 Compare August 16, 2026 16:26
@G10h4ck

G10h4ck commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

This doesn't look good, AFAIU for every event to handle there are list operations. Handling an event at least prom RsEvent perspective must be something very lightweight...

…ne (follow-up to RetroShare#330)

RetroShare#330 fixed the shutdown use-after-free (a widget's event callback firing
against a dangling `this` -> SIGSEGV in qobject_cast<QThread*> inside
RsQThreadUtils::postToObject) by holding a single recursive mutex across the
whole handleEvent() dispatch, so unregisterEventsHandler() could fence on it.
That barrier is correct for the UAF but couples unrelated handlers and can
deadlock.

Two event handlers are deliberately synchronous and blocking: the passphrase
request (rsserver/rsloginhandler.cc) and the plugin-confirmation dialog
(plugins/pluginmanager.cc). They are delivered via sendEvent(), so
handleEvent() runs on the caller's thread, and their GUI handler blocks that
caller with Qt::BlockingQueuedConnection until the GUI thread answers a
modal. With one global dispatch lock:

  1. a background thread requests the passphrase -> holds the dispatch lock
     -> blocks waiting on the GUI thread;
  2. the GUI thread destroys any unrelated widget -> its destructor calls
     unregisterEventsHandler() -> blocks acquiring the dispatch lock;
  3. background waits for GUI, GUI waits for the lock -> hard hang.

Replace the global barrier with a per-handler one, keeping the dispatch
path as cheap as before RetroShare#330:

  - each registered callback lives in a shared_ptr<Handler> carrying an
    atomic inFlight counter; handleEvent() snapshots the shared_ptrs (one
    refcount increment each, no std::function copy anymore) and increments
    inFlight under mHandlerMapMtx, so an unregister either erases a handler
    before the snapshot or sees it in flight;
  - after each callback, inFlight is decremented; the condition variable is
    only touched when an unregister is actually waiting (atomic waiter
    counter), so the common path takes no extra lock and does no allocation;
  - unregisterEventsHandler() erases the handler then waits until inFlight
    drops to the number of times this same thread is running it (tracked by
    a thread-local frame stack), so a callback unregistering itself, or one
    re-entered via a nested sendEvent(), never waits on itself.

Callbacks run with no events-service lock held again, so a slow or blocking
handler only delays unregister of that same handler, never of an unrelated
widget torn down on another thread. If a callback throws, the not-yet-run
handlers are released before the exception propagates, so a later
unregister cannot block forever.

Touches only src/services/rseventsservice.{cc,h}.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@jolavillette
jolavillette force-pushed the fix/rsevents-per-handler-barrier branch from c5445e1 to bf1b23d Compare September 19, 2026 03:21
@jolavillette

Copy link
Copy Markdown
Contributor Author

Fair point, the map/multiset bookkeeping was too much for the hot path. I reworked the branch (force-pushed, rebased on master):

  • each registered callback now lives in a shared_ptr<Handler> with an atomic inFlight counter; handleEvent() snapshots the shared_ptrs (one refcount bump each, it no longer copies the std::functions as before) and bumps the counter under mHandlerMapMtx;
  • after each callback the counter is decremented; the condition variable is only touched when an unregister is actually waiting (atomic waiter counter), so the normal dispatch path takes no extra lock and does no allocation;
  • unregisterEventsHandler() erases the handler then waits for its counter, minus the times the current thread itself is running it (thread-local frame stack), so a callback unregistering itself never waits on itself.

Net effect: dispatch is cheaper than current master (one vector reserve per event instead of a list of std::function copies), and callbacks still run with no service lock held so the blocking passphrase / plugin-confirmation handlers can't deadlock a widget teardown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants