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

pull adamjonas wants to merge 1 commits into bitcoin:master from adamjonas:assert_get_ancestor changing 5 files +22 −6
  1. adamjonas commented at 7:01 pm on October 23, 2019: member

    Re-opening #11342 (taking suggestions from comments) which adds 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 other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior.

  2. fanquake added the label Validation on Oct 23, 2019
  3. in src/test/miner_tests.cpp:459 in ddad4b086f outdated 
    452@@ -453,11 +453,17 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
453     BOOST_CHECK(CheckFinalTx(CTransaction(tx), flags)); // Locktime passes
454     BOOST_CHECK(!TestSequenceLocks(CTransaction(tx), flags)); // Sequence locks fail
455 
456-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++)
457-        ::ChainActive().Tip()->GetAncestor(::ChainActive().Tip()->nHeight - i)->nTime += 512; //Trick the MedianTimePast
458+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++) {
459+        CBlockIndex* ancestor = ::ChainActive().Tip()->GetAncestor(::ChainActive().Tip()->nHeight - i);
460+        assert(ancestor != nullptr);
461+        ancestor->nTime += 512; //Trick the MedianTimePast
    jonatack commented at 7:15 pm on October 23, 2019:
    While touching this code, it may be nice to hoist the various 512 magic values in this test (described in one place, the comment in line 461) to a well-named constant and move the comment to its definition. Perhaps also fix the uneven code spacing at the beginning of the comments.
    adamjonas commented at 7:23 pm on October 29, 2019:
    Cleaned up as suggested. Thanks!
  4. jonatack commented at 7:17 pm on October 23, 2019: member
    Concept ACK.
  5. in src/validation.cpp:335 in ddad4b086f outdated 
    302@@ -303,6 +303,7 @@ bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flag
303                 }
304             }
305             lp->maxInputBlock = tip->GetAncestor(maxInputHeight);
306+            assert(lp->maxInputBlock);
    practicalswift commented at 8:28 pm on October 23, 2019:
    If this assertion did not hold we wouldn’t necessarily dereference any null pointer, would we? Could this assertion be moved to a place where it is obvious that an assertion failure is better than continued execution? Unlike the other cases it is not entirely obvious here.
    adamjonas commented at 7:27 pm on October 29, 2019:
    tip->GetAncestor(maxInputHeight) shouldn’t return a nullptr because maxInputHeight is always less than the tip height, but did leave a comment explaining why we’d stop execution if that ever happened.
  6. practicalswift commented at 8:31 pm on October 23, 2019: contributor

    Concept ACK

    Explicit is better than implicit.

    In case the assumptions do not hold then a predictable assertion failure is the better alternative.

  7. adamjonas force-pushed on Oct 29, 2019
  8. in src/rpc/blockchain.cpp:1614 in 22cca817e4 outdated 
    1610@@ -1611,6 +1611,7 @@ static UniValue getchaintxstats(const JSONRPCRequest& request)
1611     }
1612 
1613     const CBlockIndex* pindexPast = pindex->GetAncestor(pindex->nHeight - blockcount);
1614+    assert(pindexPast != nullptr);
    MarcoFalke commented at 7:26 pm on October 29, 2019:
    We don’t use assert in rpc code anymore. please use the CHECK_NONFATAL to return a bug report to the user instead of crashing the whole server
    adamjonas commented at 7:41 pm on October 29, 2019:
    Updated
  9. adamjonas force-pushed on Oct 29, 2019
  10. jnewbery commented at 7:10 pm on October 30, 2019: member
    Code review ACK 59f51d5647d00ca30d431ff1d6f90edaa78f7951
  11. practicalswift commented at 9:31 am on November 1, 2019: contributor

    @adamjonas Thanks for making implicit assumptions explicit by adding assertions and documentation.

    If you want to continue working on making currently implicit != nullptr assumptions explicit you might want to try compiling with -Wnull-dereference to see what the compiler regards as potential null pointer dereferences.

    FWIW gcc 7.4.0 with -Wnull-dereference against current master:

     0interfaces/chain.cpp:360:63: warning: potential null pointer dereference [-Wnull-dereference]
 1interfaces/node.cpp:191:56: warning: potential null pointer dereference [-Wnull-dereference]
 2miner.cpp:333:18: warning: potential null pointer dereference [-Wnull-dereference]
 3net_processing.cpp:1206:19: warning: potential null pointer dereference [-Wnull-dereference]
 4net_processing.cpp:1299:52: warning: potential null pointer dereference [-Wnull-dereference]
 5net_processing.cpp:1305:43: warning: potential null pointer dereference [-Wnull-dereference]
 6net_processing.cpp:1431:182: warning: potential null pointer dereference [-Wnull-dereference]
 7net_processing.cpp:1499:65: warning: potential null pointer dereference [-Wnull-dereference]
 8net_processing.cpp:1619:78: warning: potential null pointer dereference [-Wnull-dereference]
 9net_processing.cpp:1637:45: warning: potential null pointer dereference [-Wnull-dereference]
10net_processing.cpp:1711:24: warning: potential null pointer dereference [-Wnull-dereference]
11net_processing.cpp:1745:117: warning: potential null pointer dereference [-Wnull-dereference]
12net_processing.cpp:2006:49: warning: potential null pointer dereference [-Wnull-dereference]
13net_processing.cpp:2086:56: warning: potential null pointer dereference [-Wnull-dereference]
14net_processing.cpp:2180:47: warning: potential null pointer dereference [-Wnull-dereference]
15net_processing.cpp:2191:41: warning: potential null pointer dereference [-Wnull-dereference]
16net_processing.cpp:2192:62: warning: potential null pointer dereference [-Wnull-dereference]
17net_processing.cpp:2193:59: warning: potential null pointer dereference [-Wnull-dereference]
18net_processing.cpp:2195:40: warning: potential null pointer dereference [-Wnull-dereference]
19net_processing.cpp:2196:60: warning: potential null pointer dereference [-Wnull-dereference]
20net_processing.cpp:2197:41: warning: potential null pointer dereference [-Wnull-dereference]
21net_processing.cpp:2199:73: warning: potential null pointer dereference [-Wnull-dereference]
22net_processing.cpp:2201:73: warning: potential null pointer dereference [-Wnull-dereference]
23net_processing.cpp:2340:114: warning: potential null pointer dereference [-Wnull-dereference]
24net_processing.cpp:2392:47: warning: potential null pointer dereference [-Wnull-dereference]
25net_processing.cpp:3227:15: warning: potential null pointer dereference [-Wnull-dereference]
26net_processing.cpp:3381:29: warning: potential null pointer dereference [-Wnull-dereference]
27net_processing.cpp:3998:37: warning: potential null pointer dereference [-Wnull-dereference]
28net_processing.cpp:401:34: warning: potential null pointer dereference [-Wnull-dereference]
29net_processing.cpp:581:47: warning: potential null pointer dereference [-Wnull-dereference]
30net_processing.cpp:653:33: warning: potential null pointer dereference [-Wnull-dereference]
31rpc/mining.cpp:446:64: warning: potential null pointer dereference [-Wnull-dereference]
32rpc/mining.cpp:500:30: warning: potential null pointer dereference [-Wnull-dereference]
33test/miner_tests.cpp:371:35: warning: potential null pointer dereference [-Wnull-dereference]
34test/miner_tests.cpp:383:35: warning: potential null pointer dereference [-Wnull-dereference]
35test/miner_tests.cpp:414:35: warning: potential null pointer dereference [-Wnull-dereference]
36test/miner_tests.cpp:445:110: warning: potential null pointer dereference [-Wnull-dereference]
37test/miner_tests.cpp:458:110: warning: potential null pointer dereference [-Wnull-dereference]
38test/validation_block_tests.cpp:322:67: warning: potential null pointer dereference [-Wnull-dereference]
39txmempool.cpp:1027:55: warning: potential null pointer dereference [-Wnull-dereference]
40txmempool.cpp:1027:87: warning: potential null pointer dereference [-Wnull-dereference]
41validation.cpp:1453:24: warning: potential null pointer dereference [-Wnull-dereference]
42validation.cpp:333:44: warning: potential null pointer dereference [-Wnull-dereference]
43validation.cpp:387:85: warning: potential null pointer dereference [-Wnull-dereference]
44validation.cpp:4416:102: warning: potential null pointer dereference [-Wnull-dereference]
45wallet/db.cpp:32:19: warning: null pointer dereference [-Wnull-dereference]
46wallet/test/wallet_tests.cpp:121:41: warning: potential null pointer dereference [-Wnull-dereference]
47wallet/test/wallet_tests.cpp:41:41: warning: potential null pointer dereference [-Wnull-dereference]
  12. ryanofsky commented at 1:36 pm on November 1, 2019: member

    If you want to continue working on making currently implicit != nullptr assumptions explicit you might want to try compiling with -Wnull-dereference to see what the compiler regards as potential null pointer dereferences.

    I’m not sure there’s much benefit to fixing these disabled warnings relative to the cost of implementing and reviewing the fixes. But there are going to be future PRs, another way of going about them instead of calling functions that return pointers and assert != nullptr at half the call sites, it can be better to write functions that return references and assert or throw internally.

    https://github.com/bitcoin/bitcoin/blob/ddd46293bd83ed6339dd26ddf886d31eefa1a9b5/src/wallet/wallet.h#L761-L762

    https://github.com/bitcoin/bitcoin/blob/ddd46293bd83ed6339dd26ddf886d31eefa1a9b5/src/wallet/rpcdump.cpp#L90-L97

  13. MarcoFalke commented at 5:13 pm on November 1, 2019: member
    Hitting an assertion is always better than a segmentation fault, so while none of the -Wnull-dereference warnings are serious, it still makes sense to get rid of them in the long run. Either by replacing them with references where possible, as suggested by ryanofsky, or adding an assert where references are not applicable.
  14. adamjonas commented at 4:41 pm on November 21, 2019: member
    I’m happy to follow up and keep working on these, but I’d prefer to open a separate PR(s) to address the other instances and maintain a low burden of review.
  15. DrahtBot commented at 0:44 am on May 28, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    No conflicts as of last run.

  16. DrahtBot added the label Needs rebase on Jun 8, 2020
  17. 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 undefined behavior.

Co-Authored-By: danra <danra@users.noreply.github.com>
    b11df4afef
  18. adamjonas force-pushed on Jun 9, 2020
  19. DrahtBot removed the label Needs rebase on Jun 9, 2020
  20. adamjonas commented at 0:36 am on August 16, 2020: member
    Going to close this for lack of interest. Maybe @MarcoFalke or @fanquake could label it up for grabs if someone wants to pick it back up again?
  21. adamjonas closed this on Aug 16, 2020
  22. fanquake added the label Up for grabs on Aug 16, 2020
  23. in src/consensus/tx_verify.cpp:69 in b11df4afef 
    65@@ -66,7 +66,9 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
66         int nCoinHeight = prevHeights[txinIndex];
67 
68         if (txin.nSequence & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) {
69-            int64_t nCoinTime = block.GetAncestor(std::max(nCoinHeight-1, 0))->GetMedianTimePast();
70+            const CBlockIndex* ancestor = block.GetAncestor(std::max(nCoinHeight-1, 0));
    jonatack commented at 10:36 am on August 16, 2020: 
    0- const CBlockIndex* ancestor = block.GetAncestor(std::max(nCoinHeight-1, 0));
1+ const CBlockIndex* ancestor = block.GetAncestor(std::max(nCoinHeight - 1, 0));
  24. in src/test/miner_tests.cpp:513 in b11df4afef 
    508@@ -508,9 +509,12 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
509     // but relative locked txs will if inconsistently added to mempool.
510     // For now these will still generate a valid template until BIP68 soft fork
511     BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3U);
512-    // However if we advance height by 1 and time by 512, all of them should be mined
513-    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++)
514-        ::ChainActive().Tip()->GetAncestor(::ChainActive().Tip()->nHeight - i)->nTime += 512; //Trick the MedianTimePast
515+    // However if we advance height by 1 and time by SEQUENCE_LOCK_TIME, all of them should be mined
516+    for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++) {
    jonatack commented at 10:38 am on August 16, 2020: 
    0- for (int i = 0; i < CBlockIndex::nMedianTimeSpan; i++) {
1+ for (int i = 0; i < CBlockIndex::nMedianTimeSpan; ++i) {
  25. jonatack commented at 10:40 am on August 16, 2020: member

    Apologies, this PR fell off my radar.

    Code review ACK b11df4afefaa3c56c22f570cc4f324cd1c2ac689

    A couple of completely ignorable nits if you re-open and have to retouch.

  26. practicalswift commented at 3:57 pm on August 16, 2020: contributor
    @adamjonas Would you be willing to re-open this given renewed reviewer interest? :)
  27. DrahtBot locked this on Feb 15, 2022
  28. MarcoFalke removed the label Up for grabs on Apr 29, 2022


adamjonas jonatack practicalswift MarcoFalke jnewbery ryanofsky DrahtBot

Labels
Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me