Add nullptr-check to CChain::Contains(), tests by optout21 · Pull Request #34416 · bitcoin/bitcoin

optout21 · 2026-01-27T05:25:08Z

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 · 2026-01-27T05:25:17Z

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  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34654 ([RFC] BlockMap and CChain Concurrency Improvement by alexanderwiederin)

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.

maflcko · 2026-01-27T08:28:27Z

src/net_processing.cpp

 bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
 {
    AssertLockHeld(cs_main);
+    if (!pindex) return false;


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.

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).

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

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

Being done in 6539829 ( #34440 )

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

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

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.)

Note: BlockRequestAllowed change has been done in #34464.

optout21 · 2026-01-29T09:25:41Z

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 · 2026-01-29T11:16:43Z

Marked ready for review.

maflcko · 2026-01-29T19:52:01Z

src/net_processing.cpp

 bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
 {
    AssertLockHeld(cs_main);
+    if (!pindex) return false;


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

src/test/chain_tests.cpp

HowHsu · 2026-02-05T09:42:32Z

test ACK 0c2df01

1f8f7d4 Change BlockRequestAllowed() to take ref (optout) Pull request description: As [suggested here](#34416 (comment)), a minor refactor of `PeerManagerImpl::BlockRequestAllowed()` to take reference parameter (instead of pointer). The motivation is to make the code safer, by minimizing the risk of null-dereference, and to be more consistent. The change is local to the `PeerManagerImpl::BlockRequestAllowed()` class. Related to #34440. ACKs for top commit: maflcko: review ACK 1f8f7d4 🎐 l0rinc: tested ACK 1f8f7d4 stickies-v: ACK 1f8f7d4 Tree-SHA512: 9c2de2d3e7d067e018db7ec54c79f512ccc1da54574d4fb362f6697ee6e235783779d7094cf20856cd34e08a1dbc74609d8351fe7b2287cd8ec0c836ef07be19

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

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.

optout21 · 2026-02-05T23:22:05Z

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.
0c2df01..6c3a91f
Ready for (re)review.

HowHsu · 2026-02-07T10:03:46Z

ACK 6c3a91f

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

optout21 mentioned this pull request Jan 27, 2026

Add nullptr-check to CChain::Contains() optout21/bitcoin#3

Closed

optout21 marked this pull request as ready for review January 27, 2026 07:31

maflcko reviewed Jan 27, 2026

View reviewed changes

optout21 marked this pull request as draft January 27, 2026 13:20

optout21 mentioned this pull request Jan 29, 2026

Refactor CChain methods to use references, tests #34440

Draft

optout21 force-pushed the cchain-contains branch from 7f27d10 to 0c2df01 Compare January 29, 2026 09:29

optout21 marked this pull request as ready for review January 29, 2026 11:16

maflcko suggested changes Jan 29, 2026

View reviewed changes

optout21 mentioned this pull request Jan 30, 2026

Change BlockRequestAllowed() to take ref (minor refactor) #34464

Merged

DrahtBot mentioned this pull request Jan 30, 2026

test: Add test on skip heights in CBlockIndex #33661

Open

HowHsu reviewed Feb 5, 2026

View reviewed changes

src/test/chain_tests.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label Feb 5, 2026

optout21 added 2 commits February 5, 2026 23:57

Add CChain basic tests

ced935a

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

optout21 force-pushed the cchain-contains branch from 0c2df01 to 6c3a91f Compare February 5, 2026 23:15

DrahtBot added CI failed and removed Needs rebase labels Feb 6, 2026

optout21 mentioned this pull request Feb 7, 2026

index: handle case where pindex_prev equals chain tip in NextSyncBlock() #32875

Open

DrahtBot removed the CI failed label Feb 9, 2026

DrahtBot mentioned this pull request Feb 23, 2026

[RFC] BlockMap and CChain Concurrency Improvement #34654

Draft

Conversation

optout21 commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

optout21 commented Jan 29, 2026

Uh oh!

optout21 commented Jan 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HowHsu commented Feb 5, 2026

Uh oh!

optout21 commented Feb 5, 2026

Uh oh!

HowHsu commented Feb 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

optout21 commented Jan 27, 2026 •

edited

Loading

DrahtBot commented Jan 27, 2026 •

edited

Loading

optout21 Feb 5, 2026 •

edited

Loading

HowHsu commented Feb 7, 2026 •

edited

Loading