Merge #18742: miner: Avoid stack-use-after-return in validationinterface

7777f2a4bb miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
fa5ceb25fc test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue (MarcoFalke)
fa770ce7fe validationinterface: Rework documentation, Rename pwalletIn to callbacks (MarcoFalke)
fab6d060ce test: Add unregister_validation_interface_race test (MarcoFalke)

Pull request description:

  When a validationinterface has itself unregistered in one thread, but is about to get executed in another thread [1], there is a race:

  * The validationinterface destructing itself
  * The validationinterface getting dereferenced for execution

  [1] 64139803f1/src/validationinterface.cpp (L82-L83)

  This happens in the miner. More generally it happens everywhere where at least one thread is generating notifications and another one is unregistering a validationinterface.

  This issue has been fixed in commit ab31b9d6fe, but the fix has not been applied to the miner.

  Example where this happened in practice: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675322230#L4414

ACKs for top commit:
  promag:
    Code review ACK 7777f2a4bb.
  laanwj:
    Code review ACK 7777f2a4bb

Tree-SHA512: 8087119243c71ba18a823a63515f3730d127162625d8729024278b447af29e2ff206f4840ee3d90bf84f93a2c5ab73b76c7e7044c83aa93b5b51047a166ec3d3

src/rpc/mining.cpp

@@ -874,7 +874,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
     return result;
 }
 
-class submitblock_StateCatcher : public CValidationInterface
+class submitblock_StateCatcher final : public CValidationInterface
 {
 public:
     uint256 hash;
@@ -942,17 +942,17 @@ static UniValue submitblock(const JSONRPCRequest& request)
     }
 
     bool new_block;
-    submitblock_StateCatcher sc(block.GetHash());
+    auto sc = std::make_shared<submitblock_StateCatcher>(block.GetHash());
-    RegisterValidationInterface(&sc);
+    RegisterSharedValidationInterface(sc);
     bool accepted = ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block);
-    UnregisterValidationInterface(&sc);
+    UnregisterSharedValidationInterface(sc);
     if (!new_block && accepted) {
         return "duplicate";
     }
-    if (!sc.found) {
+    if (!sc->found) {
         return "inconclusive";
     }
-    return BIP22ValidationResult(sc.state);
+    return BIP22ValidationResult(sc->state);
 }
 
 static UniValue submitheader(const JSONRPCRequest& request)

src/test/validation_block_tests.cpp

@@ -32,7 +32,7 @@ struct MinerTestingSetup : public RegTestingSetup {
 
 BOOST_FIXTURE_TEST_SUITE(validation_block_tests, MinerTestingSetup)
 
-struct TestSubscriber : public CValidationInterface {
+struct TestSubscriber final : public CValidationInterface {
     uint256 m_expected_tip;
 
     explicit TestSubscriber(uint256 tip) : m_expected_tip(tip) {}
@@ -175,8 +175,8 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
         LOCK(cs_main);
         initial_tip = ::ChainActive().Tip();
     }
-    TestSubscriber sub(initial_tip->GetBlockHash());
+    auto sub = std::make_shared<TestSubscriber>(initial_tip->GetBlockHash());
-    RegisterValidationInterface(&sub);
+    RegisterSharedValidationInterface(sub);
 
     // create a bunch of threads that repeatedly process a block generated above at random
     // this will create parallelism and randomness inside validation - the ValidationInterface
@@ -204,14 +204,12 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     for (auto& t : threads) {
         t.join();
     }
-    while (GetMainSignals().CallbacksPending() > 0) {
+    SyncWithValidationInterfaceQueue();
-        UninterruptibleSleep(std::chrono::milliseconds{100});
-    }
 
-    UnregisterValidationInterface(&sub);
+    UnregisterSharedValidationInterface(sub);
 
     LOCK(cs_main);
-    BOOST_CHECK_EQUAL(sub.m_expected_tip, ::ChainActive().Tip()->GetBlockHash());
+    BOOST_CHECK_EQUAL(sub->m_expected_tip, ::ChainActive().Tip()->GetBlockHash());
 }
 
 /**

src/test/validationinterface_tests.cpp

@@ -12,6 +12,40 @@
 
 BOOST_FIXTURE_TEST_SUITE(validationinterface_tests, TestingSetup)
 
+struct TestSubscriberNoop final : public CValidationInterface {
+    void BlockChecked(const CBlock&, const BlockValidationState&) override {}
+};
+
+BOOST_AUTO_TEST_CASE(unregister_validation_interface_race)
+{
+    std::atomic<bool> generate{true};
+
+    // Start thread to generate notifications
+    std::thread gen{[&] {
+        const CBlock block_dummy;
+        const BlockValidationState state_dummy;
+        while (generate) {
+            GetMainSignals().BlockChecked(block_dummy, state_dummy);
+        }
+    }};
+
+    // Start thread to consume notifications
+    std::thread sub{[&] {
+        // keep going for about 1 sec, which is 250k iterations
+        for (int i = 0; i < 250000; i++) {
+            auto sub = std::make_shared<TestSubscriberNoop>();
+            RegisterSharedValidationInterface(sub);
+            UnregisterSharedValidationInterface(sub);
+        }
+        // tell the other thread we are done
+        generate = false;
+    }};
+
+    gen.join();
+    sub.join();
+    BOOST_CHECK(!generate);
+}
+
 class TestInterface : public CValidationInterface
 {
 public:

src/validationinterface.cpp

@@ -89,22 +89,26 @@ public:
 
 static CMainSignals g_signals;
 
-void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler) {
+void CMainSignals::RegisterBackgroundSignalScheduler(CScheduler& scheduler)
+{
     assert(!m_internals);
     m_internals.reset(new MainSignalsInstance(&scheduler));
 }
 
-void CMainSignals::UnregisterBackgroundSignalScheduler() {
+void CMainSignals::UnregisterBackgroundSignalScheduler()
+{
     m_internals.reset(nullptr);
 }
 
-void CMainSignals::FlushBackgroundCallbacks() {
+void CMainSignals::FlushBackgroundCallbacks()
+{
     if (m_internals) {
         m_internals->m_schedulerClient.EmptyQueue();
     }
 }
 
-size_t CMainSignals::CallbacksPending() {
+size_t CMainSignals::CallbacksPending()
+{
     if (!m_internals) return 0;
     return m_internals->m_schedulerClient.CallbacksPending();
 }
@@ -114,10 +118,11 @@ CMainSignals& GetMainSignals()
     return g_signals;
 }
 
-void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> pwalletIn) {
+void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> callbacks)
-    // Each connection captures pwalletIn to ensure that each callback is
+{
-    // executed before pwalletIn is destroyed. For more details see #18338.
+    // Each connection captures the shared_ptr to ensure that each callback is
-    g_signals.m_internals->Register(std::move(pwalletIn));
+    // executed before the subscriber is destroyed. For more details see #18338.
+    g_signals.m_internals->Register(std::move(callbacks));
 }
 
 void RegisterValidationInterface(CValidationInterface* callbacks)
@@ -132,24 +137,28 @@ void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> c
     UnregisterValidationInterface(callbacks.get());
 }
 
-void UnregisterValidationInterface(CValidationInterface* pwalletIn) {
+void UnregisterValidationInterface(CValidationInterface* callbacks)
+{
     if (g_signals.m_internals) {
-        g_signals.m_internals->Unregister(pwalletIn);
+        g_signals.m_internals->Unregister(callbacks);
     }
 }
 
-void UnregisterAllValidationInterfaces() {
+void UnregisterAllValidationInterfaces()
+{
     if (!g_signals.m_internals) {
         return;
     }
     g_signals.m_internals->Clear();
 }
 
-void CallFunctionInValidationInterfaceQueue(std::function<void ()> func) {
+void CallFunctionInValidationInterfaceQueue(std::function<void()> func)
+{
     g_signals.m_internals->m_schedulerClient.AddToProcessQueue(std::move(func));
 }
 
-void SyncWithValidationInterfaceQueue() {
+void SyncWithValidationInterfaceQueue()
+{
     AssertLockNotHeld(cs_main);
     // Block until the validation queue drains
     std::promise<void> promise;

src/validationinterface.h

@@ -22,20 +22,20 @@ class CValidationInterface;
 class uint256;
 class CScheduler;
 
-// These functions dispatch to one or all registered wallets
+/** Register subscriber */
-
+void RegisterValidationInterface(CValidationInterface* callbacks);
-/** Register a wallet to receive updates from core */
+/** Unregister subscriber. DEPRECATED. This is not safe to use when the RPC server or main message handler thread is running. */
-void RegisterValidationInterface(CValidationInterface* pwalletIn);
+void UnregisterValidationInterface(CValidationInterface* callbacks);
-/** Unregister a wallet from core */
+/** Unregister all subscribers */
-void UnregisterValidationInterface(CValidationInterface* pwalletIn);
-/** Unregister all wallets from core */
 void UnregisterAllValidationInterfaces();
 
 // Alternate registration functions that release a shared_ptr after the last
 // notification is sent. These are useful for race-free cleanup, since
 // unregistration is nonblocking and can return before the last notification is
 // processed.
+/** Register subscriber */
 void RegisterSharedValidationInterface(std::shared_ptr<CValidationInterface> callbacks);
+/** Unregister subscriber */
 void UnregisterSharedValidationInterface(std::shared_ptr<CValidationInterface> callbacks);
 
 /**