boost::signals2 disconnections don’t block for slots to finish executing (& CValidationInterface lifecycle races) #18065

1. 
   markblundeberg commented at 1:44 am on February 4, 2020: none

   I’ve noticed a rare use-after-scope race in submitblock while testing Bitcoin ABC built in debug mode, and the involved code is identical to Core so I believe the same concern exists here.

   Essentially the issue is that UnregisterValidationInterface does disconnect the slots promptly, but the slots may be in the process of executing at that precise moment, in the scheduler thread. Even though UpdatedBlockTip is an empty function for the submitblock_StateCatcher sc, it does get wrapped by std::bind and various internal boost wrappers so there’s a tiny time window to have a problem. The race looks something like this (Sched = scheduler thread, RPC = thread calling submitblock function)

   • [Sched] Invoke UpdatedBlockTip signal.
   • [Sched] Observe the connection for UpdatedBlockTip and extract the slot-to-be-executed.
   • [RPC] submitblock disconnects the connection.
   • [Sched] Start executing the slot, starting with various boost wrappers, passing through std::bind, then a call to the CValidationInterface’s virtual UpdatedBlockTip method.
   • [RPC] submitblock function completes; sc is destroyed and zeroed.
   • [Sched] Look up the sc vtable entry for UpdatedBlockTip and call it. Segfault because sc’s vtable pointer has been zeroed.

   Boost signals2 thread safety discussion: https://www.boost.org/doc/libs/1_55_0/doc/html/signals2/thread-safety.html

   Note that since we unlock the connection’s mutex before executing its associated slot, it is possible a slot will still be executing after it has been disconnected by a connection::disconnect(), if the disconnect was called concurrently with signal invocation.

   This was the direct fix in Bitcoin ABC however it does not address the bigger issue: 1) not all signals are queued at the scheduler and so there could be a race with a third thread, and 2) there are other places in the code base (like wallet) that seem to not wait after unregistering. I am not super familiar with signals2 so I’m not sure what is the ‘correct’ fix. But the TL;DR is that scoped_connection only protects the lifecycle of the std::bind and not anything deeper.

   Relevant code

   The scope in submitblock: https://github.com/bitcoin/bitcoin/blob/651e34388832149402fea0d26f3dc13bbe197f5a/src/rpc/mining.cpp#L781-L792 The submitblock_StateCatcher definition: https://github.com/bitcoin/bitcoin/blob/651e34388832149402fea0d26f3dc13bbe197f5a/src/rpc/mining.cpp#L713-L729

2. markblundeberg added the label Bug on Feb 4, 2020
3. 
   markblundeberg commented at 4:18 am on February 4, 2020: none

   A reading of the tutorial suggests that tracking is the correct idiom to use https://www.boost.org/doc/libs/1_61_0/doc/html/signals2/tutorial.html#signals2.tutorial.connection-management … If that idiom were followed, then RegisterValidationInterface should take a shared_ptr and UnregisterValidationInterface is likely not needed at all (connections will clean themselves up).
4. markblundeberg renamed this:
   boost::signals2 disconnections don't block for slots to finish executing (& object lifecycle races)
   boost::signals2 disconnections don't block for slots to finish executing (& CValidationInterface lifecycle races)
   on Feb 4, 2020
5. 
   laanwj commented at 8:05 am on February 5, 2020: member

   Thanks for reporting!
6. MarcoFalke added the label Brainstorming on Feb 5, 2020
7. 
   promag commented at 2:55 pm on February 5, 2020: member

   I’ve done similar change in #15700 but for RegisterValidationInterface to make it atomic. I think such approach for unregister would be better.
8. 
   promag commented at 9:02 am on April 9, 2020: member

   This can be closed. See #18524 and #18551.
9. fanquake closed this on Apr 9, 2020

   ---

10. MarcoFalke locked this on Feb 15, 2022

Contributors
markblundeberg laanwj promag

Labels
Brainstorming Bug