Avoid double free in simplicity pointer when copying PrecomputedTransactionData

Author: psgreco

src/script/interpreter.cpp

@@ -2669,7 +2669,7 @@ void PrecomputedTransactionData::Init(const T& txTo, std::vector<CTxOut>&& spent
         simplicityRawTx.version = txTo.nVersion;
         simplicityRawTx.lockTime = txTo.nLockTime;
 
-        m_simplicity_tx_data = simplicity_elements_mallocTransaction(&simplicityRawTx);
+        m_simplicity_tx_data_ptr = std::make_unique<SimplicityTxData>(&simplicityRawTx);
 
         m_bip341_taproot_ready = true;
     }
@@ -3119,9 +3119,10 @@ bool GenericTransactionSignatureChecker<T>::CheckSimplicity(const valtype& progr
     simplicity_err error;
     tapEnv* simplicityTapEnv = simplicity_elements_mallocTapEnv(&simplicityRawTap);
 
-    assert(txdata->m_simplicity_tx_data);
+    assert(txdata->m_simplicity_tx_data_ptr);
+    assert(txdata->m_simplicity_tx_data_ptr->m_simplicity_tx_data);
     assert(simplicityTapEnv);
-    if (!simplicity_elements_execSimplicity(&error, 0, txdata->m_simplicity_tx_data, nIn, simplicityTapEnv, txdata->m_hash_genesis_block.data(), budget, 0, program.data(), program.size(), witness.data(), witness.size())) {
+    if (!simplicity_elements_execSimplicity(&error, 0, txdata->m_simplicity_tx_data_ptr->m_simplicity_tx_data, nIn, simplicityTapEnv, txdata->m_hash_genesis_block.data(), budget, 0, program.data(), program.size(), witness.data(), witness.size())) {
         assert(!"simplicity_elements_execSimplicity internal error");
     }
     simplicity_elements_freeTapEnv(simplicityTapEnv);

src/script/interpreter.h

@@ -169,9 +169,23 @@ enum : uint32_t {
 
 bool CheckSignatureEncoding(const std::vector<unsigned char> &vchSig, unsigned int flags, ScriptError* serror);
 
+struct SimplicityTxData {
+    transaction* m_simplicity_tx_data = nullptr;
+    // No default assignment operator or copy constructor
+    SimplicityTxData& operator=(const SimplicityTxData&) = delete;
+    SimplicityTxData(const SimplicityTxData&) = delete;
+    SimplicityTxData(rawTransaction* tx) {
+        m_simplicity_tx_data = simplicity_elements_mallocTransaction(tx);
+    };
+    ~SimplicityTxData() {
+        simplicity_elements_freeTransaction(m_simplicity_tx_data);
+        m_simplicity_tx_data = nullptr;
+    };
+};
 struct PrecomputedTransactionData
 {
-    transaction* m_simplicity_tx_data = nullptr;
+    // Make sure that only one copy of the simplicity tx is kept around if the struct is moved
+    std::unique_ptr<SimplicityTxData> m_simplicity_tx_data_ptr;
     // BIP341 precomputed data.
     // These are single-SHA256, see https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-15.
     uint256 m_prevouts_single_hash;
@@ -221,9 +235,6 @@ struct PrecomputedTransactionData
 
     template <class T>
     explicit PrecomputedTransactionData(const T& tx);
-    ~PrecomputedTransactionData() {
-        simplicity_elements_freeTransaction(m_simplicity_tx_data);
-    }
 };
 
 enum class SigVersion

src/test/fuzz/simplicity.cpp

@@ -204,13 +204,14 @@ FUZZ_TARGET_INIT(simplicity, initialize_simplicity)
     PrecomputedTransactionData txdata{GENESIS_HASH};
     std::vector<CTxOut> spent_outs_copy{spent_outs};
     txdata.Init(mtx, std::move(spent_outs_copy));
-    assert(txdata.m_simplicity_tx_data != NULL);
+    assert(txdata.m_simplicity_tx_data_ptr);
+    assert(txdata.m_simplicity_tx_data_ptr->m_simplicity_tx_data);
 
     // 4. Main test
     unsigned char imr_out[32];
     unsigned char *imr = mtx.vin[0].prevout.hash.data()[2] & 2 ? imr_out : NULL;
 
-    const transaction* tx = txdata.m_simplicity_tx_data;
+    const transaction* tx = txdata.m_simplicity_tx_data_ptr->m_simplicity_tx_data;
     tapEnv* taproot = simplicity_elements_mallocTapEnv(&simplicityRawTap);
     simplicity_elements_execSimplicity(&error, imr, tx, nIn, taproot, GENESIS_HASH.data(), budget, amr, prog_bytes.data(), prog_bytes.size(), wit_bytes.data(), wit_bytes.size());
 

src/test/fuzz/simplicity_tx.cpp

@@ -217,7 +217,8 @@ FUZZ_TARGET_INIT(simplicity_tx, initialize_simplicity_tx)
         // that we will allocate Simplicity data. The check for whether to do this is very
         // lax: is this a 34-byte scriptPubKey that starts with OP_1 and does it have a
         // nonempty witness.
-        assert(txdata.m_simplicity_tx_data != NULL);
+        assert(txdata.m_simplicity_tx_data_ptr);
+        assert(txdata.m_simplicity_tx_data_ptr->m_simplicity_tx_data);
     }
 
     const CTransaction tx{mtx};