wallet: Uninitialized read in bumpfee(…) #17642

Uninitialized read in bumpfee(…).

The problem can be verified by running test/functional/wallet_bumpfee.py --valgrind (see PR #17633 for --valgrind).

Live demo:

$ test/functional/wallet_bumpfee.py --valgrind --tracerpc
2019-11-30T20:58:24.457000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_x66swmkm
…
-152-> bumpfee ["b8f5472384ca8f1b69c64f058db13d545e3d0b82aec4e777a77087830159ef11", {"fee_rate": 0.0015}]
2019-11-30T21:00:18.358000Z TestFramework (ERROR): Unexpected exception caught during testing
…
ConnectionRefusedError: [Errno 111] Connection refused
$ cat /tmp/bitcoin_func_test_x66swmkm/node1/stderr/*
==17181== Thread 15 b-httpworker.0:
==17181== Conditional jump or move depends on uninitialised value(s)
==17181==    at 0x8F00BC: ValueFromAmount(long const&) (core_write.cpp:21)
==17181==    by 0x76FA48: bumpfee(JSONRPCRequest const&) (rpcwallet.cpp:3482)
==17181==    by 0x375CE2: CRPCCommand::CRPCCommand(char const*, char const*, UniValue (*)(JSONRPCRequest const&), std::initializer_list<char const*>)::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const (server.h:104)
==17181==    by 0x375AE0: std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), CRPCCommand::CRPCCommand(char const*, char const*, UniValue (*)(JSONRPCRequest const&), std::initializer_list<char const*>)::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) (std_function.h:282)
==17181==    by 0x16E0E0: std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const (std_function.h:687)
==17181==    by 0x165D3E: interfaces::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(CRPCCommand const&)::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}::operator()(JSONRPCRequest const&, UniValue&, bool) const (chain.cpp:202)
==17181==    by 0x165B00: std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), interfaces::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(CRPCCommand const&)::{lambda(JSONRPCRequest const&, UniValue&, bool)#1}>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) (std_function.h:282)
==17181==    by 0x16E0E0: std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const (std_function.h:687)
==17181==    by 0x41FF47: ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) (server.cpp:449)
==17181==    by 0x41FBC2: CRPCTable::execute(JSONRPCRequest const&) const (server.cpp:432)
==17181==    by 0x67771B: HTTPReq_JSONRPC(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (httprpc.cpp:190)
==17181==    by 0x336249: std::_Function_handler<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&), bool (*)(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (std_function.h:282)
…

The following diff avoids the uninitialized read but is not a correct fix:

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index d906f6ddf072..d09d08f05480 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -3437,7 +3437,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
 
 
     std::vector<std::string> errors;
-    CAmount old_fee;
+    CAmount old_fee = -1;
     CAmount new_fee;
     CMutableTransaction mtx;
     feebumper::Result res;
@@ -3479,7 +3479,9 @@ static UniValue bumpfee(const JSONRPCRequest& request)
     }
     UniValue result(UniValue::VOBJ);
     result.pushKV("txid", txid.GetHex());
-    result.pushKV("origfee", ValueFromAmount(old_fee));
+    if (MoneyRange(old_fee)) {
+        result.pushKV("origfee", ValueFromAmount(old_fee));
+    }
     result.pushKV("fee", ValueFromAmount(new_fee));
     UniValue result_errors(UniValue::VARR);
     for (const std::string& error : errors) {

A demo of the unitialized read:

$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1401472.87914520
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1400508.05266608
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1404964.24912920
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1E-8
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1398604.42601496

Code:

#!/usr/bin/env python3

from decimal import Decimal

from test_framework.messages import BIP125_SEQUENCE_NUMBER
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import connect_nodes

MIN_RELAY_FEE = Decimal("0.00000141")
FEE_RATE = 0.0015

SEND_AMOUNT_1 = Decimal("49")
SEND_AMOUNT_2 = Decimal("48")


class UninitializedOrigFeeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 2
        self.setup_clean_chain = True
        self.extra_args = [
            ["-walletrbf={}".format(i), "-mintxfee=0.00002", "-addresstype=bech32"]
            for i in range(self.num_nodes)
        ]

    def run_test(self):
        connect_nodes(self.nodes[0], 1)
        self.sync_all()
        peer_node, rbf_node = self.nodes
        rbf_node_address = rbf_node.getnewaddress()
        peer_node.generate(101)
        self.sync_all()
        peer_node.sendtoaddress(rbf_node_address, SEND_AMOUNT_1)
        self.sync_all()
        peer_node.generate(1)
        self.sync_all()
        dest_address = peer_node.getnewaddress()
        tx_input = dict(
            sequence=BIP125_SEQUENCE_NUMBER,
            **next(u for u in rbf_node.listunspent() if u["amount"] == SEND_AMOUNT_1)
        )
        destinations = {dest_address: SEND_AMOUNT_2}
        destinations[rbf_node.getrawchangeaddress()] = (
            SEND_AMOUNT_1 - SEND_AMOUNT_2 - MIN_RELAY_FEE
        )
        rawtx = rbf_node.createrawtransaction([tx_input], destinations)
        signedtx = rbf_node.signrawtransactionwithwallet(rawtx)
        rbfid = rbf_node.sendrawtransaction(signedtx["hex"])
        self.sync_mempools((rbf_node, peer_node))
        bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": FEE_RATE})
        print("origfee: {}".format(bumped_tx["origfee"]))


if __name__ == "__main__":
    UninitializedOrigFeeTest().main()