type-context: fix async disconnect race condition found by antithesis - #349
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.
If your review is incorrectly listed, please copy-paste 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. |
|
Code review ACK 914dc83. This one-line change builds an empty LGTM. |
2dba13047 Merge bitcoin-core/libmultiprocess#363: ci: use LLVM 23 in Bitcoin Core CI 161197a5c Merge bitcoin-core/libmultiprocess#352: ci: add cmake debug output fb4ac7eb8 ci: use LLVM 23 in Bitcoin Core CI 7bac69de1 Merge bitcoin-core/libmultiprocess#360: pull latest .clang-tidy from downstream 79ddc44eb Merge bitcoin-core/libmultiprocess#359: ci: bump cmake version to 4.3.4 in newdeps job bf229bf82 Merge bitcoin-core/libmultiprocess#351: ci: do not ignore NIXPKGS_CHANNEL in local ci runs 073ac4f19 Merge bitcoin-core/libmultiprocess#347: refactor: Replace EventLoop::post() with sync() taking kj::FunctionParam fa1db7a9e pull latest .clang-tidy from downstream 5c49666a1 refactor: rename EventLoop::m_post_fn to m_sync_fn 2330fbe81 refactor: replace EventLoop::post() with sync() taking kj::FunctionParam 4d454a81d Merge bitcoin-core/libmultiprocess#358: refactor: Enable readability-container-contains dba99582b Merge bitcoin-core/libmultiprocess#357: doc: Update Cap'n Proto version to match minimum 00923922a Merge bitcoin-core/libmultiprocess#356: ci: Remove hard-coded -j4 from sanitize config 766867fb3 ci: clarify CAPNP_CHECKOUT=master is the v1.x release branch cc3675280 ci: bump cmake version to 4.3.4 in newdeps job fa101113b refactor: Enable readability-container-contains 81f824b02 doc: Update Cap'n Proto version to match minimum fa30e2093 ci: Remove hard-coded -j4 from sanitize config a7ff9d5da ci: add cmake debug output f2e8df82e Merge bitcoin-core/libmultiprocess#350: cmake: add type-unordered-set.h and version.h to public headers 0e146c046 Merge bitcoin-core/libmultiprocess#349: type-context: fix async disconnect race condition found by antithesis 49f95e263 Merge bitcoin-core/libmultiprocess#212: ci: add newdeps job testing newer versions of cmake and capnproto 7c73cceda ci: rename CI-internal variables to use CI_ prefix fe1b8339f ci: do not ignore NIXPKGS_CHANNEL in local ci runs 914dc839f proxy: fix data race between server request threads and disconnect handling 275c8eefd Merge bitcoin-core/libmultiprocess#345: Remove trailing whitespace and Add -Wtrailing-whitespace to default ci config cd7162fb8 Merge bitcoin-core/libmultiprocess#304: proxy: fix BuildList to use non-const iteration for interface types 9b136782a ci: Add -Wtrailing-whitespace to default config 2f4be9ec6 refactor: Remove trailing whitespace 2448d282c cmake: add type-unordered-set.h and version.h to public headers b3fc922ee ci: add newdeps job testing newest versions of cmake and capnproto 390b5f901 Merge bitcoin-core/libmultiprocess#344: test: listen_tests and connect_tests follow-ups d6f8588d1 proxy: fix BuildList to use non-const iteration for interface types e18ca520f Merge bitcoin-core/libmultiprocess#343: test: fix race in connect_tests disconnect-deferred-failure test c39c7850c doc: note construct() call in valid init interface test b9c36c617 test: close sockets unconditionally and check errors with KJ_SYSCALL 7eb741e63 test: drop unnecessary KJ_EXPECT(true) 113f1d4d2 test: join server thread unconditionally in connect tests 44bc4630b test: drop mp:: prefixes in connect tests 038d33eb3 test: share DefaultLogHandler between test files b54a16330 test: drop TestSetup socket members in connect tests 70467c5a7 test: add m_ prefix to TestSetup members in connect tests cc260f252 test: replace capnp fix link with upstream PR 137a6e4e0 test: fix race in connect_tests disconnect-deferred-failure test 8dab0d4bd Merge bitcoin-core/libmultiprocess#341: ci: add -Wextra-semi to llvm config b3b134eed ci: add -Wextra-semi to llvm config bdd0cd694 Merge bitcoin-core/libmultiprocess#339: refactor: add `[[noreturn]]` attributes a779a0976 ci: add -Wmissing-noreturn 636aaff57 refactor: add missing [[noreturn]] attributes cc11c2b1b Merge bitcoin-core/libmultiprocess#338: test: check ReadList return value 2d6e863c7 Merge bitcoin-core/libmultiprocess#334: ci: Set CMAKE_BUILD_PARALLEL_LEVEL to enable parallelism by default d4d10ff98 Merge bitcoin-core/libmultiprocess#332: ci: add -Wextra-semi to default config b540e70f2 Merge bitcoin-core/libmultiprocess#324: proxy: Name threads spawned by the event loop e5e367e78 Merge bitcoin-core/libmultiprocess#312: util: report back child errors to parent and throw 2220df68c Merge bitcoin-core/libmultiprocess#298: Fix error handling when creating clients (`mp::ConnectStream`) 51defb79e Merge bitcoin-core/libmultiprocess#340: ci: Update `capnproto` prerequisites on NetBSD 7e94790b0 ci: Update `capnproto` prerequisites on NetBSD 9f25ffca5 test: Cover OS thread names for worker, pool, and async threads 648a18589 proxy: Name threads spawned by the event loop 49834b260 ci: add -Wextra-semi to default config fae9a637e example: Remove unused kj/async.h include bb473690c Fix error handling when creating clients 44d191420 Add test coverage for ConnectStream 231361ae5 Correct stale UnixListener doc comment 060c1a50d Extract `UnixListener` class to a dedicated file 62f25af06 test: check ReadList return value ce51d7372 ci: Set CMAKE_BUILD_PARALLEL_LEVEL to enable parallism in build jobs by default 67302cd13 Merge bitcoin-core/libmultiprocess#331: Remove code for Cap'n Proto versions before 0.9 f13c64ab5 Merge bitcoin-core/libmultiprocess#330: ci: Compile with minimum supported g++ in olddeps 8e026f662 Merge bitcoin-core/libmultiprocess#327: build: avoid unnecessary capnp-rpc dependency for mpgen e5206e9eb Merge bitcoin-core/libmultiprocess#325: cmake: Remove `QUIET` option from `find_package(CapnProto ...)` 879efea2b Merge bitcoin-core/libmultiprocess#321: ci: Roll NetBSD releases to 11.0, drop 9.4 abf127a31 Merge bitcoin-core/libmultiprocess#317: ipc: Fix mpgen capnp tool path for vcpkg/Windows builds c437d7f10 Merge bitcoin-core/libmultiprocess#310: test: cover immediate client disconnects for `ListenConnections` 31bff8a67 Merge bitcoin-core/libmultiprocess#307: refactor: memcpy -> std::ranges::copy f355108b0 Merge bitcoin-core/libmultiprocess#303: type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point 2d678177c Merge bitcoin-core/libmultiprocess#296: ci: Bump channel to nixos-26.05 3f05b1162 util: kill and reap child on SpawnProcess error 4a56c1837 util: report back child error to parent and throw a9e70dbe7 ci: Add NetBSD release 11.0 2d33b14fb ci: Switch to default compiler on NetBSD 9.4 36f740027 ci: Drop NetBSD release 9.4 bd508311b refactor: Drop stray semicolons after function definitions 788f17a85 Remove code for Cap'n Proto versions before 0.9 7402affd0 ci: Pin oldeps config to older nixpkgs channel to compile older cmake with older gcc edf634356 ci: Compile with minimum supported g++-11 in olddeps fa47449af cmake: avoid unnecessary capnp-rpc dependency for mpgen a494b764d cmake: Remove `QUIET` option from `find_package(CapnProto ...)` 26452e02d refactor: memcpy -> std::ranges::copy e1dcc6eb1 Merge bitcoin-core/libmultiprocess#316: cmake: Fix stale codegen when mpgen binary changes 7a72df02e type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point 45b685c3f type-number, type-chrono: Fix static assert signed/unsigned comparisons 45f625597 type-number: exclude bool from the integral overload 8d6d46494 Merge bitcoin-core/libmultiprocess#315: Fix startup race in example a6fc80d25 Merge bitcoin-core/libmultiprocess#311: bugfix: clear FD_CLOEXEC in child instead of parent before fork 496fb84e6 test: cover immediate client disconnects for `ListenConnections` 36c6c6352 doc: Document reference-counted EventLoop lifetime 3a997e113 Fix startup race in mpexample f5c15ce33 Merge bitcoin-core/libmultiprocess#323: refactor: access ThreadContext through CurrentThread(), ci: switch Bitcoin Core to master 66298c737 ci: Switch back to Bitcoin Core's master branch 86b481050 refactor: access ThreadContext through CurrentThread() eea9c64f6 cmake: Fix stale codegen when mpgen binary changes a26a08496 cmake: Fix mpgen capnp tool path for vcpkg/Windows builds 140d9ba6f test: allow custom log handler in `ListenSetup` 1e0c7ff9a util: Clear FD_CLOEXEC in child instead of parent before fork 8550ee6a3 util, refactor: Add ChildFail helper for post-fork child errors 17eab90b5 test: Fix typo in listen_tests.cpp ce865a9ba refactor: Directly use value in CustomBuildField 3f221b5bf Merge bitcoin-core/libmultiprocess#274: Add nonunix platform support e8de5c7b6 Merge bitcoin-core/libmultiprocess#305: refactor: memcpy to std::ranges::copy to work around ubsan warn 1b0f60560 doc: Remove trailing whitespace d8f8ca311 ipc: Wrap mpgen main() in try-catch to print errors fbe5a14ad ci: Check out bitcoin/bitcoin PR #35084 instead of master 39d3690d8 types: Replace SFINAE with requires clauses to avoid MSVC C2039 error ba6852020 proxy, refactor: Fix C4305 truncation warning in Accessor on MSVC 1d81d4781 util, refactor: Fix PtrOrValue constructor for move-only types on MSVC b883fe1e5 proxy: Fix shutdownWrite() exception handling on macOS with dynamic libraries 0012411cc proxy: Call shutdownWrite() in Connection destructor 38312ad19 proxy, refactor: Change ConnectStream and ServeStream to accept stream objects e96d5d742 proxy, refactor: Replace EventLoop wakeup fd integers with KJ stream objects db4f9a3d7 cmake: Bump minimum required Cap'n Proto version to 0.9 652934fb7 util, refactor: Add SocketPair() and use it in SpawnProcess 1c6ef7a26 util, refactor: Do not fork() and exec() separately 1389cf313 util, refactor: Add SpawnConnectInfo type alias and use it c7ca1f00b util, refactor: Add SocketId type alias and use it be46a3520 util, refactor: Add ProcessId type alias and use it 91a78db78 doc: Bump version 13 > 14 9307e68e5 Merge bitcoin-core/libmultiprocess#306: doc: Bump version 12 > 13 fac7b9b7f refactor: memcpy to std::ranges::copy to work around ubsan warn 1bd702560 Merge bitcoin-core/libmultiprocess#297: test: add map serialization round-trip coverage 438fdd243 doc: Bump version 12 > 13 28e056576 Merge bitcoin-core/libmultiprocess#269: proxy: add local connection limit to ListenConnections 39a10ce89 proxy: add local connection limit to ListenConnections() 43172f52d test: add dedicated ListenConnections coverage 033f81219 doc/version: Bump version 11 > 12 463d073cb test: rename vBool to vector_bool 16bf05dea Merge bitcoin-core/libmultiprocess#302: refactor: rename EventLoop::m_num_clients to m_num_refs dd537da9e Merge bitcoin-core/libmultiprocess#301: test: recursive async IPC calls and cleanups 400291de0 Merge bitcoin-core/libmultiprocess#299: ci: remove libevent from Core CIs 092be515a Merge bitcoin-core/libmultiprocess#285: Add ReadList helper 5b617880c Merge bitcoin-core/libmultiprocess#283: Add `makePool` method on `ThreadMap` d49983041 refactor: rename EventLoop::m_num_clients to m_num_refs 6450345c9 type: reserve first when reading std::unordered_set 4d0f8db5f proxy: add ReadList helper and dedup map/set/vector read handlers 0e49d9118 Add `makePool` method on `ThreadMap` 5519f7f94 test: recursive async IPC calls a29ceff40 ci: remove libevent from Core CIs 85df23384 test: add mapStringInt to foo.capnp to cover map serialization and deserialization fa2c56ec2 ci: Bump channel to nixos-26.05 8412fcdc6 Merge bitcoin-core/libmultiprocess#295: Mark Waiter m_cv as guarded by m_mutex 1593ee2d1 Merge bitcoin-core/libmultiprocess#294: test: Add passDouble smoke test 9885d7dd3 Merge bitcoin-core/libmultiprocess#286: proxy-client: fix TSan data race in clientDestroy fa35501c4 Mark Waiter m_cv as guarded by m_mutex faaedb11f test: Add passDouble smoke test 733c64318 Merge bitcoin-core/libmultiprocess#292: type-number: fix clang-tidy modernize-use-nullptr 9cc3479ab Merge bitcoin-core/libmultiprocess#291: cmake: Add `mp_headers` custom target 201abd9e3 Merge bitcoin-core/libmultiprocess#289: cmake: make target_capnp_sources use CURRENT dirs 99820c8ae Merge bitcoin-core/libmultiprocess#279: doc: Add comments to FIELD_* constants in proxy.h 73b985540 Merge bitcoin-core/libmultiprocess#278: doc: Fix and expand design.md e7e91b2e2 Merge bitcoin-core/libmultiprocess#277: Add std::unordered_set support and a helper BuildList to dedup list build handlers 91a951f59 tidy fix: modernize-use-nullptr 16362f42d cmake: Add `mp_headers` custom target 615a94fe3 cmake: document ONLY_CAPNP option in target_capnp_sources 90982f75c mpgen: iwyu changes required by previous commit 25bb3e67f proxy-client: fix TSan data race in clientDestroy 620f297f3 cmake: make target_capnp_sources use CURRENT dirs 9de4b885a test: use camelCase + $Proxy.name for FooStruct fields 011b91793 type: add std::unordered_set support 20d19b964 proxy: add BuildList helper and dedup map/set/vector build handlers e863c6cdf doc: Add comments to FIELD_* constants in proxy.h 18db0ab95 doc: Fix and expand design.md git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 2dba130478ab71a745fe96c5726719fe357e6d17
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.