wallet: Fix uninitialized read in bumpfee(…)

practicalswift commented at 9:19 pm on November 30, 2019: contributor

Fix uninitialized read in bumpfee(…).

The “fix” is tentative so I’m marking this PR as WIP. Wallet people, please chime in: how should the code path where old_fee was being uninitialized be handled? :)

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

Live demo:

 0$ test/functional/wallet_bumpfee.py --valgrind --tracerpc
 12019-11-30T20:58:24.457000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_x66swmkm
 2…
 3-152-> bumpfee ["b8f5472384ca8f1b69c64f058db13d545e3d0b82aec4e777a77087830159ef11", {"fee_rate": 0.0015}]
 42019-11-30T21:00:18.358000Z TestFramework (ERROR): Unexpected exception caught during testing
 5…
 6ConnectionRefusedError: [Errno 111] Connection refused
 7$ cat /tmp/bitcoin_func_test_x66swmkm/node1/stderr/*
 8==17181== Thread 15 b-httpworker.0:
 9==17181== Conditional jump or move depends on uninitialised value(s)
10==17181==    at 0x8F00BC: ValueFromAmount(long const&) (core_write.cpp:21)
11==17181==    by 0x76FA48: bumpfee(JSONRPCRequest const&) (rpcwallet.cpp:3482)
12==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)
13==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)
14==17181==    by 0x16E0E0: std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const (std_function.h:687)
15==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)
16==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)
17==17181==    by 0x16E0E0: std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const (std_function.h:687)
18==17181==    by 0x41FF47: ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) (server.cpp:449)
19==17181==    by 0x41FBC2: CRPCTable::execute(JSONRPCRequest const&) const (server.cpp:432)
20==17181==    by 0x67771B: HTTPReq_JSONRPC(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (httprpc.cpp:190)
21==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)
22…

[wip] wallet: Fix uninitialized read in bumpfee(...) 294d4536cc

fanquake added the label Wallet on Nov 30, 2019

DrahtBot commented at 11:12 pm on November 30, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16373 (bumpfee: Return PSBT when wallet has privkeys disabled by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

instagibbs commented at 4:34 am on December 1, 2019: member

I mean really this is a bug. It looks like old_fee isn’t being set when the user provides an explicit feerate. A test should bear this out. Tests should probably assert that origfee isn’t 0 at a minimum, or get larger on each bump.

practicalswift commented at 10:06 am on December 1, 2019: contributor

@instagibbs Thanks for clarifying. I’m opening this as an issue instead. I don’t know the wallet code good enough to be comfortable fixing this the correct way :)

practicalswift closed this on Dec 1, 2019

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022

wallet: Fix uninitialized read in bumpfee(…) #17640

Conflicts