From dcac626d18b000d2d26a4c51add29ca58b9ff77d Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Mon, 23 Jan 2023 17:52:38 +0100 Subject: [PATCH] Guard CBlockIndex::nStatus/nFile/nDataPos/nUndoPos by cs_main Summary: > CBlockIndex::nStatus may be racy, i.e. potentially accessed by multiple threads, see [[https://github.com/bitcoin/bitcoin/pull/17161 | core#17161]]. A solution is to guard it by cs_main, along with fellow data members nFile, nDataPos and nUndoPos. Co-authored-by: Vasil Dimov This concludes backport of [[https://github.com/bitcoin/bitcoin/pull/22932 | core#22932]] and [[https://github.com/bitcoin/bitcoin/pull/24197 | core#24197]] https://github.com/bitcoin/bitcoin/pull/22932/commits/6ea56827842b9b2bd730edc38f3a7b1f46f6247b https://github.com/bitcoin/bitcoin/pull/24197/commits/20276ca5d124285bdd1bda4cd777ca186b378555 Depends on D13037 Test Plan: With clang and DEBUG: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D13038 --- src/avalanche/test/processor_tests.cpp | 1 + src/blockindex.h | 8 ++++---- src/chain.h | 1 + src/rpc/avalanche.cpp | 3 ++- src/rpc/blockchain.cpp | 2 +- src/rpc/rawtransaction.cpp | 3 ++- src/test/blockindex_tests.cpp | 5 ++--- src/test/interfaces_tests.cpp | 1 + src/txdb.cpp | 1 + src/txdb.h | 4 +++- 10 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/avalanche/test/processor_tests.cpp b/src/avalanche/test/processor_tests.cpp index a1c5a3c1ad..876bc0bc7b 100644 --- a/src/avalanche/test/processor_tests.cpp +++ b/src/avalanche/test/processor_tests.cpp @@ -285,6 +285,7 @@ struct BlockProvider { } void invalidateItem(CBlockIndex *pindex) { + LOCK(::cs_main); pindex->nStatus = pindex->nStatus.withFailed(); } diff --git a/src/blockindex.h b/src/blockindex.h index 2111c9556d..0f8d08f019 100644 --- a/src/blockindex.h +++ b/src/blockindex.h @@ -39,13 +39,13 @@ class CBlockIndex { int nHeight{0}; //! Which # file this block is stored in (blk?????.dat) - int nFile{0}; + int nFile GUARDED_BY(::cs_main){0}; //! Byte offset within blk?????.dat where this block's data is stored - unsigned int nDataPos{0}; + unsigned int nDataPos GUARDED_BY(::cs_main){0}; //! Byte offset within rev?????.dat where this block's undo data is stored - unsigned int nUndoPos{0}; + unsigned int nUndoPos GUARDED_BY(::cs_main){0}; //! (memory only) Total amount of work (expected number of hashes) in the //! chain up to and including this block @@ -85,7 +85,7 @@ class CBlockIndex { public: //! Verification status of this block. See enum BlockStatus - BlockStatus nStatus{}; + BlockStatus nStatus GUARDED_BY(::cs_main){}; //! block header int32_t nVersion{0}; diff --git a/src/chain.h b/src/chain.h index c46a57226c..efce775b29 100644 --- a/src/chain.h +++ b/src/chain.h @@ -81,6 +81,7 @@ class CDiskBlockIndex : public CBlockIndex { } SERIALIZE_METHODS(CDiskBlockIndex, obj) { + LOCK(::cs_main); int _nVersion = s.GetVersion(); if (!(s.GetType() & SER_GETHASH)) { READWRITE(VARINT_MODE(_nVersion, VarIntMode::NONNEGATIVE_SIGNED)); diff --git a/src/rpc/avalanche.cpp b/src/rpc/avalanche.cpp index a07cd0d204..ae40f132b0 100644 --- a/src/rpc/avalanche.cpp +++ b/src/rpc/avalanche.cpp @@ -1184,7 +1184,8 @@ static RPCHelpMan isfinaltransaction() { if (!tx) { std::string errmsg; if (pindex) { - if (!pindex->nStatus.hasData()) { + if (WITH_LOCK(::cs_main, + return !pindex->nStatus.hasData())) { throw JSONRPCError( RPC_MISC_ERROR, "Block data not downloaded yet."); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 9363c6eed4..3b9a2b057c 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -928,7 +928,7 @@ static RPCHelpMan getblockfrompeer() { throw JSONRPCError(RPC_MISC_ERROR, "Block header missing"); } - if (index->nStatus.hasData()) { + if (WITH_LOCK(::cs_main, return index->nStatus.hasData())) { throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded"); } diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 5db47a06d2..09e3c86441 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -251,7 +251,8 @@ static RPCHelpMan getrawtransaction() { if (!tx) { std::string errmsg; if (blockindex) { - if (!blockindex->nStatus.hasData()) { + if (WITH_LOCK(::cs_main, + return !blockindex->nStatus.hasData())) { throw JSONRPCError(RPC_MISC_ERROR, "Block not available"); } diff --git a/src/test/blockindex_tests.cpp b/src/test/blockindex_tests.cpp index 33348eef77..d89cdeb761 100644 --- a/src/test/blockindex_tests.cpp +++ b/src/test/blockindex_tests.cpp @@ -44,6 +44,7 @@ BOOST_AUTO_TEST_CASE(get_disk_positions) { BlockValidity::UNKNOWN, BlockValidity::RESERVED, BlockValidity::TREE, BlockValidity::TRANSACTIONS, BlockValidity::CHAIN, BlockValidity::SCRIPTS}; + LOCK(cs_main); for (BlockValidity validity : validityValues) { // Test against all combinations of data and undo flags for (int flags = 0; flags <= 0x03; flags++) { @@ -68,7 +69,6 @@ BOOST_AUTO_TEST_CASE(get_disk_positions) { } // Data and undo positions should be unmodified - LOCK(::cs_main); FlatFilePos dataPosition = index.GetBlockPos(); if (flags & 0x01) { BOOST_CHECK(dataPosition.nFile == expectedFile); @@ -269,6 +269,7 @@ BOOST_AUTO_TEST_CASE(index_validity_tests) { BlockValidity::TREE, BlockValidity::TRANSACTIONS, BlockValidity::CHAIN, BlockValidity::SCRIPTS}; std::set boolValues = {false, true}; + LOCK(::cs_main); for (BlockValidity validity : validityValues) { for (bool withFailed : boolValues) { for (bool withFailedParent : boolValues) { @@ -278,8 +279,6 @@ BOOST_AUTO_TEST_CASE(index_validity_tests) { .withFailedParent(withFailedParent); for (BlockValidity validUpTo : validityValues) { - LOCK(::cs_main); - // Test isValidity() bool isValid = index.IsValid(validUpTo); if (validUpTo <= validity && !withFailed && diff --git a/src/test/interfaces_tests.cpp b/src/test/interfaces_tests.cpp index 04891311c9..52678cf3fe 100644 --- a/src/test/interfaces_tests.cpp +++ b/src/test/interfaces_tests.cpp @@ -150,6 +150,7 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor) { } BOOST_AUTO_TEST_CASE(hasBlocks) { + LOCK(::cs_main); auto &chain = m_node.chain; const CChain &active = Assert(m_node.chainman)->ActiveChain(); diff --git a/src/txdb.cpp b/src/txdb.cpp index ff20c38064..368a4508bd 100644 --- a/src/txdb.cpp +++ b/src/txdb.cpp @@ -300,6 +300,7 @@ bool CBlockTreeDB::ReadFlag(const std::string &name, bool &fValue) { bool CBlockTreeDB::LoadBlockIndexGuts( const Consensus::Params ¶ms, std::function insertBlockIndex) { + AssertLockHeld(::cs_main); std::unique_ptr pcursor(NewIterator()); uint64_t version = 0; diff --git a/src/txdb.h b/src/txdb.h index 4c14977d0c..9c2ff8b422 100644 --- a/src/txdb.h +++ b/src/txdb.h @@ -119,7 +119,9 @@ class CBlockTreeDB : public CDBWrapper { bool ReadFlag(const std::string &name, bool &fValue); bool LoadBlockIndexGuts( const Consensus::Params ¶ms, - std::function insertBlockIndex); + std::function insertBlockIndex) + EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + ; //! Attempt to update from an older database format. //! Returns whether an error occurred.