Skip to content

host dsig: don't let dsig_unsub() return while its callback can still run - #66

Merged
andoma merged 2 commits into
masterfrom
dsig-unsub-inflight-race
Oct 1, 2026
Merged

andoma merged 2 commits into
masterfrom
dsig-unsub-inflight-race

Conversation

@kapouchima

@kapouchima kapouchima commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

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 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) leaves
the 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 then
dereferenced 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

  1. test/dsig: a new host test suite that fails on current master. The
    cases 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:

    case master this PR
    unsub_waits_for_running_callback FAIL PASS
    unsubbed_callback_not_called_after_unsub (the crash) FAIL PASS
    unsub_waits_for_running_expiry (TTL path) FAIL PASS
    unsub_from_own_callback (guards against deadlocking) PASS PASS
    unsub_and_free_under_traffic FAIL, ASan heap-use-after-free PASS
  2. The fix: 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, 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 (nested dsig_input()
    included), and those are not waited for. Both dispatch paths share the
    snapshot code, which also drops bus_thread()'s unchecked realloc()s.

The new contract is documented in dsig.h: once dsig_unsub() returns, the
callback 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

make -C test/dsig run     # 0 failed (4 failed on master)
make -C test/dsig asan    # clean (heap-use-after-free on master)
make -C test/vllp run-sim # 0 failures, unchanged

Run on macOS (clang, plus TSan) and on Linux (Ubuntu 24.04, gcc 14.2, aarch64).
test/vllp only builds on Linux (pthread_condattr_setclock), unrelated to
this change.

Co-Authored-By: Claude noreply@anthropic.com

kapouchima and others added 2 commits October 1, 2026 10:23
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
@andoma
andoma merged commit c09e262 into master Oct 1, 2026
1 check passed
@andoma
andoma deleted the dsig-unsub-inflight-race branch October 1, 2026 09:24
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