Sanity assert GetAncestor() != nullptr where appropriate #11342
Conversation
|
There are other places where the result of |
| @@ -316,6 +316,7 @@ bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool | |||
| } | |||
| } | |||
| lp->maxInputBlock = tip->GetAncestor(maxInputHeight); | |||
| assert(lp->maxInputBlock != nullptr); | |||
There was a problem hiding this comment.
Just assert(lp->maxInputBlock);?
There was a problem hiding this comment.
@promag While legal, I believe for security-critical code implicit conversions such as these should be discouraged. See also http://releases.llvm.org/3.8.0/tools/clang/tools/extra/docs/clang-tidy/checks/readability-implicit-bool-cast.html
There was a problem hiding this comment.
I see why implicit conversions should be discouraged for floating point numbers and integers or bools, but I don't see any danger or downsides of assert(some_pointer);
Add sanity asserts for return value of `CBlockIndex::GetAncestor()` where appropriate. In validation.cpp `CheckSequenceLocks`, check the return value of `tip->GetAncestor(maxInputHeight)` stored into `lp->maxInputBlock`. If it ever returns `nullptr` because the ancestor isn't found, it's going to be a bad bug to keep going, since a `LockPoints` object with the `maxInputBlock` member set to `nullptr` signifies no relative lock time. In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is UB.
|
@promag Done, |
|
Any specific reason for using |
| The last travis run for this pull request was 302 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
| Needs rebase |
|
Closing because of inactivity, see that @MarcoFalke already added up for grabs |
Add sanity asserts for return value of
CBlockIndex::GetAncestor()where appropriate.In validation.cpp
CheckSequenceLocks, check the return value oftip->GetAncestor(maxInputHeight)stored intolp->maxInputBlock. If it ever returnsnullptrbecause the ancestor isn't found, it's going to be a bad bug to keep going, since aLockPointsobject with themaxInputBlockmember set tonullptrsignifies no relative lock time.In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is UB.