← Watch feed
Change CChain::Contains() to take reference
Public commit record
80/100 · Strong
What the developer wrote
Authored by optout
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.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Explains rationale or failure mode
What changed, and why it matters
This commit hardens a core Bitcoin method called CChain::Contains() so it can no longer be accidentally called with a null (missing) block pointer, which would crash the node. The change turns the method's input from a pointer into a reference, forcing every caller to prove the block exists before calling it. Several call sites were updated, and one place in startup index syncing now explicitly handles a missing block instead of silently passing a null pointer. It is a defensive fix that removes a class of potential null-dereference crashes, but the commit message does not frame it as a critical security vulnerability and no exploit is demonstrated.
Recommended action
Treat as a low-to-moderate hardening commit. Review whether any remaining callers of CChain::Next() still accept nullptr (the commit message notes the next commit will address that). Ensure fuzzing and unit tests cover null/missing block index paths for RPC, REST, and P2P handlers. No urgent patch deployment is indicated absent a demonstrated exploit, but the change is safe to include in normal release cadence.
Security signals we found
01
Null-pointer dereference hardening in consensus-adjacent code
02
Signature change from pointer to reference to enforce non-null at compile time
03
Addition of explicit null check in StartIndexBackgroundSync before Contains() call
04
Use of Assert() macro in net_processing to document assumed non-null invariant
05
Removal of commented-out crash test indicating prior awareness of null-deref behavior
Evidence from the diff
CChain::Contains() previously accepted const CBlockIndex and dereferenced it unconditionally (return (this)[pindex->nHeight] == pindex). The commit changes the signature to const CBlockIndex&, eliminating the possibility of a nullptr dereference inside Contains() itself. Callers are updated to pass *pindex or a reference; some already-guarded loops remain logically equivalent, while init.cpp now adds an explicit !pindex check before calling Contains(). net_processing.cpp uses Assert(pindexWalk) in one path. The change is type-system-level hardening rather than a runtime behavior change for most paths, because most existing callers already held non-null pointers. It reduces attack surface for crafted RPC/REST/P2P inputs that could previously reach Contains() with a null lookup result.
Changed components
src/chain.h / src/chain.cpp - CChain::Contains() and CChain::Next()src/init.cpp - index background syncsrc/net_processing.cpp - block fetching and header validationsrc/rest.cpp - REST block header endpointssrc/rpc/blockchain.cpp - getchaintips, getchaintxstats, getdescriptoractivitysrc/rpc/rawtransaction.cpp - getrawtransactionsrc/rpc/txoutproof.cpp - verifytxoutproofsrc/txmempool.cpp - LockPoints validitysrc/validation.cpp - block locator, invalidate block, check block indexsrc/kernel/bitcoinkernel.cpp - kernel chain contains wrapperInspect captured patch +41 / −37
diff --git a/src/chain.cpp b/src/chain.cpp
index 46a51ac8..958872e5 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 c2782920..7c3a369c 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 407ee0fe..6642c87f 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 dc03be71..34dc9ce2 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 c7061d52..4eed2e12 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1503,7 +1503,7 @@ void PeerManagerImpl::FindNextBlocks(std::vector<const CBlockIndex*>& 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<const CBlockIndex*> 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 53dd3d16..25fe5d8e 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 0821ebba..956f7066 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1631,7 +1631,7 @@ static RPCMethod getchaintips()
std::set<const CBlockIndex*> 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 74652c17..a0f93f0c 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 f68f53f6..628825fc 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 daae8f3f..c85439a4 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 5aefd656..a22cd2b1 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 a2b93233..6fc38c50 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<const CBlock>());
}
-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<const CBlockIndex*, const CBlockIndex*> 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];
Risk score
Our methodology →Why this scored 47/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.