host dsig: don't let dsig_unsub() return while its callback can still run - #66
Merged
Merged
Conversation
dsig_input() and bus_thread()'s TTL expiry copy (cb, opaque) under the
bus lock and call them after unlocking. dsig_unsub() only unlinks and
frees, so it can return while a callback is running, or before a
callback already snapshotted for delivery gets called. A caller that
then frees the opaque leaves the dispatching thread calling into freed
memory.
That crashed tissuescanner: two subscribers on one signal, dsig_input()
snapshotted both and ran the first, slow one while another thread
unsubscribed the second and freed its std::function; the second call
then dereferenced freed memory.
The handshake-driven cases fail on every run rather than by timing
luck; the stress case is the field crash as it happens, and under
'make asan' reports the use-after-free directly:
- unsub_waits_for_running_callback (fails)
- unsubbed_callback_not_called_after_unsub (fails, the crash)
- unsub_waits_for_running_expiry (fails)
- unsub_from_own_callback (passes; guards the fix
against deadlocking)
- unsub_and_free_under_traffic (fails; heap-use-after-free
under ASan)
make -C test/dsig run
make -C test/dsig asan
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WJ8d5M17KS9DxRaSehUwHx
… run dsig_input() and bus_thread()'s TTL expiry copied (cb, opaque) under the bus lock and called them after unlocking, while dsig_unsub() only unlinked and freed. So dsig_unsub() could return with the callback running on another thread, or with a call still queued in a snapshot, and a caller freeing the opaque afterwards left that thread calling into freed memory. This crashed tissuescanner: dsig_input() was running a slow subscriber's callback while another thread unsubscribed the next subscriber on the same signal and freed its std::function. Snapshots now hold a reference to each dsig_sub_t instead of a copy of (cb, opaque), and re-check 'dead' under the lock right before each call. dsig_unsub() marks the sub dead, waits for calls in progress on other threads to return, and leaves the free to the last snapshot still referencing it. A callback that unsubscribes itself is not waited for; a thread-local stack of the calls in progress tells dsig_unsub() which ones are its own, nested dsig_input() included. Both dispatch paths share the snapshot code, which also drops bus_thread()'s unchecked realloc()s. make -C test/dsig run # all pass, previously 4 failures make -C test/vllp run-sim # unchanged, 0 failures Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJ8d5M17KS9DxRaSehUwHx
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
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.
Problem
dsig_input()andbus_thread()'s TTL expiry copy(cb, opaque)under thebus lock and call them after unlocking.
dsig_unsub()only unlinks and frees,so it can return while the callback is running on another thread, or before a
callback that was already snapshotted for delivery gets called. A caller that
frees the opaque after
dsig_unsub()returns (the natural thing to do) leavesthe dispatching thread calling into freed memory.
This crashed a production device. Two subscribers on one signal:
dsig_input()snapshotted both and ran the first, slow one, while another thread
unsubscribed the second and freed its
std::function. The second call thendereferenced freed memory (SIGSEGV; the core shows glibc's safe-linking marker
in the callback's storage, and the unsubscribing thread just past the free).
Commits
test/dsig: a new host test suite that fails on current master. Thecases are driven by explicit handshakes, so they fail on every run rather
than by timing luck, plus one stress case that is the field crash as it
happens:
unsub_waits_for_running_callbackunsubbed_callback_not_called_after_unsub(the crash)unsub_waits_for_running_expiry(TTL path)unsub_from_own_callback(guards against deadlocking)unsub_and_free_under_trafficThe fix: snapshots now hold a reference to each
dsig_sub_tinstead ofa copy of
(cb, opaque), and re-checkdeadunder the lock right beforeeach call.
dsig_unsub()marks the sub dead, waits for calls in progress onother threads, and leaves the free to the last snapshot still referencing
it. A callback may unsubscribe itself: a thread-local stack of the calls in
progress tells
dsig_unsub()which ones are its own (nesteddsig_input()included), and those are not waited for. Both dispatch paths share the
snapshot code, which also drops
bus_thread()'s uncheckedrealloc()s.The new contract is documented in
dsig.h: oncedsig_unsub()returns, thecallback is not running and will not be called again. Because it may now wait,
it must not be called while holding a lock the callback takes.
Testing
Run on macOS (clang, plus TSan) and on Linux (Ubuntu 24.04, gcc 14.2, aarch64).
test/vllponly builds on Linux (pthread_condattr_setclock), unrelated tothis change.
Co-Authored-By: Claude noreply@anthropic.com