type-context.h: Extent cancel_mutex lock to prevent theoretical race#245
Open
ryanofsky wants to merge 2 commits intobitcoin-core:masterfrom
Open
type-context.h: Extent cancel_mutex lock to prevent theoretical race#245ryanofsky wants to merge 2 commits intobitcoin-core:masterfrom
ryanofsky wants to merge 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. |
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.
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