Fix invalid checks (NULL checks after dereference, redundant checks, etc.) #10898

practicalswift commented at 7:49 PM on July 21, 2017: contributor

Contains:

Remove redundant NULL checks after throwing new
Remove redundant check (!ecc is always true)
Remove duplicate uriParts.size() > 0 check
Use two boolean literals instead of re-using variable

fanquake added the label Refactoring on Jul 21, 2017

in src/rest.cpp:432 in 53d37d2579 outdated

 426 | @@ -427,9 +427,8 @@ static bool rest_getutxos(HTTPRequest* req, const std::string& strURIPart)
 427 |  
 428 |      if (uriParts.size() > 0)
 429 |      {
 430 | -
 431 |          //inputs is sent over URI scheme (/rest/getutxos/checkmempool/txid1-n/txid2-n/...)
 432 | -        if (uriParts.size() > 0 && uriParts[0] == "checkmempool")
 433 | +        if (uriParts[0] == "checkmempool")
 434 |              fCheckMemPool = true;

benma commented at 7:27 AM on July 24, 2017:

add braces or put on same line according do the dev notes.

in src/net_processing.cpp:426 in 7d10df6528 outdated

 424 |                  // As per BIP152, we only get 3 of our peers to announce
 425 |                  // blocks using compact encodings.
 426 | -                connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion](CNode* pnodeStop){
 427 | -                    connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
 428 | +                connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, nCMPCTBLOCKVersion](CNode* pnodeStop) {
 429 | +                    connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, false, nCMPCTBLOCKVersion));

benma commented at 7:30 AM on July 24, 2017:

Now the code is less readable, as I have to guess or look up what false means here. I'd restore the var in this case (same issue below).

practicalswift commented at 2:16 PM on July 24, 2017:

Fixed by adding two comments :-)

in src/bitcoin-tx.cpp:685 in 6ce3cb6234 outdated

 686 | @@ -687,18 +687,18 @@ static void MutateTx(CMutableTransaction& tx, const std::string& command,
 687 |      else if (command == "outaddr")
 688 |          MutateTxAddOutAddr(tx, commandVal);
 689 |      else if (command == "outpubkey") {
 690 | -        if (!ecc) { ecc.reset(new Secp256k1Init()); }
 691 | +        ecc.reset(new Secp256k1Init());

benma commented at 7:39 AM on July 24, 2017:

A bit off-topic (removing the !ecc check looks good):

The usage of Secp256k1Init is pretty obscure here. Why is it needed for those few commands but not others?
Would it be a better idea to move the Secp256k1Init into the functions that are called? As close as possible to where it is actually required, which is not hard to see now.

Pinging @sipa, the author of this.

sipa commented at 10:05 PM on July 24, 2017:

Some libsecp256k1 calls require a context, and some don't. The secp256k1.h header explains which ones, but perhaps that information should be duplicated into at least the pubkey.h/key.h interfaces?

in src/qt/walletview.cpp:128 in deaf4219e2 outdated

 121 | @@ -122,11 +122,12 @@ void WalletView::setWalletModel(WalletModel *_walletModel)
 122 |      overviewPage->setWalletModel(_walletModel);
 123 |      receiveCoinsPage->setModel(_walletModel);
 124 |      sendCoinsPage->setModel(_walletModel);
 125 | -    usedReceivingAddressesPage->setModel(_walletModel->getAddressTableModel());
 126 | -    usedSendingAddressesPage->setModel(_walletModel->getAddressTableModel());
 127 |  
 128 |      if (_walletModel)

benma commented at 7:45 AM on July 24, 2017:

After a quick research it looks like _walletModel is never passed as NULL. So it would be good to change the argument from a pointer to a reference and remove this check.

benma changes_requested

practicalswift force-pushed on Jul 24, 2017

practicalswift commented at 2:17 PM on July 24, 2017: contributor

@benma Thanks for reviewing! Good feedback. All items addressed. Looks good? :-)

in src/net_processing.cpp:426 in da5f591c03 outdated

 424 |                  // As per BIP152, we only get 3 of our peers to announce
 425 |                  // blocks using compact encodings.
 426 | -                connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion](CNode* pnodeStop){
 427 | -                    connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
 428 | +                connman.ForNode(lNodesAnnouncingHeaderAndIDs.front(), [&connman, nCMPCTBLOCKVersion](CNode* pnodeStop){
 429 | +                    connman.PushMessage(pnodeStop, CNetMsgMaker(pnodeStop->GetSendVersion()).Make(NetMsgType::SENDCMPCT, /*fAnnounceUsingCMPCTBLOCK=*/false, nCMPCTBLOCKVersion));

benma commented at 8:38 PM on July 24, 2017:

I am not sure whether I like this 😆

Maybe nicer to define two constants for true/false, or an enum?

I commend the effort to reduce the number of lines, but in some cases, being descriptive is worth it.

utACK anyway, I'll leave the decision to you.

benma commented at 8:38 PM on July 24, 2017: none

utACK da5f591c03e04cfc159aa9d8ed1036576f51aa25

practicalswift force-pushed on Aug 14, 2017

practicalswift commented at 3:49 PM on August 14, 2017: contributor

Rebased!

practicalswift force-pushed on Sep 10, 2017

practicalswift commented at 7:11 PM on September 10, 2017: contributor

Rebased! :-)

Remove redundant NULL checks after new 55224af6bd

Remove redundant check (!ecc is always true) 7466991670

Remove duplicate uriParts.size() > 0 check b5fb33943f

Use two boolean literals instead of re-using variable 4971a9a3c9

in src/qt/walletview.h:51 in 3509b82744 outdated

  47 | @@ -48,7 +48,7 @@ class WalletView : public QStackedWidget
  48 |          The wallet model represents a bitcoin wallet, and offers access to the list of transactions, address book and sending
  49 |          functionality.
  50 |      */
  51 | -    void setWalletModel(WalletModel *walletModel);
  52 | +    void setWalletModel(WalletModel& walletModel);

laanwj commented at 1:31 PM on October 2, 2017:

Please don't change the setWalletModel() interface. It is supposed to be consistent over all objects, and setting a NULL model is meaningful: it should be possible to use the view classes without a model for simple testing.

practicalswift force-pushed on Oct 2, 2017

practicalswift commented at 2:00 PM on October 2, 2017: contributor

@laanwj I've now removed the Move NULL check prior to dereference commit.

Please note that setting a NULL model will result in a null pointer dereference in current master. Should I fix that?

What about:

diff --git a/src/qt/walletview.cpp b/src/qt/walletview.cpp
index 971f5e0..a56a400 100644
--- a/src/qt/walletview.cpp
+++ b/src/qt/walletview.cpp
@@ -122,8 +122,8 @@ void WalletView::setWalletModel(WalletModel *_walletModel)
     overviewPage->setWalletModel(_walletModel);
     receiveCoinsPage->setModel(_walletModel);
     sendCoinsPage->setModel(_walletModel);
-    usedReceivingAddressesPage->setModel(_walletModel->getAddressTableModel());
-    usedSendingAddressesPage->setModel(_walletModel->getAddressTableModel());
+    usedReceivingAddressesPage->setModel(_walletModel ? _walletModel->getAddressTableModel() : nullptr);
+    usedSendingAddressesPage->setModel(_walletModel ? _walletModel->getAddressTableModel() : nullptr);

laanwj commented at 3:03 PM on October 2, 2017: member

@practicalshift Yes that should be fixed, looks good to me!

Avoid NULL pointer dereference when _walletModel is NULL (which is valid) 76fed838f3

practicalswift commented at 6:34 PM on October 2, 2017: contributor

@laanwj I've now added a NULL check for _walletModel. Looks good now? :-)

ryanofsky commented at 1:11 PM on October 3, 2017: member

utACK 76fed838f381a6efba175f3650ec7e9dc73016d3 @practicalswift, how are you finding these changes? Are you using a linter tool? Or methodically going through the source code in some way and reviewing it?

practicalswift commented at 2:11 PM on October 4, 2017: contributor

@ryanofsky I use a combination of manual code review, static analysis and fuzzing to find potential issues. In the Apple Swift project most of my results have been from fuzzing (using my own compiler fuzzer): see my apple/swift contributions here.

ryanofsky commented at 2:38 PM on October 4, 2017: member

Thanks. It could be educational (and also nice to give credit) if you mentioned the specific static analysis & fuzzing tools were that used in your commit messages and PR descriptions.

promag commented at 2:45 PM on October 4, 2017: member

Agree with @ryanofsky, also for useful when reviewing!

JeremyRubin commented at 2:50 PM on October 4, 2017: contributor

Echoing @ryanofsky, would even be useful to fill this info in on already merged PRs (people often refer to them long after the fact).

practicalswift commented at 3:34 PM on October 4, 2017: contributor

@ryanofsky @promag @JeremyRubin Sure! Thanks for showing interest! That is appreciated.

Here are pointers to some of the open source tools that I use regularly:

clang's Thread Safety Analysis - see PR #11226 (please review!) + #10866 (please review!)
libFuzzer - see PR #10440 (please review!)
afl-fuzz - see PR #10415 and #10409
valgrind - see PR #11035 (please review!)
clang-tidy - see PR #10735
clang-format - see PR #10602
pycodestyle - see PR #9508
clang's AddressSanitizer (ASan: -fsanitize=address)
clang's LeakSanitizer (LSan: -fsanitize=leak)
clang's MemorySanitizer (MSan: -fsanitize=memory)
clang's ThreadSanitizer (TSan: -fsanitize=thread)
clang's UndefinedBehaviorSanitizer (UBSan: -fsanitize=undefined)
flintpp
oclint
cppcheck

The list is not at all exhaustive - I try to use as many tools as possible to increase diversity in the classes of issues covered. In addition to the above I use some custom/proprietary analysis tools/scripts, but they are not that interesting or general.

If you want to help, please review the open PR:s mentioned above to increase the chance of them getting merged :-)

ryanofsky commented at 6:19 PM on October 12, 2017: member

Can this be merged? Has two utACKs and is a small, minor change.

laanwj commented at 9:55 PM on October 12, 2017: member

Looks good to me, utACK 76fed83

laanwj merged this on Oct 12, 2017

laanwj closed this on Oct 12, 2017

laanwj referenced this in commit 470c730e3f on Oct 12, 2017

PastaPastaPasta referenced this in commit 9e6edd6aa8 on Dec 22, 2019

PastaPastaPasta referenced this in commit 1aa5f0ecc9 on Jan 2, 2020

PastaPastaPasta referenced this in commit 7e261a4386 on Jan 4, 2020

PastaPastaPasta referenced this in commit bf33268a00 on Jan 12, 2020

PastaPastaPasta referenced this in commit d5e3836049 on Jan 12, 2020

PastaPastaPasta referenced this in commit 5ecdfab973 on Jan 12, 2020

PastaPastaPasta referenced this in commit 13a5f694c9 on Jan 12, 2020

PastaPastaPasta referenced this in commit 222d0c4edd on Jan 12, 2020

PastaPastaPasta referenced this in commit d66fb573f4 on Jan 12, 2020

PastaPastaPasta referenced this in commit e7c27f3b77 on Jan 16, 2020

ckti referenced this in commit f93bc35311 on Mar 28, 2021

practicalswift deleted the branch on Apr 10, 2021

furszy referenced this in commit 60d36292bc on Jul 26, 2021

gades referenced this in commit 4c856539c4 on Jan 29, 2022

DrahtBot locked this on Aug 18, 2022