Fix use-after-free in ConnectBlock check-queue lifetime (CVE-2024-52911 parity)

src/gtest/test_validation.cpp

@@ -1,9 +1,11 @@
 #include <gtest/gtest.h>
 
+#include "checkqueue.h"
 #include "consensus/upgrades.h"
 #include "consensus/validation.h"
 #include "main.h"
 #include "pow.h"
+#include "script/interpreter.h"
 #include "txdb.h"
 #include "utiltest.h"
 
@@ -371,3 +373,79 @@ TEST(Validation, ChainstateCommitmentBindsCoinMetadata)
     EXPECT_EQ(base.hashSerialized, bumpHeight.hashSerialized);
     EXPECT_NE(base.hashSerializedFull, bumpHeight.hashSerializedFull);
 }
+
+// Regression guard for the CVE-2024-52911-parity use-after-free in ConnectBlock.
+//
+// In ConnectBlock, queued CScriptChecks hold a raw PrecomputedTransactionData*
+// into a local `txdata` vector, and ~CCheckQueueControl() calls Wait() on every
+// (including early) return. Crucially, Wait() drains any still-queued checks ON
+// THE CALLING THREAD (the "master" in CCheckQueue::Loop). So if `txdata` is
+// declared AFTER `control`, reverse-order destruction frees txdata BEFORE
+// ~control's Wait() runs the queued check that dereferences it -> heap UAF.
+//
+// The check below mirrors CScriptCheck: it holds a PrecomputedTransactionData*
+// and dereferences it when run. Because no background worker threads are started,
+// the queued check is guaranteed to execute inside ~control's Wait() on this
+// thread -> the reproduction is DETERMINISTIC, not racy.
+//
+//  * EXPECT_TRUE(ran) deterministically guards the property the fix depends on:
+//    ~CCheckQueueControl drains queued checks. If that contract regresses, the
+//    fix becomes a silent no-op and this fails WITHOUT needing a sanitizer.
+//  * Under AddressSanitizer, swapping the declaration order of `txdata` and
+//    `control` below makes the drained check read freed memory -> ASan reports
+//    heap-use-after-free. Build with: ./configure --with-sanitizers=address
+//
+// NOTE: this guards the lifetime *mechanism*; the exact declaration order inside
+// ConnectBlock itself is guarded by the inline comment there + code review.
+struct LifetimeCheck {
+    const PrecomputedTransactionData* pdata;
+    uint256* sink;
+    bool* ran;
+    LifetimeCheck() : pdata(nullptr), sink(nullptr), ran(nullptr) {}
+    LifetimeCheck(const PrecomputedTransactionData* pdataIn, uint256* sinkIn, bool* ranIn)
+        : pdata(pdataIn), sink(sinkIn), ran(ranIn) {}
+    bool operator()() {
+        // Dereference pdata exactly as CScriptCheck dereferences its txdata.
+        if (pdata != nullptr && sink != nullptr) {
+            *sink = pdata->hashPrevouts;
+        }
+        if (ran != nullptr) {
+            *ran = true;
+        }
+        return true;
+    }
+    void swap(LifetimeCheck& other) {
+        std::swap(pdata, other.pdata);
+        std::swap(sink, other.sink);
+        std::swap(ran, other.ran);
+    }
+};
+
+TEST(Validation, CheckQueueControlDrainsQueuedCheckBeforeTxdataDestroyed) {
+    CCheckQueue<LifetimeCheck> queue(128);
+    uint256 sink;
+    bool ran = false;
+    {
+        // ConnectBlock's FIXED ordering: txdata declared BEFORE control, so
+        // ~control's Wait() (which drains the queued check on this thread) runs
+        // BEFORE txdata is destroyed. Swapping these two lines reproduces the
+        // use-after-free under AddressSanitizer.
+        std::vector<PrecomputedTransactionData> txdata;
+        txdata.reserve(1);
+        CMutableTransaction mtx;
+        mtx.vin.resize(1);
+        txdata.emplace_back(CTransaction(mtx));
+
+        CCheckQueueControl<LifetimeCheck> control(&queue);
+
+        std::vector<LifetimeCheck> vChecks;
+        vChecks.emplace_back(&txdata[0], &sink, &ran);
+        control.Add(vChecks);
+
+        // Simulate an early consensus-failure return (coinbase overpay / bad
+        // Sapling root): leave scope WITHOUT calling control.Wait().
+    }
+    // ~control must have drained the queued check on this thread while txdata
+    // was still alive. If it did not, the fix is void.
+    EXPECT_TRUE(ran);
+}

src/main.cpp

@@ -2613,6 +2613,14 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
 
     CBlockUndo blockundo;
 
+    // txdata must be declared before `control`: queued CScriptChecks hold raw
+    // PrecomputedTransactionData* into this vector, and ~CCheckQueueControl()
+    // calls Wait() for in-flight worker checks on every (including early) return.
+    // Declaring txdata first guarantees reverse-order destruction runs that Wait()
+    // before txdata is destroyed, closing the use-after-free (CVE-2024-52911 parity).
+    std::vector<PrecomputedTransactionData> txdata;
+    txdata.reserve(block.vtx.size()); // Required so that pointers to individual PrecomputedTransactionData don't get invalidated
+
     CCheckQueueControl<CScriptCheck> control(fExpensiveChecks && nScriptCheckThreads ? &scriptcheckqueue : NULL);
 
     int64_t nTimeStart = GetTimeMicros();
@@ -2649,8 +2657,6 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
     // Grab the consensus branch ID for the block's height
     auto consensusBranchId = CurrentEpochBranchId(pindex->nHeight, Params().GetConsensus());
 
-    std::vector<PrecomputedTransactionData> txdata;
-    txdata.reserve(block.vtx.size()); // Required so that pointers to individual PrecomputedTransactionData don't get invalidated
     for (unsigned int i = 0; i < block.vtx.size(); i++)
     {
         const CTransaction &tx = block.vtx[i];