type-context: fix async disconnect race condition found by antithesis - #349
Open
ryanofsky wants to merge 1 commit into
Open
type-context: fix async disconnect race condition found by antithesis#349ryanofsky wants to merge 1 commit into
ryanofsky wants to merge 1 commit into
Conversation
…ndling ThreadSanitizer reported a data race between a server thread executing an async request and the event loop thread handling an abrupt remote disconnect (bitcoin-core#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>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline and AI policy for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix a race condition reported in #348 where if a disconnect happens during an IPC call that uses a worker thread (an IPC call taking an
mp.Contextparameter), it can trigger a read-write race detected by TSAN, and also theoretically cause a null pointer dereference (described in the commit message).The race condition happens because when
RpcCallContext::getResults()is called for the first time, it checks the connection state. So currently if there is a disconnect, when the worker thread callsgetResults, this can read connection state at the same time capnproto writes as it processes the disconnect.Fix this issue by calling
getResultsonce from the event loop thread before executing the IPC call on the worker thread, so the results message pointer will be cached, and future calls togetResultsfrom the worker thread won't access the connection state or have any race condition.This is a one-line fix with many comments and a test.