Add nullptr-check to CChain::Contains(), tests

optout21 commented at 5:25 am on January 27, 2026: contributor

Add unit tests to the CChain class, and add a nullptr-check to CChain::Contains() to avoid potential memory access violation by nullptr dereference.

The CChain::Contains() method (in src/chain.h) dereferences its input without checking. The Next() method also calls into this with a nullptr if invoked with nullptr. While most call sites have indirect guarantee that the input is not nullptr, it’s not easy to establish this to all call sites with high confidence. These methods are publicly available. There is no known high-level use case to trigger this error, but the fix is easy, and makes the code safer.

Also, unit tests were added to the CChain class, filling a gap.

Changes:

Add basic unit tests for CChain class methods
Add a nullptr-check to CChain::Contains()

Alternative. I have considered changing Contains() to take a reference instead of pointer, however, when applying that to various methods, it grew to a much bigger changeset – see #34440. Having in mind the friction of larger PRs, I think the present minimal one-check solution (with the tests) makes sense on its own.

This change is remotely related to and indirectly triggered by #32875 .

DrahtBot commented at 5:25 am on January 27, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34416.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	HowHsu

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34440 (Refactor CChain methods to use references, tests by optout21)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

optout21 marked this as ready for review on Jan 27, 2026

in src/net_processing.cpp:1926 in 7f27d102ae outdated

1922@@ -1923,6 +1923,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
1923 bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1924 {
1925     AssertLockHeld(cs_main);
1926+    if (!pindex) return false;

maflcko commented at 8:28 am on January 27, 2026:

i think this goes in the wrong direction. All call sites have checked against nullptr, and there should never be a reason to call this with nullptr, so this seems it should be a reference. Just like BlockRequested and FetchBlock take a reference.

optout21 commented at 1:19 pm on January 27, 2026:

Following this feedback, I will reconsider the switch to pass-by-reference. As mentioned in the description, I started in that direction, but was concerned about consistency. For consistency, it makes sense to also change the other methods (e.g. Next()), increasing the scope. Const input pointer parameters are relatively easy to change to pass-by-reference; but return values where nullptr is a valid option should be changed to std::option<&...>. These end up in a larger changeset (arguably not adding much value).

maflcko commented at 1:23 pm on January 27, 2026:

Well, i haven’t looked into the lower level chain util methods. For some methods, it could make sense to accept a pointer. My comment was mostly about BlockRequestAllowed, and other similar high level functions that only work on blocks that exist and whose presence has been previously confirmed

maflcko commented at 7:52 pm on January 29, 2026:

bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex*) should be changed to bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex&)

optout21 commented at 8:06 am on January 30, 2026:

Being done in https://github.com/bitcoin/bitcoin/pull/34440/changes/6539829321aa13b4afb8d898a62eb3be382607c0 ( #34440 )

maflcko commented at 9:06 am on January 30, 2026:

i don’t understand why the some line of code should be changed, only to revert it in the next commit. Please submit 6539829321aa13b4afb8d898a62eb3be382607c0 as a separate pull request

maflcko commented at 9:07 am on January 30, 2026:

also, the name pindex should probably be changed to block_index (or so), like in the function immediately below

optout21 commented at 3:06 pm on January 30, 2026:

i don’t understand why the some line of code should be changed, only to revert it in the next commit. Please submit 6539829 as a separate pull request

I can do it in a separate PR, should be trivial. The add/remove is a side-effect of independent commits: one commit changed the method called from here (Contains), so a null check had to be added before the call. Then when this method has also been change to take a reference input, the check is no longer needed. (The check moved outwards.)

optout21 commented at 11:27 pm on February 5, 2026:

Note: BlockRequestAllowed change has been done in #34464.

optout21 marked this as a draft on Jan 27, 2026

optout21 commented at 9:25 am on January 29, 2026: contributor

I’m exploring the option to use references, but I’ve decided to keep this simple PR as first step.

As suggested, I look into using references instead of pointers. Howerver, that is a bigger refactor. I think the minimal change (one extra check) and unit tests from this PR make sense in isolation, as the bigger change will be heavier. Draft PR #34440 opened. I will strip this one a bit more. CC: @maflcko

optout21 force-pushed on Jan 29, 2026

optout21 marked this as ready for review on Jan 29, 2026

optout21 commented at 11:16 am on January 29, 2026: contributor

Marked ready for review.

in src/test/chain_tests.cpp:60 in 0c2df01acd outdated

56@@ -57,9 +57,9 @@ BOOST_AUTO_TEST_CASE(cchain_basic_tests)
57         BOOST_CHECK_EQUAL(chain_0.Tip(), nullptr);
58         BOOST_CHECK_EQUAL(chain_0[0], nullptr);
59         BOOST_CHECK_EQUAL(chain_0.Contains(&genesis), false);
60-        // BOOST_CHECK_EQUAL(chain_0.Contains(nullptr), false); // fail with memory access violation
61+        BOOST_CHECK_EQUAL(chain_0.Contains(nullptr), false);

HowHsu commented at 1:19 pm on January 29, 2026:

Would it be better to adjust the order of these two commits to make this one first, so that no tests needed to be changed in this commit?

optout21 commented at 9:06 am on February 5, 2026:

IMHO it’s better to have the test first. The commented out tests could be omitted from the first test commit, and only added with the behavior change, but I chose this way so that it’s visible in the commit how it affects behavior/test.

The general approach I try to follow here:

find/identify an issue
create a test for it that exposes the issue
fix the issue and adjust the test to the correct behavior

In this case the call with nullptr crashes, so there is no easy way to actually test that, hence it was added just as a comment.

HowHsu commented at 9:39 am on February 5, 2026:

IMHO it’s better to have the test first. The commented out tests could be omitted from the first test commit, and only added with the behavior change, but I chose this way so that it’s visible in the commit how it affects behavior/test.

The general approach I try to follow here:

find/identify an issue

create a test for it that exposes the issue

fix the issue and adjust the test to the correct behavior

In this case the call with nullptr crashes, so there is no easy way to actually test that, hence it was added just as a comment.

Make sense.

maflcko changes_requested

HowHsu commented at 9:42 am on February 5, 2026: none

test ACK 0c2df01

fanquake referenced this in commit 9d76947294 on Feb 5, 2026

DrahtBot added the label Needs rebase on Feb 5, 2026

Add CChain basic tests

Add basic unit tests to the `CChain` class, filling a gap.

ced935ac6c

Add nullptr-check to CChain::Contains()

The `CChain::Contains()` method dereferences its input without checking,
potentially resulting in nullptr-dereference if invoked with `nullptr`.
The `Next()` method can also trigger this. Avoid the memory access
violation by adding an extra check for the `pindex` input.
The result is `false` for `nullptr` input.

6c3a91f9ce

optout21 force-pushed on Feb 5, 2026

optout21 commented at 11:22 pm on February 5, 2026: contributor

Rebased, following merging of #34464 . One adaptation became obsolete: the added nullptr-check in PeerManagerImpl::BlockRequestAllowed() is no longer needed, as the input parameter has been changed to a reference (in #34464). The file src/net_processing.cpp is no longer in the changeset. 0c2df01acda5891e5c2cea89753ffb5715b2f287..6c3a91f9cefe28a9c7a59963a51c2d64d8c005a2 Ready for (re)review.

DrahtBot added the label CI failed on Feb 6, 2026

DrahtBot removed the label Needs rebase on Feb 6, 2026

HowHsu commented at 10:03 am on February 7, 2026: none

ACK 6c3a91f9cefe28a9c7a59963a51c2d64d8c005a2

By the way, the CI test failed not because of the PR code.

DrahtBot removed the label CI failed on Feb 9, 2026

Add nullptr-check to CChain::Contains(), tests #34416

Code Coverage & Benchmarks

Reviews

Conflicts