ThreadSanitizer reported a data race between a server thread executing an
async request and the event loop thread handling an abrupt remote disconnect
(https://github.com/bitcoin-core/libmultiprocess/issues/348): the server
thread called call_context.getResults(), which reads Cap'n Proto connection
state, while the event loop thread overwrote that state.
In addition to the general undefined behavior, the race has one interleaving
with a concrete failure: a server thread can dereference a null pointer and
crash the process, meaning a client that disconnects mid-call can take down
the server. RpcConnectionState::disconnect() (capnp/rpc.c++) runs on the event
loop thread and tears down the connection in two steps, moving the live
connection out of the RpcConnectionState::connection field (nulling the stored
pointer) and then flipping the field to its disconnected state. A server
thread calling getResults() between the two steps passes the is<Connected>()
check but then dereferences the nulled pointer. The other interleavings are
harmless: reading the field before both writes builds results into an outgoing
message that is simply never sent, and reading it after both writes takes the
normal disconnected code path, which builds results into a message detached
from the connection. There is no use-after-free, since the objects involved
stay alive through reference counts and the existing cancellation handshake.
The underlying problem is that connection state may only be accessed on the
event loop thread, and nothing lets libmultiprocess order server thread
accesses against the disconnect teardown:
- The teardown happens with no warning. The Connection::onDisconnect promise
used to clean up after disconnects only fires after capnp has finished
tearing down the connection and shutting down the stream.
- The in-flight request is not canceled first. With capnp's allowCancellation
feature off (the default), LocalClient::callInternal (capnp/capability.c++)
detaches a fork of the call promise, so capnp's teardown does not destroy
the promise chain that would trigger the CancelMonitor cancellation
handshake in PassField. Enabling allowCancellation would not help either:
disconnect() would then cancel in-flight requests as part of its teardown,
but only after the connection field has already been overwritten, so the
cancellation handshake still could not order server thread reads against
those writes, only narrow the window.
So no mutex or flag in libmultiprocess can help; the only options are making
server threads stop reading connection state, or patching capnp.
Fortunately, only the first getResults() call on a request reads connection
state, to decide whether to allocate the results struct inside a real outgoing
message or in a message detached from the connection
(RpcCallContext::getResults in capnp/rpc.c++). The response it allocates is
cached, and later getResults() calls return it without reading connection
state.
So fix the race by initializing the results struct on the event loop thread,
in the existing loop.sync() call that runs before a request executes. The
getResults() calls that later run on the server thread just return the cached
response and never touch connection state. The cost is that if the method
throws, the preallocated results message is wasted (error returns are built
separately), the same tradeoff capnp itself makes with its internal "force
initialization of response" getResults calls.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
