shutdown: Stop threads before resetting ptrs #13894

MarcoFalke commented at 4:13 PM on August 6, 2018: member

On shutdown some threads would continue to run after or during a pointer reset. This leads to occasional segfaults on shutdown.

Fix this by resetting the smart pointers after all threads that might read from them have been stopped.

This should fix:

A segfault in the txindex thread, that occurs when the txindex destructor is done, but the thread was not yet stopped (as this is done in the base index destructor)
A segfault in the scheduler thread, which dereferences conman. (e.g. CheckForStaleTipAndEvictPeers)

MarcoFalke force-pushed on Aug 6, 2018

DrahtBot commented at 4:49 PM on August 6, 2018: member

Note to reviewers: This pull request conflicts with the following ones:

#12934 ([net] [validation] Call ProcessNewBlock() asynchronously by skeees)

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.

shutdown: Stop threads before resetting ptrs faab63111d

in src/init.cpp:222 in fa89c6339f outdated

 217 | @@ -222,6 +218,12 @@ void Shutdown()
 218 |      threadGroup.interrupt_all();
 219 |      threadGroup.join_all();
 220 |  
 221 | +    // After the threads that potentially access these pointers have been stopped,
 222 | +    // desctruct and reset all to nullptr.

Empact commented at 7:52 PM on August 6, 2018:

nit: destruct

MarcoFalke force-pushed on Aug 6, 2018

MarcoFalke commented at 9:51 PM on August 6, 2018: member

For reference on current master:

2018-08-06T21:43:45Z Shutdown: In progress...
2018-08-06T21:43:45Z dnsseed thread start
2018-08-06T21:43:45Z net thread start
2018-08-06T21:43:45Z msghand thread start
2018-08-06T21:43:45Z addcon thread start
2018-08-06T21:43:45Z opencon thread start
2018-08-06T21:43:45Z addcon thread exit
2018-08-06T21:43:45Z net thread exit
2018-08-06T21:43:45Z torcontrol thread exit
2018-08-06T21:43:45Z opencon thread exit
2018-08-06T21:43:45Z dnsseed thread exit
2018-08-06T21:43:45Z msghand thread exit
==14658== Thread 9 bitcoin-schedule:
==14658== Invalid read of size 8
==14658==    at 0x238DD4: PeerLogicValidation::CheckForStaleTipAndEvictPeers(Consensus::Params const&) (net_processing.cpp:3207)

...

2018-08-06T21:43:47Z scheduler thread interrupt
2018-08-06T21:43:47Z Dumped mempool: 0.00263s to copy, 0.017917s to dump
2018-08-06T21:43:47Z Shutdown: done

and

2018-08-06T21:49:55Z Shutdown: In progress...
2018-08-06T21:49:55Z txindex thread start
pure virtual method called
terminate called without an active exception
==14993== 
==14993== Process terminating with default action of signal 6 (SIGABRT): dumping core
==14993==    at 0x7115FEB: raise (in /usr/lib64/libc-2.27.so)
==14993==    by 0x71005C0: abort (in /usr/lib64/libc-2.27.so)
==14993==    by 0x660EA9A: ??? (in /usr/lib64/libstdc++.so.6.0.25)
==14993==    by 0x6614EFB: ??? (in /usr/lib64/libstdc++.so.6.0.25)
==14993==    by 0x6614F56: std::terminate() (in /usr/lib64/libstdc++.so.6.0.25)
==14993==    by 0x6615DA4: __cxa_pure_virtual (in /usr/lib64/libstdc++.so.6.0.25)
==14993==    by 0x387AF0: BaseIndex::ThreadSync() (base.cpp:140)
==14993==    by 0x1F4DBE: operator() (std_function.h:687)
==14993==    by 0x1F4DBE: void TraceThread<std::function<void ()> >(char const*, std::function<void ()>) (util.h:329)
==14993==    by 0x388BA6: __invoke_impl<void, void (*)(char const*, std::function<void()>), char const*, std::_Bind<void (BaseIndex::*(BaseIndex*))()> > (invoke.h:60)
==14993==    by 0x388BA6: __invoke<void (*)(char const*, std::function<void()>), char const*, std::_Bind<void (BaseIndex::*(BaseIndex*))()> > (invoke.h:95)
==14993==    by 0x388BA6: _M_invoke<0, 1, 2> (thread:234)
==14993==    by 0x388BA6: operator() (thread:243)
==14993==    by 0x388BA6: std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(char const*, std::function<void ()>), char const*, std::_Bind<void (BaseIndex::*(BaseIndex*))()> > > >::_M_run() (thread:186)
==14993==    by 0x6641522: ??? (in /usr/lib64/libstdc++.so.6.0.25)
==14993==    by 0x6EC7593: start_thread (in /usr/lib64/libpthread-2.27.so)
==14993==    by 0x71D90DE: clone (in /usr/lib64/libc-2.27.so)
==14993== 
==14993== HEAP SUMMARY:
==14993==     in use at exit: 59,700,169 bytes in 84,309 blocks
==14993==   total heap usage: 98,728 allocs, 14,419 frees, 88,664,198 bytes allocated
==14993== 
==14993== LEAK SUMMARY:
==14993==    definitely lost: 9,085,501 bytes in 81,256 blocks
==14993==    indirectly lost: 856 bytes in 8 blocks
==14993==      possibly lost: 13,139,949 bytes in 33 blocks
==14993==    still reachable: 37,473,863 bytes in 3,012 blocks
==14993==                       of which reachable via heuristic:
==14993==                         newarray           : 48 bytes in 1 blocks
==14993==         suppressed: 0 bytes in 0 blocks
==14993== Rerun with --leak-check=full to see details of leaked memory
==14993== 
==14993== For counts of detected and suppressed errors, rerun with: -v
==14993== Use --track-origins=yes to see where uninitialised values come from
==14993== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
Aborted (core dumped)

in src/init.cpp:212 in faab63111d

 208 | @@ -209,11 +209,7 @@ void Shutdown()
 209 |      // using the other before destroying them.
 210 |      if (peerLogic) UnregisterValidationInterface(peerLogic.get());
 211 |      if (g_connman) g_connman->Stop();
 212 | -    peerLogic.reset();
 213 | -    g_connman.reset();
 214 | -    if (g_txindex) {
 215 | -        g_txindex.reset();
 216 | -    }
 217 | +    if (g_txindex) g_txindex->Stop();

domob1812 commented at 7:37 AM on August 7, 2018:

Since this is newly added, was it missing completely (independent of the position of pointer resets)?

MarcoFalke commented at 10:31 AM on August 7, 2018:

The base destructor was calling it, but yes, imo it was missing.

domob1812 commented at 12:26 PM on August 7, 2018:

Thanks for fixing then especially!

MarcoFalke commented at 3:46 PM on August 7, 2018:

To clarify: right now it would be called twice. Once here and then another time (as belt-and-supenders) in the destructor when the pointer is reset. (Just like for conman)

Though, the belt-and-suspenders are not enough and can lead to segfaults occasionally. (See above valgrind output)

domob1812 commented at 7:37 AM on August 7, 2018: contributor

utACK faab63111d8f27335aa1f09c1a48da3be45135de

laanwj added the label Bug on Aug 7, 2018

laanwj added this to the milestone 0.17.0 on Aug 7, 2018

achow101 commented at 12:45 AM on August 8, 2018: member

utACK faab63111d8f27335aa1f09c1a48da3be45135de

ken2812221 referenced this in commit df9f712746 on Aug 8, 2018

laanwj merged this on Aug 8, 2018

laanwj closed this on Aug 8, 2018

laanwj commented at 1:21 PM on August 8, 2018: member

utACK faab63111d8f27335aa1f09c1a48da3be45135de

MarcoFalke deleted the branch on Aug 8, 2018

MarcoFalke referenced this in commit e8061831e8 on Aug 27, 2018

codablock referenced this in commit 35b041ed14 on Mar 23, 2020

codablock referenced this in commit 4e8f4ea202 on Mar 24, 2020

random-zebra referenced this in commit ac523660b4 on Feb 21, 2021

ckti referenced this in commit 88860b3fce on Mar 28, 2021

PastaPastaPasta referenced this in commit 0f3eb52994 on Jun 27, 2021

PastaPastaPasta referenced this in commit 00d2dbdfc1 on Jun 28, 2021

PastaPastaPasta referenced this in commit bebc5867b9 on Jun 29, 2021

PastaPastaPasta referenced this in commit 973195435c on Jun 29, 2021

PastaPastaPasta referenced this in commit c4987e742a on Jun 29, 2021

PastaPastaPasta referenced this in commit f61686093d on Jun 29, 2021

PastaPastaPasta referenced this in commit 289873d39e on Jun 29, 2021

Munkybooty referenced this in commit bd0763196a on Jun 30, 2021

Munkybooty referenced this in commit d74b916e77 on Jun 30, 2021

Munkybooty referenced this in commit c7fb300024 on Jul 1, 2021

PastaPastaPasta referenced this in commit 6bf1fbbf09 on Jul 1, 2021

DrahtBot locked this on Sep 8, 2021