IPC stability bugs

Bug 64

Data race on a proxy's connection pointer when a peer disconnects

Status
Fixed
Symptom
Data race; worst effect seen: sanitizer report only
Where seen
Libmultiprocess tests (mptest)
Trigger
A client disconnects abruptly, for example because it is killed during a call
First reported
2026-06-03 (first report)
Fixed
2026-06-11
Fix
mp#286
Shipped to Bitcoin Core
bitcoin#35661
In released binaries
Yes
Defect ever on master
Yes

Summary

clientDestroy(), which runs when a ProxyClient is destroyed, read the proxy's m_context.connection pointer without a lock, while the disconnect cleanup on the event-loop thread could set it to null. The only thing the read decided was which log line to write, so the practical effect was a TSan data-race report, first seen in libmultiprocess sanitizer CI on master. Released Bitcoin Core binaries (v30.0 through v31.1) contain the code; it has only been triggered with a server-owned callback proxy in mptest. Fixed in mp#286 by dropping the check.

What happened

Root cause

clientDestroy() in include/mp/proxy-types.h did if (client.m_context.connection) MP_LOG(...) else KJ_LOG(INFO, "IPC interrupted client destroy", ...), a plain read of the pointer. The sync cleanup registered by ProxyClientBase's constructor (include/mp/proxy-io.h) sets m_context.connection = nullptr under m_context.loop->m_mutex on the event-loop thread when the connection is torn down.

In mp#273's regression test the server owns a ProxyClient<FooCallback>. When the client disconnects, the event-loop thread runs ~Connection and its sync cleanups while the async cleanup thread destroys the ProxyServer<FooInterface>, whose destructor destroys the callback proxy and calls clientDestroy. The unlocked read and the locked write overlap.

Any disconnect while the server holds a callback proxy for the peer can reach this, such as a C++ wallet or GUI process dying with handlers registered on the node. A clean C++ shutdown destroys proxies first and cannot. A client-side variant (bitcoin-cli -ipcconnect dropping a proxy as the node disconnects) looks possible but has not been seen.

The connection check dates from the library's initial commit.

The code

Quoted from 25bb3e67f391^. Every generated ProxyClient destructor calls clientDestroy() (include/mp/proxy-types.h), which reads the connection pointer without a lock only to pick a log call:

template <typename Client>
void clientDestroy(Client& client)
{
    if (client.m_context.connection) {
        MP_LOG(*client.m_context.loop, Log::Debug) << "IPC client destroy " << CxxTypeName(client);
    } else {
        KJ_LOG(INFO, "IPC interrupted client destroy", CxxTypeName(client));
    }
}

The disconnect callback that ProxyClientBase's constructor registers (include/mp/proxy-io.h) writes the same pointer on the event-loop thread, under a mutex the reader never takes:

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

When the server holds a callback proxy (a ProxyClient<FooCallback> in the mp#273 test) and the client disconnects:

  1. Event loop: ~Connection runs the sync cleanups, including this callback, which sets m_context.connection = nullptr.
  2. Async cleanup thread: at the same time it destroys the ProxyServer<FooInterface>, whose destructor destroys the callback proxy, and clientDestroy reads m_context.connection.

TSan reports the unlocked read racing the locked write. The only effect of a stale read is which log line is written.

The fix removes the read (25bb3e67f391):

@@ -644,11 +644,7 @@ struct CapRequestTraits<::capnp::Request<_Params, _Results>>
 void clientDestroy(Client& client)
 {
-    if (client.m_context.connection) {
-        MP_LOG(*client.m_context.loop, Log::Debug) << "IPC client destroy " << CxxTypeName(client);
-    } else {
-        KJ_LOG(INFO, "IPC interrupted client destroy", CxxTypeName(client));
-    }
+    MP_LOG(*client.m_context.loop, Log::Debug) << "IPC client destroy " << CxxTypeName(client);
 }

m_context.loop is an EventLoopRef that stays valid after the connection is gone, so logging through it needs no check, and clientDestroy no longer touches the pointer the event loop writes.

Fix

25bb3e67f391 (co-authored with Claude) drops the branch and always logs through MP_LOG (+1/-5). Since 315ff53a0d05 ("refactor: Add ProxyContext EventLoop* member", merged in mp#160, a set of event loop locking cleanups), m_context.loop is an EventLoopRef that is valid whether or not connection is null, so the KJ_LOG fallback was dead code. mp#293 took the other route and added the lock. mp#286's other commit only adjusts IWYU pragmas in gen.cpp.

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 Locks, waits and wakeups: A field locked by some accessors and not others

  • Prevented by lock annotations, a design rule
  • Earliest detector ThreadSanitizer (did not exist then)
  • Diagnosis easy
  • Fix design easy
  • Debugging aids sanitizer output, symbolized stack traces, a reproduction script
  • Rust yes

Would Rust have prevented it? Yes. A pointer written under a mutex on one thread and read bare on another is a Send/Sync compile error; the field would live inside the Mutex and the reader would lock it, and capnproto-rust proxies are !Send so the off-loop destructor that does the read could not exist. More on Rust

Could it have been prevented?

Yes. ProxyContext::connection is written to null under EventLoop::m_mutex by the disconnect callback and was read bare in clientDestroy() since the 2019 initial commit, only to choose between two log lines. A GUARDED_BY(loop->m_mutex) annotation on the field would have made the read a compile error under clang's thread-safety analysis; the annotation pass in mp#201 covered the Waiter and EventLoop members but left this pointer bare, so the check that finds one-sided locking at compile time was available for ten months and not applied to the field that needed it. The fix (25bb3e67f391) does not add the lock, as mp#293 independently proposed, but deletes the read: since mp#160 made m_context.loop an EventLoopRef that outlives the connection, the fallback branch was dead code. A design rule that logging never inspects shared state would have removed the read before it could race.

Could it have been found sooner?

Yes, by ThreadSanitizer on a test that destroys a server-owned callback proxy after its peer has disconnected. The library has had a TSan job since mp#186 (2025-07) and disconnect tests since mp#160, but none of them destroyed a ProxyClient on the async cleanup thread while the loop thread processed that connection's teardown; the regression test for mp#273 was the first to, and the race reported the day after it merged. Even then it was timing-dependent: ryanofsky found the test alone trips TSan about one run in ten, and only the CxxTypeName call that mp#218 added to clientDestroy the same day shifted the timing enough for master to fail most of the time, which is why both PRs passed CI on their own.

What made it hard to debug?

Nothing about the cause: the CI job gave a symbolized pair of stacks (the write in the ProxyClientBase cleanup lambda from ~Connection on the loop thread, the read in clientDestroy from ~ProxyServerBase on the async cleanup thread), a second contributor reached the same diagnosis independently within six days, and the fix is four deleted lines. The instructive part is the detection gap: a race that needs two merged PRs to interact will pass each PR's CI and fail on master, so the sanitizer job on master, not on PRs, is what caught it, and the first run of a new test under TSan is not evidence that the test's path is race-free.

Lesson

A lock protects a field only when every reader takes it; annotate guarded fields so the compiler finds the bare read, and treat a log statement that inspects shared state as a real access with the same locking obligations as any other.

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 data-race
Worst effect observed sanitizer-report-only
Seen in mptest
Scope production
Trigger Client disconnects abruptly (client-unclean-disconnect)
Phase steady-state
Side server
Component proxy-types
Root cause kind missing-lock
Fix kind one-line-tweak
Detection tsan
Needs a peer that is not libmultiprocess C++ no
Needed a sanitizer to see yes
Defect ever on master yes
Released binary
Confidence high