From fe2d6e25e03a9f0548ca54c7257fa19651397c48 Mon Sep 17 00:00:00 2001 From: optout <13562139+optout21@users.noreply.github.com> Date: Mon, 26 Jan 2026 13:47:37 +0100 Subject: [PATCH] Change CChain::Contains() to take reference The `CChain::Contains()` method dereferences its input without checking, potentially resulting in nullptr-dereference if invoked with `nullptr`. To avoid this possibility, its input is changed to a reference instead. Call sites are adapted accoringly, extra nullptr-check is added as needed. --- src/chain.cpp | 2 +- src/chain.h | 7 ++++--- src/init.cpp | 5 ++++- src/kernel/bitcoinkernel.cpp | 2 +- src/net_processing.cpp | 11 ++++++----- src/rest.cpp | 4 ++-- src/rpc/blockchain.cpp | 8 ++++---- src/rpc/rawtransaction.cpp | 4 ++-- src/rpc/txoutproof.cpp | 2 +- src/test/chain_tests.cpp | 7 +++---- src/txmempool.cpp | 2 +- src/validation.cpp | 24 ++++++++++++------------ 12 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/chain.cpp b/src/chain.cpp index 46a51ac8bd9..958872e5ff6 100644 --- a/src/chain.cpp +++ b/src/chain.cpp @@ -53,7 +53,7 @@ const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const { } if (pindex->nHeight > Height()) pindex = pindex->GetAncestor(Height()); - while (pindex && !Contains(pindex)) + while (pindex && !Contains(*pindex)) pindex = pindex->pprev; return pindex; } diff --git a/src/chain.h b/src/chain.h index c27829208c4..7c3a369cb5c 100644 --- a/src/chain.h +++ b/src/chain.h @@ -407,15 +407,16 @@ public: } /** Efficiently check whether a block is present in this chain. */ - bool Contains(const CBlockIndex* pindex) const + bool Contains(const CBlockIndex& index) const { - return (*this)[pindex->nHeight] == pindex; + return (*this)[index.nHeight] == &index; } /** Find the successor of a block in this chain, or nullptr if the given index is not found or is the tip. */ CBlockIndex* Next(const CBlockIndex* pindex) const { - if (Contains(pindex)) + // Note: the next commit makes sure that the input parameter of Next() cannot be nullptr + if (Contains(*pindex)) return (*this)[pindex->nHeight + 1]; else return nullptr; diff --git a/src/init.cpp b/src/init.cpp index 407ee0fefff..6642c87f41d 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -2370,7 +2370,10 @@ bool StartIndexBackgroundSync(NodeContext& node) { LOCK(::cs_main); pindex = chainman.m_blockman.LookupBlockIndex(summary.best_block_hash); - if (!index_chain.Contains(pindex)) { + if (!pindex) { + LogWarning("Failed to find block manager entry for best block %s from %s, falling back to genesis for index sync", + summary.best_block_hash.ToString(), summary.name); + } else if (!index_chain.Contains(*pindex)) { pindex = index_chain.FindFork(pindex); } } diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp index dc03be71461..34dc9ce243f 100644 --- a/src/kernel/bitcoinkernel.cpp +++ b/src/kernel/bitcoinkernel.cpp @@ -1367,7 +1367,7 @@ const btck_BlockTreeEntry* btck_chain_get_by_height(const btck_Chain* chain, int int btck_chain_contains(const btck_Chain* chain, const btck_BlockTreeEntry* entry) { LOCK(::cs_main); - return btck_Chain::get(chain).Contains(&btck_BlockTreeEntry::get(entry)) ? 1 : 0; + return btck_Chain::get(chain).Contains(btck_BlockTreeEntry::get(entry)) ? 1 : 0; } btck_BlockHeader* btck_block_header_create(const void* raw_block_header, size_t raw_block_header_len) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c7061d52348..4eed2e124c8 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1503,7 +1503,7 @@ void PeerManagerImpl::FindNextBlocks(std::vector& vBlocks, c return; } - if (pindex->nStatus & BLOCK_HAVE_DATA || (activeChain && activeChain->Contains(pindex))) { + if (pindex->nStatus & BLOCK_HAVE_DATA || (activeChain && activeChain->Contains(*pindex))) { if (activeChain && pindex->HaveNumChainTxs()) { state->pindexLastCommonBlock = pindex; } @@ -1952,7 +1952,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex& block_index) { AssertLockHeld(cs_main); - if (m_chainman.ActiveChain().Contains(&block_index)) return true; + if (m_chainman.ActiveChain().Contains(block_index)) return true; return block_index.IsValid(BLOCK_VALID_SCRIPTS) && (m_chainman.m_best_header != nullptr) && (m_chainman.m_best_header->GetBlockTime() - block_index.GetBlockTime() < STALE_RELAY_AGE_LIMIT) && (GetBlockProofEquivalentTime(*m_chainman.m_best_header, block_index, *m_chainman.m_best_header, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT); @@ -2815,7 +2815,7 @@ bool PeerManagerImpl::IsAncestorOfBestHeaderOrTip(const CBlockIndex* header) return false; } else if (m_chainman.m_best_header != nullptr && header == m_chainman.m_best_header->GetAncestor(header->nHeight)) { return true; - } else if (m_chainman.ActiveChain().Contains(header)) { + } else if (m_chainman.ActiveChain().Contains(*header)) { return true; } return false; @@ -2849,7 +2849,7 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c std::vector vToFetch; const CBlockIndex* pindexWalk{&last_header}; // Calculate all the blocks we'd need to switch to last_header, up to a limit. - while (pindexWalk && !m_chainman.ActiveChain().Contains(pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { + while (pindexWalk && !m_chainman.ActiveChain().Contains(*pindexWalk) && vToFetch.size() <= MAX_BLOCKS_IN_TRANSIT_PER_PEER) { if (!(pindexWalk->nStatus & BLOCK_HAVE_DATA) && !IsBlockRequested(pindexWalk->GetBlockHash()) && (!DeploymentActiveAt(*pindexWalk, m_chainman, Consensus::DEPLOYMENT_SEGWIT) || CanServeWitnesses(peer))) { @@ -2862,7 +2862,8 @@ void PeerManagerImpl::HeadersDirectFetchBlocks(CNode& pfrom, const Peer& peer, c // very large reorg at a time we think we're close to caught up to // the main chain -- this shouldn't really happen. Bail out on the // direct fetch and rely on parallel download instead. - if (!m_chainman.ActiveChain().Contains(pindexWalk)) { + // Common ancestor must exist (genesis). + if (!m_chainman.ActiveChain().Contains(*Assert(pindexWalk))) { LogDebug(BCLog::NET, "Large reorg, won't direct fetch to %s (%d)\n", last_header.GetBlockHash().ToString(), last_header.nHeight); diff --git a/src/rest.cpp b/src/rest.cpp index 53dd3d16119..25fe5d8e80c 100644 --- a/src/rest.cpp +++ b/src/rest.cpp @@ -225,7 +225,7 @@ static bool rest_headers(const std::any& context, CChain& active_chain = chainman.ActiveChain(); tip = active_chain.Tip(); const CBlockIndex* pindex{chainman.m_blockman.LookupBlockIndex(*hash)}; - while (pindex != nullptr && active_chain.Contains(pindex)) { + while (pindex != nullptr && active_chain.Contains(*pindex)) { headers.push_back(pindex); if (headers.size() == *parsed_count) { break; @@ -552,7 +552,7 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const LOCK(cs_main); CChain& active_chain = chainman.ActiveChain(); const CBlockIndex* pindex{chainman.m_blockman.LookupBlockIndex(*block_hash)}; - while (pindex != nullptr && active_chain.Contains(pindex)) { + while (pindex != nullptr && active_chain.Contains(*pindex)) { headers.push_back(pindex); if (headers.size() == *parsed_count) break; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 0821ebba18a..956f7066ae0 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1631,7 +1631,7 @@ static RPCMethod getchaintips() std::set setPrevs; for (const auto& [_, block_index] : chainman.BlockIndex()) { - if (!active_chain.Contains(&block_index)) { + if (!active_chain.Contains(block_index)) { setOrphans.insert(&block_index); setPrevs.insert(block_index.pprev); } @@ -1657,7 +1657,7 @@ static RPCMethod getchaintips() obj.pushKV("branchlen", branchLen); std::string status; - if (active_chain.Contains(block)) { + if (active_chain.Contains(*block)) { // This block is part of the currently active chain. status = "active"; } else if (block->nStatus & BLOCK_FAILED_VALID) { @@ -1866,7 +1866,7 @@ static RPCMethod getchaintxstats() if (!pindex) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); } - if (!chainman.ActiveChain().Contains(pindex)) { + if (!chainman.ActiveChain().Contains(*pindex)) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Block is not in main chain"); } } @@ -2804,7 +2804,7 @@ static RPCMethod getdescriptoractivity() if (!pindex) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found"); } - if (!chainman.ActiveChain().Contains(pindex)) { + if (!chainman.ActiveChain().Contains(*pindex)) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Block is not in main chain"); } blockindexes_sorted.insert(pindex); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 74652c17421..a0f93f0c79c 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -73,7 +73,7 @@ static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& entry.pushKV("blockhash", hashBlock.GetHex()); const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock); if (pindex) { - if (active_chainstate.m_chain.Contains(pindex)) { + if (active_chainstate.m_chain.Contains(*pindex)) { entry.pushKV("confirmations", 1 + active_chainstate.m_chain.Height() - pindex->nHeight); entry.pushKV("time", pindex->GetBlockTime()); entry.pushKV("blocktime", pindex->GetBlockTime()); @@ -336,7 +336,7 @@ static RPCMethod getrawtransaction() UniValue result(UniValue::VOBJ); if (blockindex) { LOCK(cs_main); - result.pushKV("in_active_chain", chainman.ActiveChain().Contains(blockindex)); + result.pushKV("in_active_chain", chainman.ActiveChain().Contains(*blockindex)); } // If request is verbosity >= 1 but no blockhash was given, then look up the blockindex if (request.params[2].isNull()) { diff --git a/src/rpc/txoutproof.cpp b/src/rpc/txoutproof.cpp index f68f53f665d..628825fc80e 100644 --- a/src/rpc/txoutproof.cpp +++ b/src/rpc/txoutproof.cpp @@ -158,7 +158,7 @@ static RPCMethod verifytxoutproof() LOCK(cs_main); const CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(merkleBlock.header.GetHash()); - if (!pindex || !chainman.ActiveChain().Contains(pindex) || pindex->nTx == 0) { + if (!pindex || !chainman.ActiveChain().Contains(*pindex) || pindex->nTx == 0) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain"); } diff --git a/src/test/chain_tests.cpp b/src/test/chain_tests.cpp index daae8f3f989..c85439a4d5d 100644 --- a/src/test/chain_tests.cpp +++ b/src/test/chain_tests.cpp @@ -71,10 +71,9 @@ BOOST_AUTO_TEST_CASE(basic_tests) BOOST_CHECK_EQUAL(chain_2[2], nullptr); // Contains: call with contained & non-contained blocks - BOOST_CHECK(chain_2.Contains(&genesis)); - BOOST_CHECK(chain_2.Contains(&bi1)); - BOOST_CHECK(!chain_0.Contains(&genesis)); - // BOOST_CHECK(!chain_0.Contains(nullptr)); // fail with memory access violation + BOOST_CHECK(chain_2.Contains(genesis)); + BOOST_CHECK(chain_2.Contains(bi1)); + BOOST_CHECK(!chain_0.Contains(genesis)); // Call with non-tip & tip blocks BOOST_CHECK_EQUAL(chain_2.Next(&genesis), &bi1); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 5aefd65679f..a22cd2b199d 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -45,7 +45,7 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp) if (lp.maxInputBlock) { // Check whether active_chain is an extension of the block at which the LockPoints // calculation was valid. If not LockPoints are no longer valid - if (!active_chain.Contains(lp.maxInputBlock)) { + if (!active_chain.Contains(*lp.maxInputBlock)) { return false; } } diff --git a/src/validation.cpp b/src/validation.cpp index a2b93233b91..6fc38c50ce9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -126,7 +126,7 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato for (const uint256& hash : locator.vHave) { const CBlockIndex* pindex{m_blockman.LookupBlockIndex(hash)}; if (pindex) { - if (m_chain.Contains(pindex)) { + if (m_chain.Contains(*pindex)) { return pindex; } if (pindex->GetAncestor(m_chain.Height()) == m_chain.Tip()) { @@ -3124,7 +3124,7 @@ CBlockIndex* Chainstate::FindMostWorkChain() // Check whether all blocks on the path between the currently active chain and the candidate are valid. // Just going until the active chain is an optimization, as we know all blocks in it are valid already. bool fInvalidAncestor = false; - for (CBlockIndex *pindexTest = pindexNew; pindexTest && !m_chain.Contains(pindexTest); pindexTest = pindexTest->pprev) { + for (CBlockIndex *pindexTest = pindexNew; pindexTest && !m_chain.Contains(*pindexTest); pindexTest = pindexTest->pprev) { assert(pindexTest->HaveNumChainTxs() || pindexTest->nHeight == 0); // Pruned nodes may have entries in setBlockIndexCandidates for @@ -3507,7 +3507,7 @@ bool Chainstate::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) return ActivateBestChain(state, std::shared_ptr()); } -bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pindex) +bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* const pindex) { AssertLockNotHeld(m_chainstate_mutex); AssertLockNotHeld(::cs_main); @@ -3533,16 +3533,16 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde { LOCK(cs_main); for (auto& entry : m_blockman.m_block_index) { - CBlockIndex* candidate = &entry.second; + CBlockIndex& candidate = entry.second; // We don't need to put anything in our active chain into the // multimap, because those candidates will be found and considered // as we disconnect. // Instead, consider only non-active-chain blocks that score // at least as good with CBlockIndexWorkComparator as the new tip. if (!m_chain.Contains(candidate) && - !CBlockIndexWorkComparator()(candidate, pindex->pprev) && - !(candidate->nStatus & BLOCK_FAILED_VALID)) { - highpow_outofchain_headers.insert({candidate->nChainWork, candidate}); + !CBlockIndexWorkComparator()(&candidate, pindex->pprev) && + !(candidate.nStatus & BLOCK_FAILED_VALID)) { + highpow_outofchain_headers.insert({candidate.nChainWork, &candidate}); } } } @@ -3562,9 +3562,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // Lock for as long as disconnectpool is in scope to make sure MaybeUpdateMempoolForReorg is // called after DisconnectTip without unlocking in between LOCK(MempoolMutex()); - if (!m_chain.Contains(pindex)) break; + if (!m_chain.Contains(*pindex)) break; pindex_was_in_chain = true; - CBlockIndex* disconnected_tip{m_chain.Tip()}; + CBlockIndex* const disconnected_tip{m_chain.Tip()}; // ActivateBestChain considers blocks already in m_chain // unconditionally valid already, so force disconnect away from it. @@ -3634,7 +3634,7 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde { LOCK(cs_main); - if (m_chain.Contains(to_mark_failed)) { + if (m_chain.Contains(*to_mark_failed)) { // If the to-be-marked invalid block is in the active chain, something is interfering and we can't proceed. return false; } @@ -5148,7 +5148,7 @@ void ChainstateManager::CheckBlockIndex() const std::multimap forward; for (auto& [_, block_index] : m_blockman.m_block_index) { // Only save indexes in forward that are not part of the best header chain. - if (!best_hdr_chain.Contains(&block_index)) { + if (!best_hdr_chain.Contains(block_index)) { // Only genesis, which must be part of the best header chain, can have a nullptr parent. assert(block_index.pprev); forward.emplace(block_index.pprev, &block_index); @@ -5383,7 +5383,7 @@ void ChainstateManager::CheckBlockIndex() const pindex = range.first->second; nHeight++; continue; - } else if (best_hdr_chain.Contains(pindex)) { + } else if (best_hdr_chain.Contains(*pindex)) { // Descend further into best header chain. nHeight++; pindex = best_hdr_chain[nHeight];