Skip to content

Commit

Permalink
Various ubsan fixes
Browse files Browse the repository at this point in the history
Mostly harmless signed/unsigned conversions but also an actual
memory leak related to `BlindingData`.
  • Loading branch information
apoelstra committed Dec 16, 2020
1 parent 9df968b commit 0c5a216
Show file tree
Hide file tree
Showing 6 changed files with 18 additions and 13 deletions.
6 changes: 5 additions & 1 deletion src/pegins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,11 @@ std::vector<std::pair<CScript, CScript>> 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);

Expand Down
8 changes: 4 additions & 4 deletions src/rpc/rawtransaction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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.");
Expand Down
6 changes: 3 additions & 3 deletions src/test/fuzz/script_flags.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ void test_one_input(const std::vector<uint8_t>& buffer)

try {
const CTransaction tx(deserialize, ds);
tx.witness.vtxinwit.resize(tx.vin.size());

unsigned int verify_flags;
ds >> verify_flags;
Expand All @@ -55,7 +54,8 @@ void test_one_input(const std::vector<uint8_t>& 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
Expand All @@ -67,7 +67,7 @@ void test_one_input(const std::vector<uint8_t>& 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);
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -457,13 +457,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 ? std::make_unique<BlindDetails>() : 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());
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 ? std::make_unique<BlindDetails>() : nullptr;
if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, error, coinControl, fee_calc_out, false, blind_details.get())) {
return false;
}

Expand Down
1 change: 1 addition & 0 deletions test/sanitizer_suppressions/ubsan
Original file line number Diff line number Diff line change
Expand Up @@ -80,5 +80,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

0 comments on commit 0c5a216

Please sign in to comment.