IPC stability bugs

Bug 29

Client aborts if the connection closes just as it starts a call

Status
Fixed
Symptom
Crash; worst effect seen: crash
Where seen
Branch-build
Trigger
The server side goes away abruptly
First reported
2024-12-03 (first report)
Fixed
2025-06-19
Fix
mp#160
Shipped to Bitcoin Core
bitcoin#32345
In released binaries
No; the code was in source releases only
Defect ever on master
Yes

Summary

An IPC client could abort with memory.h:258: failed: expected ptr != nullptr; null Own<> dereference if the connection closed just as it started a call. clientInvoke, the library function that sends a call, checked on the calling thread that the connection was still open, and only then handed the request to the event loop thread. If the disconnect was handled in between, the request was built on a capability that had already been cleared. It was hit by bitcoin-node on Ctrl-C when running with a bitcoin-wallet subprocess on the wallet-over-IPC branch, and by bitcoin-cli -ipcconnect connecting to a socket that was not a Cap'n Proto server. On master the only C++ IPC client whose server can disconnect this way is the node's echoipc connection to its own child, so no released code path is known to hit it. Source builds of v22 through v29 with multiprocess enabled have the defect. Fixed in mp#160, shipped in v30.0.

What happened

Root cause

clientInvoke (then in include/mp/proxy-types.h) began with if (!proxy_client.m_context.connection) throw std::logic_error("clientInvoke call made after disconnect") on the calling thread, then posted the request to the event loop with sync(...). If the connection's disconnect handling ran on the event loop thread between the check and the posted lambda, the connection's sync cleanup had already moved m_client out, leaving a null capability, and set m_context.connection to null. The lambda then called (proxy_client.m_client.*get_request)(nullptr) on a null Own<ClientHook>, and KJ's null check failed. In darosior's case this happened inside Shutdown(), where nothing caught it.

The check has been on the calling thread since the library's initial commit. It was harmless while the only peers were subprocesses that Bitcoin Core spawned and shut down in order; the wallet-over-IPC branch and external clients added peers that close connections on their own schedule.

The code

Quoted from 56fff76f940b^. clientInvoke (include/mp/proxy-types.h) checks for a disconnect on the calling thread, then builds and sends the request on the event-loop thread:

void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, FieldObjs&&... fields)
{
    if (!proxy_client.m_context.connection) {
        throw std::logic_error("clientInvoke call made after disconnect");
    }
    // ...
    proxy_client.m_context.loop->sync([&]() {
        auto request = (proxy_client.m_client.*get_request)(nullptr);

When the connection goes away, a cleanup registered in the ProxyClientBase constructor (include/mp/proxy-io.h) runs on the event-loop thread and empties the capability:

auto cleanup_it = m_context.connection->addSyncCleanup([this]() {
    // Release client capability by move-assigning to temporary.
    {
        typename Interface::Client(std::move(m_client));
    }
    m_context.connection = nullptr;
});

Nothing orders the check against this cleanup:

  1. Client thread: m_context.connection is still set, so the check passes.
  2. Event loop: the peer disconnects; the cleanup moves m_client out and clears m_context.connection.
  3. Client thread: sync() posts the lambda.
  4. Event loop: the lambda calls get_request on the empty m_client, KJ fails with null Own<> dereference, and the exception escapes the event loop.

The fix (56fff76f940b) moves the check into the sync() lambda and reports it, like a disconnect during the call, as an ipc::Exception from loop->raise() (the DISCONNECTED mapping is omitted):

@@ -592,9 +592,6 @@ void serverDestroy(Server& server)
-    if (!proxy_client.m_context.connection) {
-        throw std::logic_error("clientInvoke call made after disconnect");
-    }
@@ -616,7 +613,16 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel
+    const char* disconnected = nullptr;
     proxy_client.m_context.loop->sync([&]() {
+        if (!proxy_client.m_context.connection) {
+            const std::unique_lock<std::mutex> lock(invoke_context.thread_context.waiter->m_mutex);
+            done = true;
+            disconnected = "IPC client method called after disconnect.";
+            invoke_context.thread_context.waiter->m_cv.notify_all();
+            return;
+        }
+
         auto request = (proxy_client.m_client.*get_request)(nullptr);
@@ -654,6 +664,7 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel
+    if (disconnected) proxy_client.m_context.loop->raise() << disconnected;

The check and the cleanup now both run on the event-loop thread, so the lambda either sees m_context.connection still set, in which case m_client has not been moved out yet, or sees it null and returns before touching m_client.

Fix

The fix commit 56fff76f940b, in mp#160 ("refactor: EventLoop locking cleanups + client disconnect exception"), moves the connection check into the sync lambda, where it runs on the loop thread and cannot interleave with disconnect handling, and reports "IPC client method called after disconnect." instead of making the call. It also maps a DISCONNECTED kj::Exception from an in-flight call to "IPC client method call interrupted by disconnect.", and raises both through the event loop's raise() so callers see one kind of exception for a closed connection (+17/-6). It depends on 315ff537fb65 ("refactor: Add ProxyContext EventLoop* member") in the same PR, which gives ProxyContext its own EventLoop* so clientInvoke can reach the loop without the possibly-null connection pointer. The Bitcoin Core handling built on these exceptions (catching them in Shutdown(), a Ctrl-C handler for spawned subprocesses) came in bitcoin#32345 ("ipc: Handle unclean shutdowns better").

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 Work or cleanup on the wrong thread: A precondition checked on the wrong thread

  • Prevented by a thread-affinity assertion, a design rule
  • Earliest detector a disconnect test (did not exist then)
  • Diagnosis easy
  • Fix design moderate
  • Debugging aids a clear assertion message, a reproduction script
  • Rust partly

Would Rust have prevented it? Partly. The unsynchronized read of m_context.connection on the calling thread is a compile error without a Mutex, and with capnproto-rust's single-threaded RPC types the calling thread could not hold the proxy at all, so the check would have to be sent to the loop thread as the fix does; the trade-off is a channel-based model for blocking calls, and check-then-use across a hand-off remains an ordering bug Rust allows. More on Rust

Could it have been prevented?

Yes. m_context.connection and m_client are cleared by a cleanup that runs on the event-loop thread, so they are loop-owned state, and clientInvoke read the first on the calling thread and used the second on the loop thread with nothing ordering the two against the disconnect handler. A check that guards an action must run where the action runs, under the same serialisation as the thing that can invalidate it; the fix (56fff76f940b) moves the check inside the sync lambda and it becomes correct by construction. A thread-affinity assert on reads of m_context.connection would have flagged the off-loop check in 2019, when it was written as a convenience error message rather than as synchronisation. The pattern is a plain time-of-check to time-of-use race, and the one thing specific to this code base is that the loop thread is the only place where "connected" has a stable meaning.

Could it have been found sooner?

Yes, by a test in which the server side drops the connection while the client is entering a call, run in a loop. No client-side disconnect test existed in the library until 2025, and Bitcoin Core's spawned servers never closed a connection the node was still using. hebasto and Sjors had seen the calling-thread check fire as a logic_error on the bitcoin#10102 branch in 2020 and 2023, which shows the path was reachable; only the narrower interleaving, disconnect after the check and before the posted lambda, gives the null-capability abort. Even after the fix, mp#183 stayed open for a unit test of a peer that disconnects without replying, which arrived only in 2026.

What made it hard to debug?

Diagnosis was quick: darosior's Ctrl-C reproduction was deterministic enough for ryanofsky to reproduce it the next day and point at the early check, and KJ's null Own<> dereference with a backtrace into clientInvoke named the site. The fix took six months because it was entangled with what a client should see when its server goes away: the check could not move onto the loop without 315ff537fb65 giving ProxyContext its own EventLoop*, both disconnect cases had to raise one exception type, and the Bitcoin Core side then needed bitcoin#32345 to catch it in Shutdown(). The June 2025 socat reproduction by pinheadmz showed the same abort from a non-Cap'n-Proto listener, which is the cheapest reproduction of this class of bug: any client call against a socket that closes without answering.

Lesson

A precondition checked on one thread and consumed on another is not a check; move it to the thread that owns the state, and make "the peer is gone" an ordinary error the caller can catch rather than an assertion in library code.

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 Server disconnects abruptly (server-unclean-disconnect)
Phase any
Side client
Component proxy-types
Root cause kind ordering
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