type-context.h: Extent cancel_mutex lock to prevent theoretical race#245
Merged
ryanofsky merged 2 commits intobitcoin-core:masterfrom Feb 25, 2026
Merged
type-context.h: Extent cancel_mutex lock to prevent theoretical race#245ryanofsky merged 2 commits intobitcoin-core:masterfrom
ryanofsky merged 2 commits intobitcoin-core:masterfrom
Conversation
As pointed out by janb84 in bitcoin/bitcoin#34422 (comment) it makes sense for the on_cancel callback to lock cancel_mutex while it is assigning request_canceled = true. The lock and assigment were introduced in bitcoin-core#240 and in an earlier version of that PR, request_canceled was a std::atomic and the assignment happened before the lock was acquired instead of after, so it was ok for the lock to be unnamed and immediately released after being acquired. But in the final verion of bitcoin-core#240 request_canceled is an ordinary non-atomic bool, and it should be assigned true with the lock held to prevent a theoretical race condition where capn'proto event loop cancels the request before the execution thread runs, and the execution thread sees the old request_canceled = false value and then unsafely accesses deleted parameters. The request being canceled so quickly and parameters being accessed so slowly, and stale request_canceled value being read even after the execution thread has the cancel_mutex lock should be very unlikely to occur in practice, but could happen in theory and is good to fix.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
Member
|
ACK ef96a5b |
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Feb 25, 2026
…451f 1868a84451f Merge bitcoin-core/libmultiprocess#245: type-context.h: Extent cancel_mutex lock to prevent theoretical race fd4a90d3103 Merge bitcoin-core/libmultiprocess#244: ci: suppress two tidy lint issues 16dfc368640 ci: avoid bugprone-unused-return-value lint in test dacd5eda464 ci: suppress nontrivial-threadlocal lint in proxy.cpp ef96a5b2be2 doc: Comment cleanups after bitcoin#240 e0f1cd76219 type-context.h: Extent cancel_mutex lock to prevent theoretical race 290702c74ce Merge bitcoin-core/libmultiprocess#240: Avoid errors from asynchronous (non-c++) clients 3a69d4755af Merge bitcoin-core/libmultiprocess#241: doc: Bump version number v7 -> v8 0174450ca2e Prevent crash on unclean disconnect if abandoned IPC call returns interface pointer ddb5f74196f Allow simultaneous calls on same Context.thread c4762c7b513 refactor: Add ProxyServer<Thread>::post() method 0ade1b40ac5 doc: Bump version number git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1868a84451fe1b6a00116375a5f717230bb2533e
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Feb 25, 2026
…451f 1868a84451f Merge bitcoin-core/libmultiprocess#245: type-context.h: Extent cancel_mutex lock to prevent theoretical race fd4a90d3103 Merge bitcoin-core/libmultiprocess#244: ci: suppress two tidy lint issues 16dfc368640 ci: avoid bugprone-unused-return-value lint in test dacd5eda464 ci: suppress nontrivial-threadlocal lint in proxy.cpp ef96a5b2be2 doc: Comment cleanups after bitcoin#240 e0f1cd76219 type-context.h: Extent cancel_mutex lock to prevent theoretical race 290702c74ce Merge bitcoin-core/libmultiprocess#240: Avoid errors from asynchronous (non-c++) clients 3a69d4755af Merge bitcoin-core/libmultiprocess#241: doc: Bump version number v7 -> v8 0174450ca2e Prevent crash on unclean disconnect if abandoned IPC call returns interface pointer ddb5f74196f Allow simultaneous calls on same Context.thread c4762c7b513 refactor: Add ProxyServer<Thread>::post() method 0ade1b40ac5 doc: Bump version number git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1868a84451fe1b6a00116375a5f717230bb2533e
fanquake
added a commit
to bitcoin/bitcoin
that referenced
this pull request
Mar 3, 2026
…ust IPC client 8fe91f3 test: Updates needed after bitcoin-core/libmultiprocess#240 (Ryan Ofsky) b7ca3bf Squashed 'src/ipc/libmultiprocess/' changes from 1fc65008f7d..1868a84451f (Ryan Ofsky) 1fea3ba ipc, test: Add tests for unclean disconnect and thread busy behavior (Ryan Ofsky) Pull request description: Includes: - bitcoin-core/libmultiprocess#241 - bitcoin-core/libmultiprocess#240 - bitcoin-core/libmultiprocess#244 - bitcoin-core/libmultiprocess#245 The main change is bitcoin-core/libmultiprocess#240 which fixes issues with asynchronous requests (#33923) and unclean disconnects (#34250) that happen with the rust mining client. It also adds tests for these fixes which had some previous review in #34284 (that PR was closed to simplify dependencies between PRs). The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#subtrees) and [lint instructions](https://github.com/bitcoin/bitcoin/tree/master/test/lint#git-subtree-checksh) Resolves #33923 and #34250 ACKs for top commit: Sjors: re-ACK 8fe91f3 janb84: reACK 8fe91f3 Eunovo: ACK 8fe91f3 Tree-SHA512: 7e8923610502ebd8603bbea703f82178ab9e956874d394da3451f5268afda2b964d0eeb399a74d49c4123e728a14c27c0296118577a6063ff03b2b8203a257ce
enirox001
pushed a commit
to enirox001/bitcoin
that referenced
this pull request
Mar 4, 2026
…451f 1868a84451f Merge bitcoin-core/libmultiprocess#245: type-context.h: Extent cancel_mutex lock to prevent theoretical race fd4a90d3103 Merge bitcoin-core/libmultiprocess#244: ci: suppress two tidy lint issues 16dfc368640 ci: avoid bugprone-unused-return-value lint in test dacd5eda464 ci: suppress nontrivial-threadlocal lint in proxy.cpp ef96a5b2be2 doc: Comment cleanups after bitcoin#240 e0f1cd76219 type-context.h: Extent cancel_mutex lock to prevent theoretical race 290702c74ce Merge bitcoin-core/libmultiprocess#240: Avoid errors from asynchronous (non-c++) clients 3a69d4755af Merge bitcoin-core/libmultiprocess#241: doc: Bump version number v7 -> v8 0174450ca2e Prevent crash on unclean disconnect if abandoned IPC call returns interface pointer ddb5f74196f Allow simultaneous calls on same Context.thread c4762c7b513 refactor: Add ProxyServer<Thread>::post() method 0ade1b40ac5 doc: Bump version number git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1868a84451fe1b6a00116375a5f717230bb2533e
chriszeng1010
pushed a commit
to chriszeng1010/bitcoin
that referenced
this pull request
Mar 17, 2026
…451f 1868a84451f Merge bitcoin-core/libmultiprocess#245: type-context.h: Extent cancel_mutex lock to prevent theoretical race fd4a90d3103 Merge bitcoin-core/libmultiprocess#244: ci: suppress two tidy lint issues 16dfc368640 ci: avoid bugprone-unused-return-value lint in test dacd5eda464 ci: suppress nontrivial-threadlocal lint in proxy.cpp ef96a5b2be2 doc: Comment cleanups after bitcoin#240 e0f1cd76219 type-context.h: Extent cancel_mutex lock to prevent theoretical race 290702c74ce Merge bitcoin-core/libmultiprocess#240: Avoid errors from asynchronous (non-c++) clients 3a69d4755af Merge bitcoin-core/libmultiprocess#241: doc: Bump version number v7 -> v8 0174450ca2e Prevent crash on unclean disconnect if abandoned IPC call returns interface pointer ddb5f74196f Allow simultaneous calls on same Context.thread c4762c7b513 refactor: Add ProxyServer<Thread>::post() method 0ade1b40ac5 doc: Bump version number git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 1868a84451fe1b6a00116375a5f717230bb2533e
Sjors
added a commit
to stratum-mining/sv2-tp
that referenced
this pull request
Apr 15, 2026
3edbe8f6 Merge bitcoin-core/libmultiprocess#268: Use throwRecoverableException instead of raw throw for stored exceptions 23be44b0 Use throwRecoverableException instead of raw throw for stored exceptions 75c2a276 Merge bitcoin-core/libmultiprocess#266: test: increase spawn test child timeout to 30 seconds 8b5f8053 Merge bitcoin-core/libmultiprocess#267: doc: Bump version 9 > 10 cc0b23fc test: increase spawn test child timeout to 30 seconds 050f878d doc: Improve versions.md descriptions and formatting c6a288a8 doc: Bump version 9 > 10 70f632bd Merge bitcoin-core/libmultiprocess#265: ci: set LC_ALL in shell scripts 8e8e5642 Merge bitcoin-core/libmultiprocess#249: fixes for race conditions on disconnects 05d34cc2 ci: set LC_ALL in shell scripts e606fd84 Merge bitcoin-core/libmultiprocess#264: ci: reduce nproc multipliers ff0eed1b refactor: Use loop variable in type-context.h ff1d8ba1 refactor: Move type-context.h getParams() call closer to use 1dbc59a4 race fix: m_on_cancel called after request finishes 1643d05b test: m_on_cancel called after request finishes f5509a31 race fix: getParams() called after request cancel 4a60c39f test: getParams() called after request cancel f11ec29e race fix: worker thread destroyed before it is initialized a1d64334 test: worker thread destroyed before it is initialized 33602338 ci: reduce nproc multipliers b090beb9 Merge bitcoin-core/libmultiprocess#256: ci: cache gnu32 nix store be862281 ci: cache gnu32 nix store 975270b6 Merge bitcoin-core/libmultiprocess#263: ci: bump timeout factor to 40 09f10e5a ci: bump timeout factor to 40 db8f76ad Merge bitcoin-core/libmultiprocess#253: ci: run some Bitcoin Core CI jobs 55a9b557 ci: set Bitcoin Core CI test repetition fb0fc84d ci: add TSan job with instrumented libc++ 0f29c387 ci: add Bitcoin Core IPC tests (ASan + macOS) 3f643203 Merge bitcoin-core/libmultiprocess#262: ci: enable clang-tidy in macOS job, use nullptr cd9f8bdc Merge bitcoin-core/libmultiprocess#258: log: add socket connected info message and demote destroy logs to debug b5d6258a Merge bitcoin-core/libmultiprocess#255: fix: use unsigned char cast and sizeof in LogEscape escape sequence d94688e2 Merge bitcoin-core/libmultiprocess#251: Improved CustomBuildField for std::optional in IPC/libmultiprocess a9499fad mp: use nullptr with pthread_threadid_np f499e378 ci: enable clang-tidy in macOS job 98f13521 log: add socket connected info message and demote destroy logs to debug 554a481e fix: use unsigned char cast and sizeof in LogEscape escape sequence 1977b9f3 Use std::forward in CustomBuildField for std::optional to allow move semantics, resolves FIXME 22bec918 Merge bitcoin-core/libmultiprocess#247: type-map: Work around LLVM 22 "out of bounds index" error 8a5e3ae6 Merge bitcoin-core/libmultiprocess#242: proxy-types: add CustomHasField hook to map Cap'n Proto values to null C++ values e8d35246 Merge bitcoin-core/libmultiprocess#246: doc: Bump version 8 > 9 97d87705 proxy-types: add CustomHasField hook for nullable decode paths 8c2f1025 refactor: add missing includes to mp/type-data.h b1638ace doc: Bump version 8 > 9 f61af487 type-map: Work around LLVM 22 "out of bounds index" error 1868a844 Merge bitcoin-core/libmultiprocess#245: type-context.h: Extent cancel_mutex lock to prevent theoretical race fd4a90d3 Merge bitcoin-core/libmultiprocess#244: ci: suppress two tidy lint issues 16dfc368 ci: avoid bugprone-unused-return-value lint in test dacd5eda ci: suppress nontrivial-threadlocal lint in proxy.cpp ef96a5b2 doc: Comment cleanups after #240 e0f1cd76 type-context.h: Extent cancel_mutex lock to prevent theoretical race 290702c7 Merge bitcoin-core/libmultiprocess#240: Avoid errors from asynchronous (non-c++) clients 0174450c Prevent crash on unclean disconnect if abandoned IPC call returns interface pointer ddb5f741 Allow simultaneous calls on same Context.thread c4762c7b refactor: Add ProxyServer<Thread>::post() method git-subtree-dir: src/ipc/libmultiprocess git-subtree-split: 3edbe8f67c182dde91c0050065d79ae268722489
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
3 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.
This is a followup to #240 that fixes a theoretical race condition in that PR pointed out by janb bitcoin/bitcoin#34422 (comment). Details are in the commit message. There is also an additional commit fixing up some documentation added in that PR