diff --git a/src/gtest/test_validation.cpp b/src/gtest/test_validation.cpp index 13d14892663..8493c83e2f4 100644 --- a/src/gtest/test_validation.cpp +++ b/src/gtest/test_validation.cpp @@ -1,9 +1,11 @@ #include +#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 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 txdata; + txdata.reserve(1); + CMutableTransaction mtx; + mtx.vin.resize(1); + txdata.emplace_back(CTransaction(mtx)); + + CCheckQueueControl control(&queue); + + std::vector 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); +} diff --git a/src/main.cpp b/src/main.cpp index eb98a68c72d..645efd135a3 100644 --- a/src/main.cpp +++ b/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 txdata; + txdata.reserve(block.vtx.size()); // Required so that pointers to individual PrecomputedTransactionData don't get invalidated + CCheckQueueControl 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 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];