Make WalletInitInterface and DummyWalletInit private, fix nullptr deref #12836

pull promag wants to merge 2 commits into bitcoin:master from promag:2018-03-walletinit changing 10 files +58 −87

promag commented at 8:00 AM on March 30, 2018: member

Implementations of WalletInitInterface don't have to be public, so make them private. This makes the interface instantiation static. Also reduces ENABLE_WALLET usage and removes the unnecessary src/wallet/init.h header.
fanquake added the label Wallet on Mar 30, 2018
jnewbery commented at 1:41 PM on March 30, 2018: member

This change fixes the nullpointer dereference here: #10762#commitcomment-28345993
jamesob commented at 1:44 PM on March 30, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12836/commits/208fad6b5e6c5f19a917e0dd06a75927281f31eb
in src/init.cpp:91 in 208fad6b5e outdated
```
  87 | +    void Flush() override {}
  88 | +    void Stop() override {}
  89 | +    void Close() override {}
  90 | +};
  91 | +
  92 | +static DummyWalletInit g_dymmy_wallet_init;
```
jnewbery commented at 6:43 PM on March 30, 2018:

typo dymmy.

Why does this need to be static?

promag commented at 9:02 PM on March 30, 2018:

Dummy typo 😄It doesn't have to be static. IMO these can have internal linkage. WDYT?

jnewbery commented at 1:39 PM on April 2, 2018:

IMO these can have internal linkage. WDYT?

Yes, seems reasonable.
MarcoFalke referenced this in commit d454e39f2a on Apr 1, 2018
promag force-pushed on Apr 1, 2018
jnewbery commented at 1:42 PM on April 2, 2018: member

Tested ACK 57c00059bb51be64a670edebc00416fc2050d5f1 once this has been rebased on master to fix the linter error.
promag force-pushed on Apr 2, 2018
promag commented at 2:19 PM on April 2, 2018: member

Rebased.
jamesob commented at 3:19 PM on April 2, 2018: member

re-utACK https://github.com/bitcoin/bitcoin/pull/12836/commits/465a8eed157a637391f60d4681a5d7c6a885cdbd
jnewbery commented at 3:46 PM on April 2, 2018: member

Looks good. ReACK 465a8eed157a637391f60d4681a5d7c6a885cdbd
instagibbs commented at 7:03 PM on April 5, 2018: member

<MarcoFalke> ^ Needs rebase
laanwj assigned laanwj on Apr 5, 2018
MarcoFalke renamed this:
~~Make WalletInitInterface and DummyWalletInit private~~
Make WalletInitInterface and DummyWalletInit private, fix nullptr deref
on Apr 5, 2018
wallet: Make WalletInitInterface and DummyWalletInit private 39bc2faa2e
wallet: Refactor to WalletInitInterface* const g_wallet_init_interface d894894aab

in src/init.cpp:77 in 465a8eed15 outdated

  71 | @@ -72,7 +72,25 @@ static const bool DEFAULT_STOPAFTERBLOCKIMPORT = false;
  72 |  
  73 |  std::unique_ptr<CConnman> g_connman;
  74 |  std::unique_ptr<PeerLogicValidation> peerLogic;
  75 | -std::unique_ptr<WalletInitInterface> g_wallet_init_interface;
  76 | +
  77 | +#if !(ENABLE_WALLET)
  78 | +class DummyWalletInit : public WalletInitInterface {

PierreRochard commented at 7:23 PM on April 5, 2018:

What's the reason for moving this class from src/walletinitinterface.h?

jamesob commented at 7:57 PM on April 5, 2018:

I think the idea is to narrow the scope of this class to where it's needed and reduce unnecessary header bloat.

promag commented at 8:06 PM on April 5, 2018:

Right.

promag force-pushed on Apr 5, 2018
promag commented at 8:10 PM on April 5, 2018: member

Rebased.
jamesob commented at 8:37 PM on April 5, 2018: member
Tested ACK https://github.com/bitcoin/bitcoin/pull/12836/commits/d894894aab78dbd5e32267313237648933248377

For both --enable-wallet and --disable-wallet:
- Ran bitcoin-qt -h
- Verified preexisting wallet data loaded
PierreRochard commented at 1:20 PM on April 6, 2018: contributor

Tested ACK d894894aab78dbd5e32267313237648933248377

Did the same as @jamesob and manually added in logging to DummyWalletInit's Open() to see that it is being used with --disable-wallet and not being used with --enable-wallet
laanwj added the label Bug on Apr 7, 2018
laanwj merged this on Apr 7, 2018
laanwj closed this on Apr 7, 2018
laanwj referenced this in commit 9abdb7c67e on Apr 7, 2018
PastaPastaPasta referenced this in commit 0ae2a12ba0 on Mar 14, 2020
PastaPastaPasta referenced this in commit 4879de76ef on Mar 19, 2020
PastaPastaPasta referenced this in commit 599df2b325 on Mar 21, 2020
PastaPastaPasta referenced this in commit 5fc1686919 on Mar 24, 2020
PastaPastaPasta referenced this in commit 2acf2a058e on May 20, 2020
UdjinM6 referenced this in commit e78db55314 on May 20, 2020
UdjinM6 referenced this in commit 0662f170f0 on May 20, 2020
ckti referenced this in commit 6efccd83cb on Mar 28, 2021
ckti referenced this in commit f10aa62724 on Mar 28, 2021
gades referenced this in commit 368a862171 on Jun 30, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12843 [tests] Test starting bitcoind with -h and -version