Add nullptr-check to CChain::Contains(), tests #34416
Conversation
Add basic unit tests to the `CChain` class, filling a gap.
Add (lengthier) unit tests for `CChain::FindFork()`.
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.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34416. ReviewsSee the guideline for information on the review process. |
| bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex) | ||
| { | ||
| AssertLockHeld(cs_main); | ||
| if (!pindex) return false; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
Add unit tests to the
CChainclass, and add a nullptr-check toCChain::Contains()to avoid potential memory access violation bynullptrdereference.The
CChain::Contains()method (insrc/chain.h) dereferences its input without checking. TheNext()method also calls into this with anullptrif invoked withnullptr. While most call sites have indirect guarantee that the input is notnullptr, 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
CChainclass, filling a gap.Changes:
CChainclass methodsCChain::Contains()CChain::FindFork(). These are lengthier, and not closely related to the topic of the PR, could be removed, but kept for extra test coverage.Alternative. I have considered changing
Contains()to take a reference instead of pointer, to better express that it cannot take anullptrinput. It's a viable solution, requires update to only a handful of call sites (solution available on branchcchain-contains1). However, since mostCChainlogic operates with pointers and pointer comparison, it would break the style, so I dropped this solution.This change is remotely related to and partly triggered by #32875 .