Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

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

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

@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