From 20b58e281adec16d4726bf49f99350ac9a6177e7 Mon Sep 17 00:00:00 2001 From: optout <13562139+optout21@users.noreply.github.com> Date: Thu, 29 Jan 2026 12:01:23 +0100 Subject: [PATCH] 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. --- src/chain.h | 7 +++---- src/index/base.cpp | 10 ++++++---- src/net_processing.cpp | 8 ++++---- src/rest.cpp | 4 ++-- src/test/blockfilter_index_tests.cpp | 4 ++-- src/test/chain_tests.cpp | 7 +++---- src/validation.cpp | 2 +- 7 files changed, 21 insertions(+), 21 deletions(-) diff --git a/src/chain.h b/src/chain.h index 7c3a369cb5c..a2295b0883c 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 9e75109d5c9..416137dd57a 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -30,7 +31,6 @@ #include #include -#include #include #include #include @@ -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 4eed2e124c8..3c302b8775f 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 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 25fe5d8e80c..d2c5a9b4899 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 25762e070db..02417314757 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 c85439a4d5d..cf620958cba 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 6fc38c50ce9..b8903e816ee 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());