Make WalletInitInterface and DummyWalletInit private, fix nullptr deref #12836

promag · 2018-03-30T08:00:01Z

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.

jnewbery · 2018-03-30T13:41:03Z

This change fixes the nullpointer dereference here: #10762 (comment)

jamesob · 2018-03-30T13:44:35Z

utACK 208fad6

jnewbery · 2018-03-30T18:43:51Z

src/init.cpp

typo dymmy.

Why does this need to be static?

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

IMO these can have internal linkage. WDYT?

Yes, seems reasonable.

63048ec [tests] Test starting bitcoind with -h and -version (John Newbery) Pull request description: Test that starting bitcoind/bitcoin-qt with `-h` and `-version` works as expected. Prompted by #10762 (comment), which is a nullpointer dereference triggered by starting bitcoin-qt with `-h`. On master, this test passes when run over bitcoind, but fails when running over bitcoin-qt. I used xvfb as a virtual frame buffer to test: ``` BITCOIND=/home/ubuntu/bitcoin/src/qt/bitcoin-qt xvfb-run ./feature_help.py --nocleanup 2018-03-30T17:09:37.767000Z TestFramework (INFO): Initializing test directory /tmp/user/1000/testdi4dre13 2018-03-30T17:09:37.767000Z TestFramework (INFO): Start bitcoin with -h for help text 2018-03-30T17:09:37.841000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main self.run_test() File "./feature_help.py", line 25, in run_test assert_equal(ret_code, 0) File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 39, in assert_equal raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) AssertionError: not(-11 == 0) 2018-03-30T17:09:37.842000Z TestFramework (INFO): Stopping nodes Traceback (most recent call last): File "./feature_help.py", line 42, in <module> HelpTest().main() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 149, in main self.stop_nodes() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 273, in stop_nodes node.stop_node() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 141, in stop_node self.stop() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 87, in __getattr__ assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection" AssertionError: Error: no RPC connection ``` Passes for bitcoind and bitcoin-qt when run on #12836. Longer term, we should consider running functional tests over bitcoin-qt in one of the Travis jobs. Tree-SHA512: 0c2f40f3d5f0e78c3a1b07dbee8fd383eebab27ed0bf2a98a5b9cc66613dbd7b70e363c56163a37e02f68ae7ff7b3ae1769705d0e110ca68a00f8693315730a4

jnewbery · 2018-04-02T13:42:15Z

Tested ACK 57c00059bb51be64a670edebc00416fc2050d5f1 once this has been rebased on master to fix the linter error.

promag · 2018-04-02T14:19:56Z

Rebased.

jamesob · 2018-04-02T15:19:19Z

re-utACK 465a8ee

jnewbery · 2018-04-02T15:46:37Z

Looks good. ReACK 465a8eed157a637391f60d4681a5d7c6a885cdbd

instagibbs · 2018-04-05T19:03:32Z

^ Needs rebase

PierreRochard · 2018-04-05T19:23:54Z

src/init.cpp

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

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

promag · 2018-04-05T20:10:20Z

Rebased.

jamesob · 2018-04-05T20:37:49Z

Tested ACK d894894

For both --enable-wallet and --disable-wallet:

Ran bitcoin-qt -h
Verified preexisting wallet data loaded

PierreRochard · 2018-04-06T13:20:19Z

Tested ACK d894894

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

…ix nullptr deref d894894 wallet: Refactor to WalletInitInterface* const g_wallet_init_interface (João Barbosa) 39bc2fa wallet: Make WalletInitInterface and DummyWalletInit private (João Barbosa) Pull request description: 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. Tree-SHA512: 203c49d8c85252d1bd0ff1d7ed8bcdc842d12d2d396e965cc70be5c8159a62e98ec23d32d2f3dc48a53e575844130d0a7dedac3cc2fe4621d31319b7a1c9ba89

63048ec [tests] Test starting bitcoind with -h and -version (John Newbery) Pull request description: Test that starting bitcoind/bitcoin-qt with `-h` and `-version` works as expected. Prompted by bitcoin#10762 (comment), which is a nullpointer dereference triggered by starting bitcoin-qt with `-h`. On master, this test passes when run over bitcoind, but fails when running over bitcoin-qt. I used xvfb as a virtual frame buffer to test: ``` BITCOIND=/home/ubuntu/bitcoin/src/qt/bitcoin-qt xvfb-run ./feature_help.py --nocleanup 2018-03-30T17:09:37.767000Z TestFramework (INFO): Initializing test directory /tmp/user/1000/testdi4dre13 2018-03-30T17:09:37.767000Z TestFramework (INFO): Start bitcoin with -h for help text 2018-03-30T17:09:37.841000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main self.run_test() File "./feature_help.py", line 25, in run_test assert_equal(ret_code, 0) File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 39, in assert_equal raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) AssertionError: not(-11 == 0) 2018-03-30T17:09:37.842000Z TestFramework (INFO): Stopping nodes Traceback (most recent call last): File "./feature_help.py", line 42, in <module> HelpTest().main() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 149, in main self.stop_nodes() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 273, in stop_nodes node.stop_node() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 141, in stop_node self.stop() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 87, in __getattr__ assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection" AssertionError: Error: no RPC connection ``` Passes for bitcoind and bitcoin-qt when run on bitcoin#12836. Longer term, we should consider running functional tests over bitcoin-qt in one of the Travis jobs. Tree-SHA512: 0c2f40f3d5f0e78c3a1b07dbee8fd383eebab27ed0bf2a98a5b9cc66613dbd7b70e363c56163a37e02f68ae7ff7b3ae1769705d0e110ca68a00f8693315730a4

…vate, fix nullptr deref d894894 wallet: Refactor to WalletInitInterface* const g_wallet_init_interface (João Barbosa) 39bc2fa wallet: Make WalletInitInterface and DummyWalletInit private (João Barbosa) Pull request description: 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. Tree-SHA512: 203c49d8c85252d1bd0ff1d7ed8bcdc842d12d2d396e965cc70be5c8159a62e98ec23d32d2f3dc48a53e575844130d0a7dedac3cc2fe4621d31319b7a1c9ba89 Signed-off-by: pasta <[email protected]> # Conflicts: # src/qt/dash.cpp # src/wallet/init.cpp # src/wallet/init.h # src/walletinitinterface.h

…vate, fix nullptr deref (#3498) d894894 wallet: Refactor to WalletInitInterface* const g_wallet_init_interface (João Barbosa) 39bc2fa wallet: Make WalletInitInterface and DummyWalletInit private (João Barbosa) Pull request description: 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. Tree-SHA512: 203c49d8c85252d1bd0ff1d7ed8bcdc842d12d2d396e965cc70be5c8159a62e98ec23d32d2f3dc48a53e575844130d0a7dedac3cc2fe4621d31319b7a1c9ba89 Signed-off-by: pasta <[email protected]> # Conflicts: # src/qt/dash.cpp # src/wallet/init.cpp # src/wallet/init.h # src/walletinitinterface.h Co-authored-by: Wladimir J. van der Laan <[email protected]>

63048ec [tests] Test starting bitcoind with -h and -version (John Newbery) Pull request description: Test that starting bitcoind/bitcoin-qt with `-h` and `-version` works as expected. Prompted by bitcoin#10762 (comment), which is a nullpointer dereference triggered by starting bitcoin-qt with `-h`. On master, this test passes when run over bitcoind, but fails when running over bitcoin-qt. I used xvfb as a virtual frame buffer to test: ``` BITCOIND=/home/ubuntu/bitcoin/src/qt/bitcoin-qt xvfb-run ./feature_help.py --nocleanup 2018-03-30T17:09:37.767000Z TestFramework (INFO): Initializing test directory /tmp/user/1000/testdi4dre13 2018-03-30T17:09:37.767000Z TestFramework (INFO): Start bitcoin with -h for help text 2018-03-30T17:09:37.841000Z TestFramework (ERROR): Assertion failed Traceback (most recent call last): File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main self.run_test() File "./feature_help.py", line 25, in run_test assert_equal(ret_code, 0) File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 39, in assert_equal raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args)) AssertionError: not(-11 == 0) 2018-03-30T17:09:37.842000Z TestFramework (INFO): Stopping nodes Traceback (most recent call last): File "./feature_help.py", line 42, in <module> HelpTest().main() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 149, in main self.stop_nodes() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 273, in stop_nodes node.stop_node() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 141, in stop_node self.stop() File "/home/ubuntu/bitcoin/test/functional/test_framework/test_node.py", line 87, in __getattr__ assert self.rpc_connected and self.rpc is not None, "Error: no RPC connection" AssertionError: Error: no RPC connection ``` Passes for bitcoind and bitcoin-qt when run on bitcoin#12836. Longer term, we should consider running functional tests over bitcoin-qt in one of the Travis jobs. Tree-SHA512: 0c2f40f3d5f0e78c3a1b07dbee8fd383eebab27ed0bf2a98a5b9cc66613dbd7b70e363c56163a37e02f68ae7ff7b3ae1769705d0e110ca68a00f8693315730a4

fanquake added the Wallet label Mar 30, 2018

jnewbery mentioned this pull request Mar 30, 2018

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

Merged

jnewbery reviewed Mar 30, 2018

View reviewed changes

fanquake mentioned this pull request Mar 31, 2018

bitcoin-qt dumps core during --help #12848

Closed

promag force-pushed the 2018-03-walletinit branch from c027672 to 57c0005 Compare April 1, 2018 22:38

jnewbery mentioned this pull request Apr 2, 2018

Trivial: Add logging line in init.cpp that was accidentally removed with #10762 #12845

Merged

promag force-pushed the 2018-03-walletinit branch from 57c0005 to 465a8ee Compare April 2, 2018 14:18

laanwj self-assigned this Apr 5, 2018

maflcko changed the title ~~Make WalletInitInterface and DummyWalletInit private~~ Make WalletInitInterface and DummyWalletInit private, fix nullptr deref Apr 5, 2018

PierreRochard reviewed Apr 5, 2018

View reviewed changes

promag added 2 commits April 5, 2018 21:09

wallet: Make WalletInitInterface and DummyWalletInit private

39bc2fa

wallet: Refactor to WalletInitInterface* const g_wallet_init_interface

d894894

promag force-pushed the 2018-03-walletinit branch from 465a8ee to d894894 Compare April 5, 2018 20:09

laanwj added the Bug label Apr 7, 2018

laanwj merged commit d894894 into bitcoin:master Apr 7, 2018

PierreRochard mentioned this pull request Sep 11, 2018

wallet: Remove trailing separators from -walletdir arg #14146

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Make WalletInitInterface and DummyWalletInit private, fix nullptr deref #12836

Make WalletInitInterface and DummyWalletInit private, fix nullptr deref #12836

Uh oh!

Conversation

promag commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Mar 30, 2018

Uh oh!

jnewbery Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

promag Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Apr 2, 2018

Uh oh!

promag commented Apr 2, 2018

Uh oh!

jamesob commented Apr 2, 2018

Uh oh!

jnewbery commented Apr 2, 2018

Uh oh!

instagibbs commented Apr 5, 2018

Uh oh!

PierreRochard Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

jamesob Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

promag Apr 5, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Apr 5, 2018

Uh oh!

jamesob commented Apr 5, 2018

Uh oh!

PierreRochard commented Apr 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

promag commented Mar 30, 2018 •

edited

Loading

jnewbery commented Mar 30, 2018 •

edited

Loading