From 14ff89103608f50a109cad112840cfd75f52a325 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Mon, 14 Dec 2020 14:58:05 +0000 Subject: [PATCH] Various ubsan fixes Mostly harmless signed/unsigned conversions but also an actual memory leak related to `BlindingData`. --- src/pegins.cpp | 6 +++++- src/rpc/rawtransaction.cpp | 8 ++++---- src/test/fuzz/script_flags.cpp | 6 +++--- src/wallet/rpcwallet.cpp | 6 +++--- src/wallet/wallet.cpp | 4 ++-- test/sanitizer_suppressions/ubsan | 1 + 6 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/pegins.cpp b/src/pegins.cpp index 77432eab46..6c22d86bda 100644 --- a/src/pegins.cpp +++ b/src/pegins.cpp @@ -468,7 +468,11 @@ std::vector> GetValidFedpegScripts(const CBlockIndex } // Next we walk backwards up to M epoch starts - for (size_t i = 0; i < params.total_valid_epochs; i++) { + for (int32_t i = 0; i < (int32_t) params.total_valid_epochs; i++) { + // We are within total_valid_epochs of the genesis + if (i * epoch_length > epoch_start_height) { + break; + } const CBlockIndex* p_epoch_start = pblockindex->GetAncestor(epoch_start_height-i*epoch_length); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index b73dd0e1b3..96b1ffa9c3 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1107,7 +1107,7 @@ static RPCHelpMan blindpsbt() // How many are we trying to blind? int num_pubkeys = 0; - unsigned int keyIndex = -1; + unsigned int keyIndex = (unsigned) -1; for (unsigned int i = 0; i < output_pubkeys.size(); i++) { const CPubKey& key = output_pubkeys[i]; if (key.IsValid()) { @@ -2324,7 +2324,7 @@ static RPCHelpMan rawblindrawtransaction() // How many are we trying to blind? int num_pubkeys = 0; - unsigned int keyIndex = -1; + unsigned int keyIndex = (unsigned) -1; for (unsigned int i = 0; i < output_pubkeys.size(); i++) { const CPubKey& key = output_pubkeys[i]; if (key.IsValid()) { @@ -2689,13 +2689,13 @@ static RPCHelpMan rawreissueasset() throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid asset address provided: %s", asset_address_uni.get_str())); } - size_t input_index = -1; + int input_index = -1; const UniValue& input_index_o = issuance_o["input_index"]; if (input_index_o.isNum()) { input_index = input_index_o.get_int(); if (input_index < 0) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Input index must be non-negative."); - } else if (input_index >= mtx.vin.size()) { + } else if ((size_t) input_index >= mtx.vin.size()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Input index must exist in transaction."); } else if (!mtx.vin[input_index].assetIssuance.IsNull()) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Selected transaction input already has issuance data."); diff --git a/src/test/fuzz/script_flags.cpp b/src/test/fuzz/script_flags.cpp index 276f0d2d1f..e219ba2ff6 100644 --- a/src/test/fuzz/script_flags.cpp +++ b/src/test/fuzz/script_flags.cpp @@ -31,7 +31,6 @@ void test_one_input(const std::vector& buffer) try { const CTransaction tx(deserialize, ds); - tx.witness.vtxinwit.resize(tx.vin.size()); unsigned int verify_flags; ds >> verify_flags; @@ -55,7 +54,8 @@ void test_one_input(const std::vector& buffer) const TransactionSignatureChecker checker{&tx, i, prevout.nValue, txdata}; ScriptError serror; - const bool ret = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, &tx.witness.vtxinwit.at(i).scriptWitness, verify_flags, checker, &serror); + const CScriptWitness *script_witness = tx.witness.vtxinwit.size() > i ? &tx.witness.vtxinwit[i].scriptWitness : nullptr; + const bool ret = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, script_witness, verify_flags, checker, &serror); assert(ret == (serror == SCRIPT_ERR_OK)); // Verify that removing flags from a passing test or adding flags to a failing test does not change the result @@ -67,7 +67,7 @@ void test_one_input(const std::vector& buffer) if (!IsValidFlagCombination(verify_flags)) return; ScriptError serror_fuzzed; - const bool ret_fuzzed = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, &tx.witness.vtxinwit.at(i).scriptWitness, verify_flags, checker, &serror_fuzzed); + const bool ret_fuzzed = VerifyScript(tx.vin.at(i).scriptSig, prevout.scriptPubKey, script_witness, verify_flags, checker, &serror_fuzzed); assert(ret_fuzzed == (serror_fuzzed == SCRIPT_ERR_OK)); assert(ret_fuzzed == ret); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index cd6ebba6c7..36c89d4227 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -463,13 +463,13 @@ UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std bilingual_str error; CTransactionRef tx; FeeCalculation fee_calc_out; - BlindDetails* blind_details = g_con_elementsmode ? new BlindDetails() : NULL; + auto blind_details = g_con_elementsmode ? MakeUnique() : nullptr; if (blind_details) blind_details->ignore_blind_failure = ignore_blind_fail; - bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS), blind_details); + bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, fee_calc_out, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS), blind_details.get()); if (!fCreated) { throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original); } - pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */, blind_details); + pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */, blind_details.get()); if (verbose) { UniValue entry(UniValue::VOBJ); entry.pushKV("txid", tx->GetHash().GetHex()); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 527275ed14..26223b4ce4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2980,8 +2980,8 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC CTransactionRef tx_new; FeeCalculation fee_calc_out; - BlindDetails* blind_details = g_con_elementsmode ? new BlindDetails() : NULL; - if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, fee_calc_out, false, blind_details)) { + auto blind_details = g_con_elementsmode ? MakeUnique() : nullptr; + if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, fee_calc_out, false, blind_details.get())) { return false; } diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index 69b086ba29..ca7f9b2804 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -79,5 +79,6 @@ implicit-unsigned-integer-truncation:leveldb/* implicit-integer-sign-change:crc32c/* implicit-signed-integer-truncation:script/interpreter.cpp +implicit-integer-sign-change:primitives/block.h implicit-integer-sign-change:primitives/confidential.cpp implicit-integer-sign-change:primitives/confidential.h