Make sure all callers of LookupBlockIndex(...) check for nullptr before dereferencing (CBlockIndex*) #13969

practicalswift commented at 7:24 PM on August 14, 2018: contributor

Make sure all callers of LookupBlockIndex(...) check for nullptr before dereferencing (CBlockIndex*).

DrahtBot commented at 9:32 PM on August 14, 2018: member

Reviewers, this pull request conflicts with the following ones:

#14437 (Refactor: Start to separate wallet from node by ryanofsky)
#10973 (Refactor: separate wallet from node by ryanofsky)

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.

in src/rpc/blockchain.cpp:883 in 5983a8d9ed outdated

 879 | @@ -880,7 +880,11 @@ static bool GetUTXOStats(CCoinsView *view, CCoinsStats &stats)
 880 |      stats.hashBlock = pcursor->GetBestBlock();
 881 |      {
 882 |          LOCK(cs_main);
 883 | -        stats.nHeight = LookupBlockIndex(stats.hashBlock)->nHeight;
 884 | +        CBlockIndex* blockIndex = LookupBlockIndex(stats.hashBlock);

domob1812 commented at 6:04 AM on August 15, 2018:

Nit: You could make this const CBlockIndex*.

in src/validation.cpp:1349 in 5983a8d9ed outdated

1346 | @@ -1347,6 +1347,7 @@ int GetSpendHeight(const CCoinsViewCache& inputs)
1347 |  {
1348 |      LOCK(cs_main);
1349 |      CBlockIndex* pindexPrev = LookupBlockIndex(inputs.GetBestBlock());

domob1812 commented at 6:05 AM on August 15, 2018:

While you are here, you could make this const.

in src/net_processing.cpp:3442 in 5983a8d9ed outdated

3438 | @@ -3439,6 +3439,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3439 |              }
3440 |              if (!fRevertToInv && !vHeaders.empty()) {
3441 |                  if (vHeaders.size() == 1 && state.fPreferHeaderAndIDs) {
3442 | +                    assert(pBestIndex);

domob1812 commented at 6:06 AM on August 15, 2018:

This is a matter of taste, but I think that the more explicit pBestIndex != nullptr is more readable as to the intent. I'm not sure if there's a style rule for how to test pointers against null which goes against that, though.

in src/wallet/rpcwallet.cpp:102 in 5983a8d9ed outdated

  98 | @@ -99,7 +99,10 @@ static void WalletTxToJSON(const CWalletTx& wtx, UniValue& entry)
  99 |      {
 100 |          entry.pushKV("blockhash", wtx.hashBlock.GetHex());
 101 |          entry.pushKV("blockindex", wtx.nIndex);
 102 | -        entry.pushKV("blocktime", LookupBlockIndex(wtx.hashBlock)->GetBlockTime());
 103 | +        CBlockIndex* blockIndex = LookupBlockIndex(wtx.hashBlock);

domob1812 commented at 6:07 AM on August 15, 2018:

Can also be const.

domob1812 approved

domob1812 commented at 6:07 AM on August 15, 2018: contributor

A few nits, but otherwise utACK 5983a8d9eda7a3e53c4d9ec715f802d434ba7cf8.

practicalswift force-pushed on Aug 15, 2018

practicalswift commented at 1:46 PM on August 15, 2018: contributor

@domob1812 Thanks for reviewing. Feedback addressed. Please re-review :-)

in src/rpc/blockchain.cpp:1071 in 1c02054c61 outdated

1067 | @@ -1064,6 +1068,9 @@ UniValue gettxout(const JSONRPCRequest& request)
1068 |      }
1069 |  
1070 |      const CBlockIndex* pindex = LookupBlockIndex(pcoinsTip->GetBestBlock());
1071 | +    if (!pindex) {

promag commented at 10:25 PM on August 15, 2018:

Is this even possible? If so then it's new behaviour, should have a test, otherwise should be an assertion.

in src/net_processing.cpp:3442 in 1c02054c61 outdated

3438 | @@ -3439,6 +3439,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
3439 |              }
3440 |              if (!fRevertToInv && !vHeaders.empty()) {
3441 |                  if (vHeaders.size() == 1 && state.fPreferHeaderAndIDs) {
3442 | +                    assert(pBestIndex != nullptr);

promag commented at 10:28 PM on August 15, 2018:

Unrelated to this PR. Above you see that if vHeaders.push is called then pBestIndex = pindex was called. IMO this should be removed.

practicalswift commented at 9:01 AM on August 16, 2018:

You don't mean that the assertion should be removed? How do you mean "unrelated to this PR" in this context? :-)

promag commented at 9:03 AM on August 16, 2018:

This is not an assertion of a LookupBlockIndex result, that's done in L3400.

in src/rpc/blockchain.cpp:884 in 1c02054c61 outdated

 879 | @@ -880,7 +880,11 @@ static bool GetUTXOStats(CCoinsView *view, CCoinsStats &stats)
 880 |      stats.hashBlock = pcursor->GetBestBlock();
 881 |      {
 882 |          LOCK(cs_main);
 883 | -        stats.nHeight = LookupBlockIndex(stats.hashBlock)->nHeight;
 884 | +        const CBlockIndex* blockIndex = LookupBlockIndex(stats.hashBlock);
 885 | +        if (!blockIndex) {

promag commented at 10:31 PM on August 15, 2018:

Is this even possible? If not, should be an assertion.

in src/wallet/rpcwallet.cpp:103 in 1c02054c61 outdated

  98 | @@ -99,7 +99,10 @@ static void WalletTxToJSON(const CWalletTx& wtx, UniValue& entry)
  99 |      {
 100 |          entry.pushKV("blockhash", wtx.hashBlock.GetHex());
 101 |          entry.pushKV("blockindex", wtx.nIndex);
 102 | -        entry.pushKV("blocktime", LookupBlockIndex(wtx.hashBlock)->GetBlockTime());
 103 | +        const CBlockIndex* blockIndex = LookupBlockIndex(wtx.hashBlock);
 104 | +        if (blockIndex) {

promag commented at 10:32 PM on August 15, 2018:

Should be an assertion? How can it be nullptr since confirms > 0?

promag commented at 10:42 PM on August 15, 2018: member

This is a matter of taste, but I think that the more explicit pBestIndex != nullptr is more readable as to the intent

Not sure but I think the most frequent is assert(pointer) so I would go that way. If you feel otherwise then submit a PR to update developer notes first?

practicalswift force-pushed on Aug 16, 2018

practicalswift commented at 10:24 AM on August 16, 2018: contributor

@promag Thanks for reviewing! Feedback addressed. Please re-review :-)

in src/wallet/rpcwallet.cpp:4008 in 8dd6609eca outdated

4004 | @@ -4003,6 +4005,7 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
4005 |          }
4006 |          // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex
4007 |          stopBlock = pindexStop ? pindexStop : pChainTip;
4008 | +        assert(stopBlock != nullptr);

promag commented at 2:15 PM on August 16, 2018:

Why this assertion?

practicalswift commented at 2:29 PM on August 16, 2018:

@promag stopBlock is dereferenced a few lines down:

response.pushKV("stop_height", stopBlock->nHeight);

promag commented at 2:44 PM on August 16, 2018:

And that's fine, if by any chance stopBlock is null then the process will terminate. In this simple code the assertion is pointless IMO.

promag commented at 2:20 PM on August 16, 2018: member

This PR only changes how the process terminates in case there is some bug. An early assertion makes sense when there are side effects until the dereferencing, which I believe is not the case here.

Also, see #13969#pullrequestreview-146286551 above.

practicalswift commented at 2:41 PM on August 16, 2018: contributor

@promag I'm not sure I follow. The assumption underlying your reasoning seems to be that the only difference between …

assert(foo);
foo->bar();

… and …

// assert(foo);
foo->bar();

... at runtime would be the line number at which the process terminates (given foo == nullptr). That seems like a very risky assumption to make? :-)

promag commented at 2:45 PM on August 16, 2018: member

@practicalswift I said

which I believe is not the case here.

practicalswift commented at 2:51 PM on August 16, 2018: contributor

@promag Ah, I think I misunderstood what you wrote. You're simply suggesting that the assertions should be made right before dereferencing and not earlier (assuming no side effects before dereferencing)?

I misunderstood and thought that you meant that the only potential problem caused by null pointer dereferencing would be process termination :-)

Make sure all callers of LookupBlockIndex(...) check for nullptr before dereferencing (CBlockIndex*) 75f360cfd8

practicalswift force-pushed on Aug 16, 2018

promag commented at 3:05 PM on August 16, 2018: member

You're simply suggesting that the assertions should be made right before dereferencing and not earlier

I mean that an assertion makes sense to prevent side effects before dereferencing.

I also think that these particular assertions are not that relevant or useful.

IMHO of course, and I'd wait for more reviews.

ryanofsky commented at 3:36 PM on August 24, 2018: member

I don't think it would be a good idea to make this change without adding a style guideline about when it is actually recommended to assert that a pointer is not null before dereferencing.

Personally, I don't like these asserts because I think they are distracting and make the code verbose and awkward to read, but I guess the benefit of adding them would be to have error messages that make it easier to determine the causes of crashes when core dumps aren't available. Another benefit might be that if there was a guideline requiring asserts before pointer dereferences, it might encourage more uses of c++ references instead of pointers, which I think would be good.

Will add my code utACK 75f360cfd8e69656bebe195e64b4ad1b1b069ae6 if other people want this change, but personally would like it better if the change was dropped or accompanied by a developer guideline.

promag commented at 7:55 PM on September 13, 2018: member

it might encourage more uses of c++ references instead of pointers @ryanofsky +1

practicalswift closed this on Oct 19, 2018

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 18, 2022