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
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