← Watch feed
Change CChain::Next() to take reference
Public commit record
80/100 · Strong
What the developer wrote
Authored by optout
Change CChain::Next() to take reference
To minimize chance of erroneous nullptr dereference, `CChain::Next()`
is changed to take a reference instead of a pointer.
Call sites have been adapted. Notably, NextSyncBlock() now checks
the FindFork() result before calling into Next(), because
the fork lookup may return null.
✓ 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 tightens up a low-level helper function in Bitcoin Core that finds the next block in the chain. It changes the function so callers must pass a real block object rather than a potentially-null pointer, and it adds a safety check in one place where the previous code could have passed a null pointer. The change is defensive hardening rather than a fix for a confirmed, exploitable bug.
Recommended action
Treat as routine defensive maintenance. Reviewers should verify that all call sites now guarantee a non-null CBlockIndex before calling Next(), and that the Assert(fork) in NextSyncBlock() is acceptable because the genesis block is always a common ancestor. No urgent deployment action is indicated.
Security signals we found
01
API hardening: pointer parameter replaced with reference to enforce non-null input
02
Null-check added: NextSyncBlock() asserts FindFork() result before use
03
Defensive refactor across consensus-adjacent code paths (net processing, validation, REST, indexes)
04
Removed commented-out test line that documented a memory access violation on null input
Evidence from the diff
CChain::Next() is refactored to accept a const CBlockIndex& instead of a const CBlockIndex*, preventing accidental null-pointer dereferences at call sites. Most call sites already hold a valid pindex, but NextSyncBlock() in src/index/base.cpp now explicitly asserts that chain.FindFork() did not return nullptr before dereferencing it via Next(). The commit message frames this as minimizing the chance of erroneous nullptr dereference. No CVE, advisory, or vendor security disclosure is present in the supplied materials.
Changed components
src/chain.hsrc/index/base.cppsrc/net_processing.cppsrc/rest.cppsrc/validation.cppsrc/test/blockfilter_index_tests.cppsrc/test/chain_tests.cppInspect captured patch +21 / −21
diff --git a/src/chain.h b/src/chain.h
index 7c3a369c..a2295b08 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -413,11 +413,10 @@ public:
}
/** 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
+ CBlockIndex* Next(const CBlockIndex& index) const
{
- // Note: the next commit makes sure that the input parameter of Next() cannot be nullptr
- if (Contains(*pindex))
- return (*this)[pindex->nHeight + 1];
+ if (Contains(index))
+ return (*this)[index.nHeight + 1];
else
return nullptr;
}
diff --git a/src/index/base.cpp b/src/index/base.cpp
index 9e75109d..416137dd 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -20,6 +20,7 @@
#include <tinyformat.h>
#include <uint256.h>
#include <undo.h>
+#include <util/check.h>
#include <util/fs.h>
#include <util/log.h>
#include <util/string.h>
@@ -30,7 +31,6 @@
#include <validation.h>
#include <validationinterface.h>
-#include <cassert>
#include <compare>
#include <cstdint>
#include <functional>
@@ -147,7 +147,7 @@ bool BaseIndex::Init()
return true;
}
-static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain& chain) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+static const CBlockIndex* NextSyncBlock(const CBlockIndex* const pindex_prev, CChain& chain) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
AssertLockHeld(cs_main);
@@ -155,7 +155,7 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
return chain.Genesis();
}
- if (const auto* pindex{chain.Next(pindex_prev)}) {
+ if (const auto* pindex{chain.Next(*pindex_prev)}) {
return pindex;
}
@@ -166,7 +166,9 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
// Since block is not in the chain, return the next block in the chain AFTER the last common ancestor.
// Caller will be responsible for rewinding back to the common ancestor.
- return chain.Next(chain.FindFork(pindex_prev));
+ const auto* fork{chain.FindFork(pindex_prev)};
+ // Common ancestor must exist (genesis).
+ return chain.Next(*Assert(fork));
}
bool BaseIndex::ProcessBlock(const CBlockIndex* pindex, const CBlock* block_data)
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 4eed2e12..3c302b87 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4215,10 +4215,10 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
// Send the rest of the chain
if (pindex)
- pindex = m_chainman.ActiveChain().Next(pindex);
+ pindex = m_chainman.ActiveChain().Next(*pindex);
int nLimit = 500;
LogDebug(BCLog::NET, "getblocks %d to %s limit %d from peer=%d\n", (pindex ? pindex->nHeight : -1), hashStop.IsNull() ? "end" : hashStop.ToString(), nLimit, pfrom.GetId());
- for (; pindex; pindex = m_chainman.ActiveChain().Next(pindex))
+ for (; pindex; pindex = m_chainman.ActiveChain().Next(*pindex))
{
if (pindex->GetBlockHash() == hashStop)
{
@@ -4354,14 +4354,14 @@ void PeerManagerImpl::ProcessMessage(Peer& peer, CNode& pfrom, const std::string
// Find the last block the caller has in the main chain
pindex = m_chainman.ActiveChainstate().FindForkInGlobalIndex(locator);
if (pindex)
- pindex = m_chainman.ActiveChain().Next(pindex);
+ pindex = m_chainman.ActiveChain().Next(*pindex);
}
// we must use CBlocks, as CBlockHeaders won't include the 0x00 nTx count at the end
std::vector<CBlock> vHeaders;
int nLimit = m_opts.max_headers_result;
LogDebug(BCLog::NET, "getheaders %d to %s from peer=%d\n", (pindex ? pindex->nHeight : -1), hashStop.IsNull() ? "end" : hashStop.ToString(), pfrom.GetId());
- for (; pindex; pindex = m_chainman.ActiveChain().Next(pindex))
+ for (; pindex; pindex = m_chainman.ActiveChain().Next(*pindex))
{
vHeaders.emplace_back(pindex->GetBlockHeader());
if (--nLimit <= 0 || pindex->GetBlockHash() == hashStop)
diff --git a/src/rest.cpp b/src/rest.cpp
index 25fe5d8e..d2c5a9b4 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -230,7 +230,7 @@ static bool rest_headers(const std::any& context,
if (headers.size() == *parsed_count) {
break;
}
- pindex = active_chain.Next(pindex);
+ pindex = active_chain.Next(*pindex);
}
}
@@ -556,7 +556,7 @@ static bool rest_filter_header(const std::any& context, HTTPRequest* req, const
headers.push_back(pindex);
if (headers.size() == *parsed_count)
break;
- pindex = active_chain.Next(pindex);
+ pindex = active_chain.Next(*pindex);
}
}
diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp
index 25762e07..02417314 100644
--- a/src/test/blockfilter_index_tests.cpp
+++ b/src/test/blockfilter_index_tests.cpp
@@ -132,7 +132,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup)
for (const CBlockIndex* block_index = m_node.chainman->ActiveChain().Genesis();
block_index != nullptr;
- block_index = m_node.chainman->ActiveChain().Next(block_index)) {
+ block_index = m_node.chainman->ActiveChain().Next(*block_index)) {
BOOST_CHECK(!filter_index.LookupFilter(block_index, filter));
BOOST_CHECK(!filter_index.LookupFilterHeader(block_index, filter_header));
BOOST_CHECK(!filter_index.LookupFilterRange(block_index->nHeight, block_index, filters));
@@ -152,7 +152,7 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, BuildChainTestingSetup)
const CBlockIndex* block_index;
for (block_index = m_node.chainman->ActiveChain().Genesis();
block_index != nullptr;
- block_index = m_node.chainman->ActiveChain().Next(block_index)) {
+ block_index = m_node.chainman->ActiveChain().Next(*block_index)) {
CheckFilterLookups(filter_index, block_index, last_header, m_node.chainman->m_blockman);
}
}
diff --git a/src/test/chain_tests.cpp b/src/test/chain_tests.cpp
index c85439a4..cf620958 100644
--- a/src/test/chain_tests.cpp
+++ b/src/test/chain_tests.cpp
@@ -76,10 +76,9 @@ BOOST_AUTO_TEST_CASE(basic_tests)
BOOST_CHECK(!chain_0.Contains(genesis));
// Call with non-tip & tip blocks
- BOOST_CHECK_EQUAL(chain_2.Next(&genesis), &bi1);
- BOOST_CHECK_EQUAL(chain_2.Next(&bi1), nullptr);
- BOOST_CHECK_EQUAL(chain_0.Next(&genesis), nullptr);
- // BOOST_CHECK_EQUAL(chain_0.Next(nullptr), nullptr); // fail with memory access violation
+ BOOST_CHECK_EQUAL(chain_2.Next(genesis), &bi1);
+ BOOST_CHECK_EQUAL(chain_2.Next(bi1), nullptr);
+ BOOST_CHECK_EQUAL(chain_0.Next(genesis), nullptr);
BOOST_CHECK_EQUAL(chain_2.Genesis(), &genesis);
BOOST_CHECK_EQUAL(chain_0.Genesis(), nullptr);
diff --git a/src/validation.cpp b/src/validation.cpp
index 6fc38c50..b8903e81 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4711,7 +4711,7 @@ VerifyDBResult CVerifyDB::VerifyDB(
reportDone = percentageDone / 10;
}
m_notifications.progress(_("Verifying blocks…"), percentageDone, false);
- pindex = chainstate.m_chain.Next(pindex);
+ pindex = chainstate.m_chain.Next(*pindex);
CBlock block;
if (!chainstate.m_blockman.ReadBlock(block, *pindex)) {
LogError("Verification error: ReadBlock failed at %d, hash=%s", pindex->nHeight, pindex->GetBlockHash().ToString());
Risk score
Our methodology →Why this scored 27/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.