IPC stability bugs

Bug 8

Node aborts when freeing a callback object from a client that disconnected

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
2025-09-29 (first report)
Fixed
2026-06-02
Fix
mp#273
Shipped to Bitcoin Core
bitcoin#35661
In released binaries
Yes
Defect ever on master
Yes

Summary

When a client passes a callback object to the node, such as a handler for chain notifications, the node holds a ProxyClient for it and calls the callback's destroy method when it frees it. bitcoin-node aborted with std::terminate when a server-owned ProxyClient for a callback object was destroyed after the peer had disconnected: the proxy's cleanup made a remote destroy() call on the dead connection, which threw inside a noexcept destructor. It was seen with a Rust wallet client against the branch of bitcoin#29409, which adds an IPC wrapper for the node's Chain interface so a wallet can run as a separate process. Released binaries (v30.0 through v31.1) contain the code but cannot reach it, because no interface on master has a server-owned callback proxy with a destroy method. Fixed in mp#273.

What happened

Root cause

The Rust client registered for chain notifications, then disconnected without first calling Handler.destroy and waiting for it. On the node, destroying the Handler server object destroys its ProxyClient<ChainNotifications>, whose cleanup lambda in ProxyClientBase (include/mp/proxy-io.h) calls Sub::destroy(*this) to make a ChainNotifications.destroy IPC call back to the client. The connection is already gone, so that call throws ipc::Exception inside a destructor that is implicitly noexcept, and std::terminate runs.

Making the destructor noexcept(false) alone did not help: xyzconstant found that the crash moved to "No event loop is running on this thread", and ryanofsky explained that the throw skipped the rest of the lambda, so m_client was later freed on the wrong thread.

Any peer that drops the connection while the node still owns a callback proxy for it can reach this, including a C++ wallet process that dies with handlers registered. A C++ client that shuts down normally tears its proxies down in order before closing, so the server never has to call back into a dead peer; only the Rust client, which drops the connection and lets objects be garbage collected, has hit it.

The unguarded call dates from the library's initial commit.

The code

ProxyClientBase in include/mp/proxy-io.h, as of 6de92e1c7324^. The constructor registers a cleanup function that the destructor runs, and the destructor is declared noexcept:

    m_context.cleanup_fns.emplace_front([this, destroy_connection, disconnect_cb]{
    {
        // ...
        Sub::destroy(*this);

        // FIXME: Could just invoke removed addCleanup fn here instead of duplicating code
        m_context.loop->sync([&]() {
            // ...
            if (m_context.connection) m_context.connection->removeSyncCleanup(disconnect_cb);

            // Release client capability by move-assigning to temporary.
            {
                typename Interface::Client(std::move(m_client));
            }
            // ...
        });
    }
    });
    // ...
template <typename Interface, typename Impl>
ProxyClientBase<Interface, Impl>::~ProxyClientBase() noexcept
{
    CleanupRun(m_context.cleanup_fns);
}

For interfaces with a destroy method, such as ChainNotifications, Sub::destroy makes a remote destroy call. When the node destroys its ProxyClient<ChainNotifications> after the Rust client has disconnected, that call throws ipc::Exception. The exception leaves a noexcept destructor and std::terminate ends the node. Removing noexcept would not be enough: the throw also skips the sync() block, so m_client is left to be released later on a thread with no event loop.

The fix catches the exception and logs a warning, so the rest of the cleanup runs (6de92e1c7324):

@@ -538,7 +538,12 @@ ProxyClientBase<Interface, Impl>::ProxyClientBase(typename Interface::Client cli
-        Sub::destroy(*this);
+        // Exceptions are caught and logged rather than propagated because
+        // ~ProxyClientBase is noexcept and the peer may be gone by the time
+        // this runs.
+        if (kj::runCatchingExceptions([&]{ Sub::destroy(*this); }) != nullptr) {
+            MP_LOG(*m_context.loop, Log::Warning) << "Remote destroy call failed during cleanup. Continuing.";
+        }

No exception can now leave the destructor, and the sync() block still removes the disconnect callback and releases m_client on the event-loop thread.

Fix

6de92e1c7324 (xyzconstant) wraps Sub::destroy in kj::runCatchingExceptions and logs a warning, "Remote destroy call failed during cleanup. Continuing." (+6/-1). Catching there lets the rest of the cleanup lambda (removeSyncCleanup and releasing m_client inside loop->sync) run, so it covers the wrong-thread failure too. The test "Destroying ProxyClient<> with destroy method after peer disconnect" (90be8354d47f) saves a callback on the server and disconnects.

In review ryanofsky noted that suppressing all destroy-time errors is not ideal but better than terminating, and propagating them would need throwing destructors across every generated interface. @enirox001 tried that and found it "broadens the change quite a bit".

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 Error paths that skip cleanup or completion: Assuming a remote call cannot fail

  • Prevented by a design rule, a review checklist item
  • Earliest detector a disconnect test (did not exist then)
  • Diagnosis moderate
  • Fix design moderate
  • Debugging aids logging, symbolized stack traces, a reproduction script
  • Rust partly

Would Rust have prevented it? Partly. The remote destroy would return a Result the compiler makes the Drop impl acknowledge, but Drop cannot propagate it and a panic there during unwinding aborts just as terminate does, so the choice to log and continue is still the author's. More on Rust

Could it have been prevented?

Yes, by a rule the project has since stated in more than one place: a call across a process boundary can fail at any moment, and code that must complete regardless (destructors, shutdown, cleanup) catches the failure and carries on. The ProxyClientBase cleanup lambda has called Sub::destroy(*this), a round trip to the peer, from a noexcept destructor since the 2019 initial commit (06e1045baba0), on the assumption that the peer is there to answer. It is the same assumption bitcoin#32345 removed from Shutdown()'s client->stop() a year earlier. A review item "does anything in a destructor or cleanup path make a remote call, and what happens when it throws?" would have found the line at any time. Whether to swallow or propagate is a real design question, which is why the fix took a discussion: ryanofsky preferred propagating in principle, enirox001 found that this means throwing destructors across every generated interface, and the merged 6de92e1c7324 logs a warning and continues, which is the only option a destructor really has.

Could it have been found sooner?

Yes, by the test the fix added: save a callback proxy on the server, disconnect the client, destroy the proxy. That case is deterministic and needs no foreign client, but until mp#273's regression test nothing in either repository exercised it, because every C++ client tears its handlers down and waits for Handler.destroy before closing, and no interface on Bitcoin Core master gives the server a callback proxy with a destroy method. It took the wallet-over-IPC branch (bitcoin#29409), which has one, plus a Rust client that drops the connection and lets objects be collected, for TheCharlatan to hit it. A foreign-client test would have found it too, but the in-process disconnect test is cheaper and was possible from the day mp#160 added the disconnect harness.

What made it hard to debug?

The first report had a gdb backtrace through std::terminate and the IPC debug log, and the log is what carried the diagnosis: "IPC server destroy ...Handler" followed by "IPC client send ChainNotifications.destroy$Params" and then "interrupted by disconnect" shows the order in three lines. Even so, ryanofsky's first reading put the throw in ~ProxyServerBase, the issue then waited six months for a second reproduction, and the obvious fix (noexcept(false)) failed in a way that needed its own analysis: xyzconstant found the crash moved to "No event loop is running on this thread", and ryanofsky worked out that the throw had skipped the loop->sync() block below it, leaving m_client to be released later on the wrong thread. Catching at the call, rather than letting it escape, covers both failures at once. The fix's test exposed a further race in clientDestroy the next day (mp#286), so the path had never been run in this order before.

Lesson

A destructor that talks to another process must assume the process is gone; catch the failure at the call so the rest of the cleanup still runs, and write the "peer disconnected, now destroy" test for every remotely owned object the moment such objects exist.

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 unhandled-exception
Fix kind local-fix
Detection user-report
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see no
Defect ever on master yes
Released binary
Confidence high