-
Notifications
You must be signed in to change notification settings - Fork 37.7k
consensus: don't call GetBlockPos in ReadBlockFromDisk without cs_main lock #22895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK 350e034
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 350e034.
Shouldn't there be a lock annotation to prevent this in the future? |
Proposed in #22932. |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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 350e034 |
Github-Pull: bitcoin#22895 Rebased-From: 350e034 (diff-minimised)
Github-Pull: bitcoin#22895 Rebased-From: 350e034
Github-Pull: bitcoin#22895 Rebased-From: 350e034
Github-Pull: bitcoin#22895 Rebased-From: 350e034 (diff-minimised)
…DataPos/nUndoPos by cs_main 6ea5682 Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main (Jon Atack) 5d59ae0 Remove/inline ReadRawBlockFromDisk(block_data, pindex, message_start) (Hennadii Stepanov) eaeeb88 Require IsBlockPruned() to hold mutex cs_main (Jon Atack) ca47b00 Require CBlockIndex::IsValid() to hold cs_main (Vasil Dimov) e9f3aa5 Require CBlockIndex::RaiseValidity() to hold cs_main (Vasil Dimov) 8ef457c Require CBlockIndex::IsAssumedValid() to hold cs_main (Vasil Dimov) 5723934 Require CBlockIndex::GetUndoPos() to hold mutex cs_main (Jon Atack) 2e557ce Require WriteUndoDataForBlock() to hold mutex cs_main (Jon Atack) 6fd4341 Require CBlockIndex::GetBlockPos() to hold mutex cs_main (Jon Atack) Pull request description: Issues: - `CBlockIndex` member functions `GetBlockPos()`, `GetUndoPos()`, `IsAssumedValid()`, `RaiseValidity()`, and `IsValid()` and block storage functions `WriteUndoDataForBlock()` and `IsBlockPruned()` are missing thread safety lock annotations to help ensure that they are called with mutex cs_main to avoid bugs like #22895. Doing this also enables the next step: - `CBlockIndex::nStatus` may be racy, i.e. potentially accessed by multiple threads, see #17161. A solution is to guard it by cs_main, along with fellow data members `nFile`, `nDataPos` and `nUndoPos`. This pull: - adds thread safety lock annotations for the functions listed above - guards `CBlockIndex::nStatus`, `nFile`, `nDataPos` and `nUndoPos` by cs_main How to review and test: - debug build with clang and verify there are no `-Wthread-safety-analysis` warnings - review the code to verify each annotation or lock is necessary and sensible, or if any are missing - look for whether taking a lock can be replaced by a lock annotation instead - for more information about Clang thread safety analysis, see - https://clang.llvm.org/docs/ThreadSafetyAnalysis.html - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lockingmutex-usage-notes - https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization Mitigates/potentially closes #17161. ACKs for top commit: laanwj: Code review ACK 6ea5682 Tree-SHA512: 3ebf429c8623c51f944a7245a2e48d2aa088dec4c4914b40aa6049e89856c1ee8586f6e2e3b65195190566637a33004468b51a781e61a082248748015167569b
Github-Pull: bitcoin#22895 Rebased-From: 350e034
Github-Pull: bitcoin#22895 Rebased-From: 350e034
269553f test: Call ceildiv helper with integer (Martin Zumsande) 2f60fc6 ci: Replace soon EOL hirsute with jammy (MarcoFalke) 801b0f0 build: patch qt to explicitly define previously implicit header include (Kittywhiskers Van Gogh) c768bfa tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow) f66bc42 tests: Test for assertion when feerate is rounded down (Andrew Chow) bd7e08e fees: Always round up fee calculated from a feerate (Andrew Chow) 227ae65 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner) 282863a refactor: include a missing <limits> header in fs.cpp (Joan Karadimov) 7febe4f consensus: don't call GetBlockPos in ReadBlockFromDisk without lock (Jon Atack) c671c6f the result of CWallet::IsHDEnabled() was initialized with true. (Saibato) a5a1538 build, qt: Fix typo in QtInputSupport check (Hennadii Stepanov) c95b188 system: skip trying to set the locale on NetBSD (fanquake) c1cdedd guix: Fix powerpc64(le) dynamic linker name (Carl Dong) 92d44ff doc: Add 23061 release notes (MarcoFalke) db76db7 Fix (inverse) meaning of -persistmempool (MarcoFalke) 85c78e0 build: Restrict check for CRC32C intrinsic to aarch64 (W. J. van der Laan) Pull request description: Collecting backports for the 22.1 release. Currently: * #23045 * #23061 * #23148 * #22390 * #22820 * #22781 * #22895 * #23335 * #23333 * #22949 * #23580 * #23504 * #24239 ACKs for top commit: achow101: ACK 269553f Tree-SHA512: b3a57ea241be7a83488eeb032276f4cf82a0987aada906a82f94a20c4acf9f2397708249dcecbe1c7575e70d09c60b835233d4718af4013c7bc58896c618274c
Commit ccd8ef6 "Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock" in #11281 moved the cs_main lock from caller to
ReadBlockFromDisk()
for callingCBlockIndex::GetBlockPos()
, but the second invocation doesn't have the lock, and IIUC there is no guarantee the compiler can know if state has changed.Use the
blockPos
local variable instead, rename it toblock_pos
, and make it const.