IPC stability bugs

Bug 77

Disconnect handler can still run after its connection was destroyed locally

Status
Open, fix under review
Symptom
Crash; worst effect seen: test failure only
Where seen
Libmultiprocess tests (mptest)
Trigger
The process closes its own connection while the peer is still there
First reported
2026-08-20 (first report)
Fixed
Not yet
Fix
mp#361
In released binaries
Yes
Defect ever on master
Yes

Summary

A Connection's onDisconnect handler could run after the Connection had been destroyed locally, if the peer disconnected and the local side destroyed the connection in adjacent event-loop turns. With the handlers in use this deletes the Connection a second time or uses a dangling pointer. It was found in code review and shown with a constructed mptest case; it has never been observed in practice. Bitcoin Core releases v22 through v31.1 contain the code, and it could possibly be reached when a client disconnects in the same loop turn that Ipc::disconnectIncoming() destroys its connection at shutdown. A fix is in mp#361, which fixes several disconnect bugs found in review, not yet merged.

What happened

Root cause

Connection::onDisconnect (include/mp/proxy-io.h) added the handler to a per-connection kj::TaskSet m_on_disconnect, so destroying the Connection would cancel it. But when the m_network.onDisconnect() promise resolved, the continuation moved the user's function onto the shared m_loop->m_task_set with kj::evalLater. That hop exists to avoid a "Promise callback destroyed itself" error when a handler deletes its own Connection (mp#7, bug page). Once re-posted, the function was no longer owned by anything the Connection destructor tears down, so if the Connection was destroyed locally in the turn between the two steps, the function still ran.

In master's handlers, _Serve erases the connection from m_incoming_connections and ConnectStream deletes the client Connection, so a late run deletes an already deleted object. On the incoming side this needs a local teardown (such as Ipc::disconnectIncoming()) of a connection whose peer has just gone away; on the outgoing side, the top-level client proxy with destroy_connection being destroyed at the same moment. Bitcoin Core's shutdown destroys connections whose peers are usually still connected, which is most likely why it has not been seen.

Fix

No fix merged yet. 7cbade8b18a8 in mp#361 (co-authored by enirox001 and Claude) gives each Connection a std::shared_ptr<void> m_alive token. Handlers are added directly to the event-loop task set, capture a weak_ptr to the token, and run only if it is still alive. Because m_network.onDisconnect() also fires on local destruction (resetting m_rpc_system drops capnp's last reference to the network), the expired token is what tells the local case apart, which lets the per-connection TaskSet, its error handler and the evalLater hop be removed. mp#336, an open PR that reference-counts Connection objects, would later replace the token with shared_ptr ownership of Connection itself.

Next steps

Timeline

Lessons

What would have prevented this bug, found it sooner, or made it easier to debug. The patterns page groups these lessons across all bugs.

Pattern An object outliving what it points to: A callback outlives what it captured

  • Prevented by a clear ownership model, a review checklist item
  • Earliest detector a disconnect test (did not exist then)
  • Diagnosis moderate
  • Fix design moderate
  • Debugging aids a reproduction script
  • Rust yes

Would Rust have prevented it? Yes. A future queued on the loop's task set must be 'static, so the handler could not capture a bare reference to the Connection; it would hold a Weak (the fix) or an Rc, and an Rc would keep the Connection alive one more turn until the handler dropped it, a delay rather than a hang here. More on Rust

Could it have been prevented?

Probably. The rule is that a callback which touches an object must stay owned by, or gated on, that object's lifetime for its whole life, including after it changes queues. c685fa9bedaf (mp#7) put handlers in a per-Connection kj::TaskSet for exactly this reason, then re-posted each fired handler to the shared loop task set with kj::evalLater to dodge a KJ "promise callback destroyed itself" error, and at that moment the handler was owned by nothing the Connection destructor tears down. The commit message claimed "onDisconnect events are now cancelled as Connection objects are destroyed", which held only until the event fired. A review question, "after this hop, what still cancels it?", would have caught the gap, but mp#7 had no reviewers. An ownership model where the handler holds a weak token (the fix in 7cbade8b18a8) or a shared reference to the Connection (mp#336) makes the question answer itself.

Could it have been found sooner?

Yes, by a test that lets the peer disconnect and destroys the Connection locally from a task queued between the disconnect event and the handler, which is the test enirox001 wrote in a day once he suspected it. Nothing like it existed in seven years: the library's disconnect tests destroy connections whose peers are still there or let the peer go and wait, and Bitcoin Core's shutdown does the same, so the two events never landed in adjacent loop turns. ASan would make the double delete fatal but only on that interleaving. This is a bug found by reading, prompted by mp#335 proposing to keep a Connection alive after disconnect(), which made a reviewer ask what a still-queued handler would do to it.

What made it hard to debug?

There was nothing to debug in the usual sense, but the diagnosis took two rounds. ryanofsky's first reading of enirox001's report was a double delete and he asked for detail; the refined scenario came with the failing test. Designing the fix needed one fact that is easy to miss: m_network.onDisconnect() also fires on local destruction, because resetting m_rpc_system drops capnp's last reference to the network, so an expired token is what tells the local case apart, and with that check in place both the per-connection TaskSet and the evalLater hop could go. The fix is 33 lines in one file and was reviewed alongside two other pre-existing bugs in mp#361; the PR is still open.

Lesson

When a workaround moves a callback to a different queue or owner, the cancellation guarantee it had before does not move with it; re-check who cancels it after the move, and prefer a liveness check inside the callback over relying on which queue holds it.

Notes

Classification

The survey's classification of this bug. The explorer charts these fields across all bugs; the method page explains them.

Field Value
Symptom crash
Worst effect observed test-failure-only
Seen in mptest
Scope production
Trigger Process closes its own connection (local-disconnect)
Phase shutdown
Side both
Component connection
Root cause kind lifetime
Fix kind local-fix
Detection review
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released binary
Confidence high