IPC stability bugs

Bug 30

Server crashes using a freed connection after a client disconnects mid-call

Status
Fixed
Symptom
Crash; worst effect seen: crash
Where seen
Branch-build, libmultiprocess tests (mptest)
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2024-10-16 (first report)
Fixed
2025-06-19
Fix
mp#118, mp#160, mp#186
Shipped to Bitcoin Core
bitcoin#31105, bitcoin#32345
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

When an IPC client disconnected while a server method it had called was still running, the server's Connection object was destroyed before the ProxyServer object handling the call. Code that later found the event loop through the ProxyServer's m_context.connection pointer read the freed Connection, and bitcoin-node crashed. It was seen three times over eight months: with a mining client killed during a long wait, with a Rust wallet client connecting and disconnecting in a loop, and with bitcoin-cli -ipcconnect run repeatedly against a node. A workaround in mp#118 covered only the first of three code paths, at the cost of a memory leak. The fix came in mp#160, shipped to Bitcoin Core in bitcoin#32345. Source builds with multiprocess enabled before v30.0 have the bug.

What happened

  1. mp#118 first fixed a crash on a stale map iterator in the same scenario (its page). With that fix applied, ryanofsky followed @tdb3's steps (Ctrl-C on the bitcoin-mine client while it is blocked in waitFeesChanged, added by bitcoin#31003, then mine a block) and got a second segfault (2024-10-16): the worker thread finished the call and crashed in EventLoop::post with this=0x0, reached from server.m_context.connection->m_loop.sync(...) in the PassField Context overload. He landed a workaround the same day. A trace tdb3 posted on 2024-10-08 still shows the erase-by-iterator code, so it came from a build without the iterator fix and shows the earlier bug.
  2. @darosior ran a Rust client that connected, called Init.makeChain and Chain.initMessage, and disconnected in a loop against bitcoin#32345 ("ipc: Handle unclean shutdowns better", then open) plus bitcoin#29409 ("multiprocess: Add capnp wrapper for Chain interface"). ryanofsky filed it as mp#182 ("IPC crash with client disconnecting during IPC call", 2025-06-10). After many iterations the node aborted with mp::EventLoopRef::operator*() const: Assertion 'm_loop' failed, and under ASan reported a heap-use-after-free in EventLoopRef::operator->() right after "IPC server: socket disconnected".
  3. @pinheadmz ran bitcoin-cli -getinfo from four watch loops against a pruned mainnet node built from the branch of bitcoin#32297 ("bitcoin-cli: Add -ipcconnect option"), which did not have the fix yet (report, 2025-07-15; filed as mp#191, "Socket disconnect followed by segfault on macos/arm64"). bitcoin-node segfaulted in serverDestroy<ProxyServer<Rpc>> -> EventLoop::log with a garbage this, reached from ~ProxyServer<Rpc> inside a Cap'n Proto promise destructor. ryanofsky attributed it to this bug, and it did not reproduce on top of bitcoin#32345, which carried the fix.

Root cause

The code assumed that a ProxyServer is always destroyed before or inside the destructor of its Connection, so m_context.connection stays valid; the old Connection::~Connection comment (src/mp/proxy.cpp) said so. ~Connection resets the RPC system expecting Cap'n Proto to release every server object before the reset returns. That does not hold while a call is in flight. The call's promise, and for a method with a Context parameter the worker thread running it, keep the ProxyServer alive, so the Connection is freed first. Three paths then read the freed Connection to find the event loop:

Until late 2024 every IPC peer was a subprocess that Bitcoin Core spawned and stopped in order, so a connection never closed with a call running. Long-blocking mining calls and external clients that disconnect on their own schedule made the window reachable, and reconnect loops made it routine.

The code

Connection::~Connection (src/mp/proxy.cpp, quoted from 949573da8411^) expects resetting the RPC system to destroy every server object of the connection before the destructor goes on:

Connection::~Connection()
{
    // Shut down RPC system first, since this will garbage collect Server
    // objects that were not freed before the connection was closed, some of
    // which may call addAsyncCleanup and add more cleanup callbacks which can
    // run below.
    m_rpc_system.reset();

A server object with a call still running is not destroyed there: the call's promise, and for a method taking a Context the worker thread running it, keep the ProxyServer alive until the call finishes, after the Connection has been freed. Two places then reach the event loop through the freed m_context.connection. The worker, after the method returns, in PassField (include/mp/proxy-types.h, quoted from 196e6fcdbc1f^):

    fn.invoke(server_context, args...);
}
KJ_IF_MAYBE(exception, kj::runCatchingExceptions([&]() {
    server.m_context.connection->m_loop.sync([&] {
        auto fulfiller_dispose = kj::mv(fulfiller);
        fulfiller_dispose->fulfill(kj::mv(call_context));
    });

And ~ProxyServerBase (include/mp/proxy-io.h, quoted from 949573da8411^), when the server object is finally destroyed:

m_context.connection->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable {
    impl.reset();
    CleanupRun(fns);
});
  1. Worker: runs a long server method.
  2. Event loop: the client disconnects; ~Connection resets the RPC system, which cannot destroy the busy ProxyServer, and the Connection is freed.
  3. Worker: the method returns and PassField calls sync() on the freed connection's m_loop (the mp#118 crash, with this == nullptr inside EventLoop::sync).
  4. Later: ~ProxyServerBase calls addAsyncCleanup on the freed connection, which locks a mutex reached through freed memory (the mp#182 ASan report and m_loop assert).

The first change, 196e6fcdbc1f in mp#118, was a workaround: when the worker found its request_threads entry already gone it returned without fulfilling the promise, which avoided the step 3 dereference by leaking the server object. The fix is spread over three commits of about 130 lines in total, so no diff is shown here. In mp#160, 315ff537fb65 gives ProxyContext its own loop pointer and changes PassField to call server.m_context.loop->sync(...), and 949573da8411 moves addAsyncCleanup to EventLoop and has ~ProxyServerBase call m_context.loop->addAsyncCleanup(...). mp#186 then reverted the workaround (c6f7fdf17350), whose leaked server objects were making a new unit test hang.

The event loop outlives every connection and every server object, so once neither path goes through m_context.connection, a server object destroyed after its Connection no longer touches freed memory.

Fix

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 child object outlives its parent

  • Prevented by a clear ownership model
  • Earliest detector a disconnect test (did not exist then)
  • Diagnosis moderate
  • Debugging aids other, a reproduction script, a clear assertion message
  • Rust yes

Would Rust have prevented it? Yes. A ProxyServer could not keep a plain reference to a Connection the RPC system frees first: it would need an Arc (keeping the connection alive past its teardown) or a Weak whose None it must handle; and with capnproto-rust's Rc-based server objects a worker thread could not keep the server alive past the loop thread's teardown in the first place, at the cost of a different model for blocking method bodies. More on Rust

Could it have been prevented?

Yes, by ownership. From the initial commit a ProxyServer reached its event loop through a raw back pointer to the Connection that created it, on the documented assumption (the old ~Connection comment) that the connection would always be destroyed last. An in-flight call keeps the server alive past the connection, so the assumption was false from day one; it only held because no peer ever left. The fix gives each proxy its own EventLoopRef (ProxyContext::loop) and moves addAsyncCleanup to the loop, which is the object that actually outlives everything. The rule: a child must hold a reference to whatever it needs at destruction, never a pointer to a parent whose lifetime it does not control. That rule was knowable in 2019; the lifetime comment in ~Connection shows the author saw the dependency and chose to document it rather than remove it.

Could it have been found sooner?

Yes. The "disconnecting and blocking" test written with the fix (84cf56a0b5f4) crashes reliably under ASan without it, and the library had ASan available all along; what it lacked until 2025-06 was any test in which a peer disconnects with a call running. For five years every peer was a subprocess Bitcoin Core spawned and stopped in order. The first long-blocking method (waitFeesChanged, bitcoin#31003) and an interactive client one could Ctrl-C produced the crash within days of existing, 1,940 days after the code was written; a reconnect loop from a Rust client then made it routine.

What made it hard to debug?

Recognizing the assumption was quick: the same-day mp#118 workaround comment names "ProxyServer objects destroyed before Connection objects". What was slow was accepting the cost of fixing it. The workaround skipped the crashing line by leaking the server (and calling the bug a null-pointer dereference, when the null came from reading freed memory), which left two other dereference sites and later caused the mp#186 test hangs. The proper fix waited eight months for darosior's ASan report (mp#182), which named the freed object unambiguously where the first trace had only shown this=0x0. A third sighting (mp#191) still needed a debugger and hand attribution because the branch predated the fix. A sanitizer run of the first repro, or an assert in ~ProxyServerBase that the connection was still alive, would have turned the mystery null into a clear lifetime violation immediately.

Lesson

When you write a comment saying "X is always destroyed before Y", treat it as a bug report: either make Y own what it needs, or add an assert that fires when the order breaks. A workaround that skips the crashing line leaves the other lines.

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 crash
Seen in branch-build, mptest
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase steady-state
Side server
Component proxy-io
Root cause kind lifetime
Fix kind refactor
Detection manual-test
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released source
Confidence high