Fix shutdown in case of errors during initialization #11783

pull laanwj wants to merge 1 commits into bitcoin:master from laanwj:2017_11_notify_callbacks_shutdown_crash changing 2 files +8 −3

laanwj commented at 11:12 AM on November 28, 2017: member

PR #10286 introduced a few steps which are not robust to early shutdown in initialization.

Stumbled upon this with #11781, not sure if there are other scenarios that can trigger it, but it's good to harden against this in any case.

E.g.

$ src/bitcoind  -debuglogfile=/dfdf
Error: Could not open debug log file /dfdf
Program received signal SIGSEGV, Segmentation fault.
UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
82          g_signals.m_internals->BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2));
(gdb) bt
[#0](/bitcoin-bitcoin/0/)  UnregisterValidationInterface (pwalletIn=0x0) at /.../bitcoin/src/validationinterface.cpp:82
[#1](/bitcoin-bitcoin/1/)  0x00005555555a11fc in Shutdown () at /.../bitcoin/src/init.cpp:196
[#2](/bitcoin-bitcoin/2/)  0x00005555555961cc in AppInit (argc=<optimized out>, argv=<optimized out>) at /.../bitcoin/src/bitcoind.cpp:183
[#3](/bitcoin-bitcoin/3/)  0x0000555555596249 in main (argc=0, argv=0x555555ecf200) at /.../bitcoin/src/bitcoind.cpp:19

Fix shutdown in case of errors during initialization

PR #10286 introduced a few steps which are not robust to early shutdown
in initialization.

Stumbled upon this with #11781, not sure if there are other scenarios
that can trigger it, but it's harden against this in any case.

d31e5c1d0f

laanwj added the label Docs and Output on Nov 28, 2017
promag commented at 12:21 PM on November 28, 2017: member
In places where m_internals is used, what about adding:
```
assert(m_internals);
```
For instance, in RegisterValidationInterface().
promag commented at 1:50 PM on November 28, 2017: member

Tested ACK d31e5c1.
jnewbery commented at 6:00 PM on November 29, 2017: member

Tested ACK d31e5c1d0f303a8cd97077d425488ed5abdf5345. Looks good to me.

Request review from @TheBlueMatt
TheBlueMatt commented at 6:46 PM on November 29, 2017: member

Except for the peerLogic de-register, given where the validationinterface is given the scheduler in AppInitMain, I'm pretty confident it'd be pretty hard to hit this on current master. Still, good idea to make it more robust, utACK d31e5c1d0f303a8cd97077d425488ed5abdf5345.
laanwj commented at 10:14 AM on November 30, 2017: member

@TheBlueMatt I agree, if it wasn't for changes that error out sooner in AppInitMain, like #11781 we'd probably never have stumbled on these.
laanwj referenced this in commit 16fff80257 on Nov 30, 2017
laanwj merged this on Nov 30, 2017
laanwj closed this on Nov 30, 2017
PastaPastaPasta referenced this in commit a5c3d3331b on Jan 17, 2020
PastaPastaPasta referenced this in commit 05bc8343a7 on Jan 22, 2020
PastaPastaPasta referenced this in commit e4996fc941 on Jan 22, 2020
PastaPastaPasta referenced this in commit 0211cbff80 on Jan 29, 2020
PastaPastaPasta referenced this in commit 696cd2869c on Jan 29, 2020
PastaPastaPasta referenced this in commit 5091ab5bcc on Jan 29, 2020
random-zebra referenced this in commit 2e510ef586 on Feb 18, 2021
ckti referenced this in commit 8ac6f5c0b4 on Mar 28, 2021
gades referenced this in commit b80463372f on Feb 15, 2022
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#1 JSON-RPC support for mobile devices ("ultra-lightweight" clients)#2 Long-term, safe, store-of-value #3 Encrypt wallet #10286 Call wallet notify callbacks in scheduler thread (without cs_main)#11781 Add `-debuglogfile` option