Skip to content

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

Closed
practicalswift wants to merge 1 commit intobitcoin:masterfrom
practicalswift:uninitialized-read-in-bumpfee
Closed

wallet: Fix uninitialized read in bumpfee(…)#17640
practicalswift wants to merge 1 commit intobitcoin:masterfrom
practicalswift:uninitialized-read-in-bumpfee

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Nov 30, 2019 •
edited
Loading

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:

$ 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)
…

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 30, 2019

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
Copy link
Member

instagibbs commented Dec 1, 2019 •
edited
Loading

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
Copy link
Contributor Author

practicalswift commented Dec 1, 2019

@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 practicalswift mentioned this pull request Mar 8, 2020
Merged
@practicalswift practicalswift deleted the uninitialized-read-in-bumpfee branch April 10, 2021 19:39
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@practicalswift @DrahtBot @instagibbs @fanquake