Make Assert(…) usable in all contexts. Make implicit assumptions explicit. #20122
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Definite ack on the first commit; not super-convinced by the actual uses.
Are these changes inspired by static analysis? They look like they're making errors go away more than making it clear why the code is safe.
I think doing an inline Assert(foo) only makes sense when foo is complicated/slow or has side-effects; when foo is just a pointer/int, better to have the assertion on its own line/statement so that it's easier to spot.
| @@ -400,7 +401,7 @@ class CChain { | |||
|
|
|||
| /** Efficiently check whether a block is present in this chain. */ | |||
| bool Contains(const CBlockIndex *pindex) const { | |||
| return (*this)[pindex->nHeight] == pindex; | |||
| return (*this)[Assert(pindex)->nHeight] == pindex; | |||
There was a problem hiding this comment.
Introducing the non-null attribute for the pindex argument might be a useful addition here?
I tried converting this to CBlockIndex& index and it has some flow-on effects (to CChain::Next, CChain::FindFork, and others) that look like they might be worth investigating and cleaning up.
| @@ -4273,7 +4273,7 @@ bool PeerManager::SendMessages(CNode* pto) | |||
| bool fGotBlockFromCache = false; | |||
| { | |||
| LOCK(cs_most_recent_block); | |||
| if (most_recent_block_hash == pBestIndex->GetBlockHash()) { | |||
| if (most_recent_block_hash == Assert(pBestIndex)->GetBlockHash()) { | |||
There was a problem hiding this comment.
Might be clearer to have:
if (!fRevertToInv && !vHeaders.empty()) {
+ assert(pBestIndex != nullptr); // was assigned to non-null pindex in order for vHeaders to be populated
if (vHeaders.size() == 1 && state.fPreferHeaderAndIDs) {instead? (prior related PR #13548)
| @@ -4321,7 +4321,7 @@ bool CVerifyDB::VerifyDB(const CChainParams& chainparams, CCoinsView *coinsview, | |||
| return error("VerifyDB(): *** coin database inconsistencies found (last %i blocks, %i good transactions before that)\n", ::ChainActive().Height() - pindexFailure->nHeight + 1, nGoodTransactions); | |||
|
|
|||
| // store block count as we move pindex at check level >= 4 | |||
| int block_count = ::ChainActive().Height() - pindex->nHeight; | |||
| int block_count = ::ChainActive().Height() - Assert(pindex)->nHeight; | |||
There was a problem hiding this comment.
I think this assertion is effectively testing if ::ChainActive().Tip() == nullptr as the other ways of setting pindex all seem to be pindex = pindex->pprev after testing pindex->pprev. Might be better to put the Assert there?
There are other places where we assume ChainActive().Tip() is non-null (ie, we immediately dereference it), but at a quick glance I couldn't assure myself that was actually always true. Might be better to actually make it always true (and add a non-null attribute on the response?).
|
@ajtowns I'm leaving this one as up for grabs now that the first commit is included in #20138 :)
They won't make any error go away, but they guarantee we get an assertion failure in case of an error (instead of UB) :) |
|
@practicalswift Sorry, I meant "making complaints from automated tools go away, without fixing any underlying bugs or making it easier for people to see why the code is safe" :) I think adding asserts should be making it clearer for people to see why the code is safe as well as automated tools. (If you are getting this from an automated tool, are there reports from it around? I remember there were previously somewhere) |
I agree about that statement :) In addition to that use I also think assertions can be useful to get deterministic termination in case where we otherwise would get a null pointer dereference (if our assumption of non-null turns out to be false). Why is that desired? One might think that a null pointer dereference is guaranteed to lead to immediate termination. Unfortunately that is not the case: deterministic termination is the best outcome. Another possible outcome is what in the literature is called "optimization-unsafety" which can be problematic from a security perspective. There is a neat paper covering optimization-safe code called "Towards Optimization-Safe Systems: Analyzing the Impact of Undefined Behavior" which I recommend for context. Personally I think adding assertions can be handy as a cheap way to avoid some instances of potentially "optimization-unsafe code". That is my personal opinion: I know others might not agree and that is fine of course :)
IIRC Coverity and Clang's Cross Translation Unit (CTU) static analysis flag these cases. I think most static analysers doing pointer analysis would flag at least some of these since it is non-trivial for them to infer non- I don't have any reports readily available, but feel free to check the instructions in #18920 for instructions on how to get Bitcoin Core reports from Clang Static Analysis, clang-tidy and cppcheck. It only takes a couple of minutes of setup :) Hope that helps! :) |
Assert(…)(util/check.h) usable in all contexts.Assert(…)to make implicit assumptions explicit.Before the first commit: