IPC stability bugs

Bug 40

Node crashes at shutdown when a client left a notifications handler behind

Status
Fixed
Symptom
Crash; worst effect seen: crash
Where seen
Branch-build
Trigger
Shutdown
First reported
2024-12-16 (first report)
Fixed
2025-08-18
Fix
bitcoin#32345, mp#160
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

bitcoin-node segfaulted at shutdown when an IPC client had registered for chain notifications and dropped the returned Handler without calling destroy (the IPC message that tells the server to free an object). libmultiprocess deferred destroying the node-side NotificationsHandlerImpl until the connection was torn down, and by then Shutdown() had already freed the ValidationSignals object its destructor unregisters from. It needs Chain.handleNotifications over IPC, which only the unmerged wallet-over-IPC branch bitcoin#29409 ("multiprocess: Add capnp wrapper for Chain interface") exposes, so no release could reach it. Fixed by bitcoin#32345 ("ipc: Handle unclean shutdowns better") together with mp#160 ("refactor: EventLoop locking cleanups + client disconnect exception").

What happened

Root cause

Two lifetimes cross:

In the December 2024 report the connection was torn down after Shutdown() had returned, so the deferred destructor always ran against a freed ValidationSignals. In the May 2025 report the connection was torn down inside Shutdown(), but the destruction was still asynchronous, so the cleanup thread raced the rest of Shutdown() and could lose.

The code

The node side is quoted from 7f65aac78b95^. NotificationsHandlerImpl in src/node/interfaces.cpp, the object behind a Chain.handleNotifications handler, keeps a reference to ValidationSignals and uses it in its destructor:

explicit NotificationsHandlerImpl(ValidationSignals& signals, std::shared_ptr<Chain::Notifications> notifications)
    : m_signals{signals}, m_proxy{std::make_shared<NotificationsProxy>(std::move(notifications))}
// ...
~NotificationsHandlerImpl() override { disconnect(); }
void disconnect() override
{
    if (m_proxy) {
        m_signals.UnregisterSharedValidationInterface(m_proxy);

Shutdown() in src/init.cpp frees that object near the end, while IPC clients may still be connected:

node.validation_signals.reset();

The library side is quoted from 949573da8411^. When a client drops a capability without calling destroy, ~ProxyServerBase (include/mp/proxy-io.h) does not destroy the implementation; it parks the destruction on the connection:

m_context.connection->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable {
    impl.reset();
    CleanupRun(fns);
});

and only Connection::~Connection (src/mp/proxy.cpp) hands the parked functions to the async cleanup thread. With darosior's client:

  1. Client: calls handleNotifications and drops the returned Handler; the NotificationsHandlerImpl is parked on the connection, still registered.
  2. Node main thread: Shutdown() resets node.validation_signals.
  3. Event loop: the client disconnects, ~Connection queues the parked destructor, and the async cleanup thread runs ~NotificationsHandlerImpl, which calls UnregisterSharedValidationInterface on the freed ValidationSignals and crashes.

Two commits close this (7f65aac78b95, 949573da8411; the disconnectIncoming plumbing and the removal of the per-connection list are omitted):

@@ -374,6 +374,12 @@ void Shutdown(NodeContext& node)
         client->stop();
     }

+    // If any -ipcbind clients are still connected, disconnect them now so they
+    // do not block shutdown.
+    if (interfaces::Ipc* ipc = node.init->ipc()) {
+        ipc->disconnectIncoming();
+    }
@@ -477,7 +486,7 @@ ProxyServerBase<Interface, Impl>::~ProxyServerBase()
-        m_context.connection->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable {
+        m_context.loop->addAsyncCleanup([impl=std::move(m_impl), fns=std::move(m_context.cleanup_fns)]() mutable {

A handler the client drops is now destroyed on the async thread right away, while the node is running, and Shutdown() tears down incoming connections before it frees ValidationSignals. The destruction after disconnectIncoming() is still asynchronous, so a client that holds its Handler until shutdown can still race the end of Shutdown().

Fix

ryanofsky's first fix (2025-05-21, described on mp#176) made shutdown wait for every object of every connection to be destroyed before freeing node state. He then judged it incompatible with the connection a spawned bitcoin-gui keeps to its parent process, and on 2025-06-05 replaced it with a simpler fix. In the merged code two commits close the mechanism:

A client that keeps its Handler registered until the very end still leaves a narrower race between the async cleanup thread and the tail of Shutdown(); that remaining race is addressed by later work, not by these commits.

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, a design rule
  • Earliest detector a test with a non-C++ client (did not exist then)
  • Diagnosis hard
  • Fix design hard
  • Debugging aids symbolized stack traces, logging, sanitizer output
  • Rust yes

Would Rust have prevented it? Yes. A handler holding &ValidationSignals could not be handed to a remote owner with a longer lifetime, so the field would have to become an Arc (or Weak), which keeps the signals object alive through the late destructor instead of freeing it; the crash becomes a lifetime extension whose shutdown consequences still need thought. More on Rust

Could it have been prevented?

Partly. NotificationsHandlerImpl holds a bare reference to ValidationSignals and unregisters in its destructor; in-process that is safe because every handler is owned by something the node destroys before Shutdown() resets node.validation_signals. Over IPC the handler's owner is a remote client, so its lifetime is no longer nested in the node's. An ownership rule, "an object handed to a client may only reference node state it keeps alive, or the node must destroy every client-owned object before that state", would have forced the question when bitcoin#30509 let outside clients connect. The library half (a dropped object parked until connection teardown, 949573da8411) was a separate latent choice from 2019. Neither was obvious at the time; the fix sequence shows the rule being worked out rather than applied.

Could it have been found sooner?

Yes, by a test that connects a client, registers notifications, and stops the node while the client is still there, under ASan so the freed ValidationSignals is fatal rather than a garbage-pointer lock. No such test existed: Bitcoin Core's spawned C++ clients are stopped by the node before it frees that state, so nothing in-tree held a Handler across shutdown until darosior's Rust client did in December 2024. The fix added an in-process Init/Shutdown harness but no client-at-shutdown case, and the May 2025 recurrence came from re-testing the same client on the branch meant to fix December.

What made it hard to debug?

The December crash was hidden behind a hang (the node waited for the client to exit first), and the May one was a segfault after Shutdown: done with a gdb backtrace on the b-capnp-loop thread inside UnregisterSharedValidationInterface with a garbage this, which says what died but not why it was still referenced. ryanofsky's first fix (wait for every connection's objects before freeing node state) was correct but incompatible with the bitcoin-gui parent connection, so it was replaced two weeks later by moving disconnectIncoming() earlier in Shutdown(); the residual race for a client that holds its handler to the end went to mp#336 and the drain work. The "IPC server destroy ...Handler" log line was the clue that the object had been dropped without destroy, and named threads in the backtrace placed the destructor on the cleanup thread.

Lesson

When an object is owned by a remote party, its destructor runs at a time the process does not choose; either make it self-sufficient (own or keep alive what it touches) or make shutdown destroy all remotely owned objects before the state they use.

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
Scope production
Trigger Shutdown (shutdown)
Phase shutdown
Side server
Component bitcoin-init
Root cause kind lifetime
Fix kind local-fix
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