Sanity assert GetAncestor() != nullptr where appropriate #11342

1. 
   danra commented at 3:21 pm on September 15, 2017: contributor

   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.

2. fanquake added the label Validation on Sep 16, 2017
3. 
   promag commented at 0:34 am on September 20, 2017: member

   There are other places where the result of GetAncestor is not asserted or tested. IMO this PR could also handle those.
4. in src/validation.cpp:319 in 42dde93ad2 outdated

   315@@ -316,6 +316,7 @@ bool CheckSequenceLocks(const CTransaction &tx, int flags, LockPoints* lp, bool
   316                 }
   317             }
   318             lp->maxInputBlock = tip->GetAncestor(maxInputHeight);
   319+            assert(lp->maxInputBlock != nullptr);
   

   ---

   ---

   

   promag commented at 0:35 am on September 20, 2017:

   Just assert(lp->maxInputBlock);?

   ---

   

   danra commented at 4:25 pm on September 21, 2017:

   @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

   ---

   

   MarcoFalke commented at 7:41 pm on July 20, 2018:

   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);
5. danra force-pushed on Sep 21, 2017
6. danra renamed this:
   Add sanity assert in CheckSequenceLocks
   Sanity assert GetAncestor() != nullptr where appropriate
   on Sep 21, 2017
7. Sanity assert GetAncestor() != nullptr where appropriate

   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.

   33ce435013
8. danra force-pushed on Sep 21, 2017
9. 
   danra commented at 5:00 pm on September 21, 2017: contributor

   @promag Done, GetAncestor()’s return value is now verified in all places where I found it is needed.
10. 
    laanwj commented at 6:40 am on September 28, 2017: member

    Any specific reason for using assert(x != nullptr) instead of just assert(x)? (this works as-is for smart pointers)
11. 
    danra commented at 5:40 pm on September 28, 2017: contributor

    @laanwj Yes, see my reply to the similar question by @promag above.
12. 
    DrahtBot commented at 7:21 pm on July 20, 2018: member
13. DrahtBot closed this on Jul 20, 2018

    ---

14. DrahtBot reopened this on Jul 20, 2018

    ---

15. 
    DrahtBot commented at 1:28 pm on August 13, 2018: member
16. DrahtBot added the label Needs rebase on Aug 13, 2018
17. MarcoFalke added the label Up for grabs on Aug 13, 2018
18. 
    laanwj commented at 12:29 pm on August 31, 2018: member

    Closing because of inactivity, see that @MarcoFalke already added up for grabs
19. laanwj closed this on Aug 31, 2018

    ---

20. fanquake removed the label Up for grabs on Oct 23, 2019
21. laanwj removed the label Needs rebase on Oct 24, 2019
22. DrahtBot locked this on Dec 16, 2021

Contributors
danra promag MarcoFalke laanwj DrahtBot

Labels
Validation