Data race: server thread calls CallContext::getResults() while event loop tears down RpcConnectionState on remote disconnect

[dergoegge]

Found by ThreadSanitizer in a Bitcoin Core bitcoin-node build (libmultiprocess e8de5c7b68, capnproto 1.0.1) running under Antithesis (see here for how use it in Bitcoin Core).

(Full antithesis log)

LLM analysis:

Summary

Server method bodies run on a worker thread and call call_context.getResults() directly from that thread. When the peer disconnects abruptly, capnp's RpcConnectionState::disconnect() runs on the event loop thread and clobbers the connection state under the worker's feet. Nothing synchronizes the two.

Both TSAN reports hit the same RpcConnectionState heap block, on the same kj::OneOf<Own<VatNetworkBase::Connection>, Exception> connection member:

offset	writer — T2 b-capnp-loop	reader — T27 (IPC worker)
+0x38 (ptr)	Own::Own(Own&&)	Own::operator->()
+0x28 (tag)	destroyVariant / init<Exception>	OneOf::is<Own<Connection>>()

capnproto-c++-1.0.1 src/capnp/rpc.c++:

// writer: RpcConnectionState::disconnect(), :426-427
auto dyingConnection = kj::mv(connection.get<Connected>());   // write +0x38
connection.init<Disconnected>(kj::cp(networkException));      // write +0x28

// reader: RpcConnectionState::RpcCallContext::getResults(), :2581-2584
if (redirectResults || !connectionState->connection.is<Connected>()) {              // read +0x28
  ...
} else {
  auto message = connectionState->connection.get<Connected>()->newOutgoingMessage(  // read +0x38

A TOCTOU on the connection state: the worker passes is<Connected>(), the event loop then disconnects, and the worker dereferences a moved-from (null) Own — or takes the raw pointer just before dyingConnection drops the last reference and writes the return message into a freed transport.

Abridged stacks:

Write of size 8 at 0x72780002f438 by thread T2:
  #0 kj::Own<capnp::_::VatNetworkBase::Connection>::Own(Own&&)
  #1 capnp::_::RpcConnectionState::disconnect(kj::Exception&&)          rpc.c++
  #2 capnp::_::RpcConnectionState::taskFailed(kj::Exception&&)          rpc.c++
  #3 kj::TaskSet::Task::fire()
  #5 kj::EventLoop::turn()
  #9 kj::Promise<unsigned long>::wait(kj::WaitScope&, kj::SourceLocation)
 #10 mp::EventLoop::loop()                                             src/mp/proxy.cpp:247

Previous read of size 8 at 0x72780002f438 by thread T27 (mutexes: write M0):
  #0 kj::Own<capnp::_::VatNetworkBase::Connection>::operator->()
  #1 capnp::_::RpcConnectionState::RpcCallContext::getResults(kj::Maybe<capnp::MessageSize>)  rpc.c++
  #2 capnp::CallContext<Mining::CreateNewBlockParams, Mining::CreateNewBlockResults>::getResults(...)
  #3 mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall>::invoke<...>
     ...
 #16 mp::Unlock<mp::Lock, kj::Function<void ()>&>(mp::Lock&, kj::Function<void ()>&)          include/mp/util.h:216
 #17 mp::Waiter::wait<mp::ProxyServer<mp::ThreadMap>::makeThread(...)::$_0::...>
 #20 mp::ProxyServer<mp::ThreadMap>::makeThread(...)::$_0::operator()() const

Why it happens

1. PassField for mp.Context (include/mp/type-context.h:74-196) posts the method body to a dedicated worker thread via ProxyServer<Thread>::post. The generated ServerRet::invoke then calls call_context.getResults() on that thread — but capnp RpcCallContext is event-loop-thread-only.

2. The event loop holds no lock while capnp runs. EventLoop::loop() (src/mp/proxy.cpp:246-247) acquires m_mutex only after wait_stream->read(...).wait(waitScope) returns; the entire kj loop — including taskFailed → disconnect() — turns inside that wait() with the mutex released.

3. The M0 the worker holds is the per-request cancel_mutex (include/mp/type-context.h:100-101), which is documented (:113-126) to block the event loop thread from freeing the request's params/results structs mid-execution. But it only guards the promise-cancellation path: Connection::~Connection → m_canceler.cancel() → CancelProbe dtor → CancelMonitor::m_on_cancel → Lock cancel_lock{cancel_mutex}.

On a remote disconnect, capnp's RpcConnectionState::disconnect() runs first, directly from taskFailed, before the onDisconnect handler destroys the Connection. That path never touches cancel_mutex and never waits for in-flight workers. That's the unprotected window.

(TSAN's "created at" stack for M0 points at a different request's post; expected, since cancel_mutex is a stack local and the address is reused.)

Reproduction

Sequence from the run:

22.618  node: "IPC server: socket connected"           (client connects)
23.511  test harness SIGKILLs the client               (exit 137)
23.617  node: "CreateNewBlock(): block weight: 804"    (worker T27 executing the call)
23.675  TSAN: data race

Minimal recipe: TSAN-instrumented server, client issues a Mining.createNewBlock (any method whose body is slow enough to still be executing), SIGKILL the client mid-call. The abrupt EOF drives taskFailed → disconnect() concurrently with the worker's getResults().

Antithesis' coverage instrumentation (__sanitizer_cov_trace_pc_guard_internal → sleep_for) lands inside the window, which is why TSAN adds As if synchronized via sleep — that's what widened the race, not a false positive.

Possible fixes

1. Route getParams()/getResults() through loop.sync() so all capnp access stays on the event loop thread. Correct, but a round-trip per field access.
2. Copy params into a worker-owned MessageBuilder before dispatch and write results back in a single loop.sync() after the body returns. One hop per call, and it removes the need for the cancel_mutex / request_canceled handshake entirely, since the worker would no longer alias anything the event loop can free.
3. Make the cancel_mutex handshake cover capnp's internal disconnect — would need a hook inside RpcConnectionState::disconnect(), i.e. patching capnp.

[dergoegge]

The LLM made up the capnp version. This happens with the version we have in depends (1.5.0), and the capnp rpc code remains the same.

[ryanofsky]

Thanks! The thing I'm confused by here is that case is supposed to be already handled by cancelling the request in the onDisconnect handler

libmultiprocess/src/mp/proxy.cpp

Lines 117 to 118 in 275c8ee

	// Try to cancel any calls that may be executing.
	m_canceler.cancel("Interrupted by disconnect");

But the bug report seems to suggest that capnproto is actually discarding all connection state before triggering onDisconnect, which is surprising. If connection state can literally just disappear at any time with no notification than that really leaves us with few options other than the unappealing possible fixes suggested by the LLM above of synchronizing every access to parameters and results (1) or making complete copies of them (2). I thought we were already doing (3)...

[ryanofsky]

To answer my questions above:

• The onDisconnect handler doesn't help with this case because it runs too late, only after the VatNetworkBase::Connection object is destroyed, not before.
• Using cancel_mutex to handle disconnects already prevents the results message from being deleted while the worker thread might access it. It just doesn't prevent the connection state which is read by the RpcCallContext::getResults from being updated by the capnproto event loop in the background.

Turns out there is a nice one-line fix for this implemented in #349:

--- a/include/mp/type-context.h
+++ b/include/mp/type-context.h
@@ -131,6 +131,8 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn&
             std::tie(request_thread, inserted) = SetThread(
                 GuardedRef{thread_context.waiter->m_mutex, request_threads}, server.m_context.connection,
                 [&] { return Accessor::get(call_context.getParams()).getCallbackThread(); });
+            // Cache results message so it is safe to access from the worker thread.
+            call_context.getResults();
         });
 
         // If an entry was inserted into the request_threads map,