rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure

kallewoof commented at 5:05 am on March 6, 2020: member

Initialize the nFeeRequired variable to avoid using an uninitialized value for errors happening before it is set to 0.

Note: this originally fixed nFeeRet in wallet.cpp.

fanquake added the label Wallet on Mar 6, 2020

practicalswift commented at 4:22 pm on March 6, 2020: contributor

Concept ACK

Good catch!

FWIW I still think it would be a good idea to consider moving towards a “initialize-to-zero-or-sane-value-by-default” policy. Realistically and empirically it seems like that is the only way we’ll permanently avoid the steady stream of uninitialised reads we’re seeing in the project.

From the C++ Core Guidlines (edited by Herb Sutter & Bjarne Stroustrup):

ES.20: Always initialize an object

Reason: Avoid used-before-set errors and their associated undefined behavior. Avoid problems with comprehension of complex initialization. Simplify refactoring.

The “always initialize rule” is deliberately stronger than the an “object must be set before used” language rule. The latter, more relaxed rule, catches the technical bugs, but:

It leads to less readable code

It encourages people to declare names in greater than necessary scopes

It leads to harder to read code

It leads to logic bugs by encouraging complex code

It hampers refactoring

J F Bastien of Clang/LLVM fame from the “Automatic variable initialization” LLVM patch thread:

Is it a good idea? Security-minded folks think so, and apparently so does the Microsoft Visual Studio team [1] who say “Between 2017 and mid 2018, this feature would have killed 49 MSRC cases that involved uninitialized struct data leaking across a trust boundary. It would have also mitigated a number of bugs involving uninitialized struct data being used directly.”. They seem to use pure zero initialization, and claim to have taken the overheads down to within noise. Don’t just trust Microsoft though, here’s another relevant person asking for this [2]. It’s been proposed for GCC [3] and LLVM [4] before.

Kees (“Case”) Cook of the Kernel Self-Protection Project (KSPP):

Always-initialized local variables: just do it

Kostya Serebryany - the father of AddressSanitizer, libFuzzer, OSS-Fuzz and Clang Control Flow Integrity - on the topic of auto-initialization in Clang/LLVM:

Very exciting, and long overdue. Thanks for doing this! Countless security bugs would have been mitigated by this, see below. […] Here are some links to bugs, vulnerabilities and full exploits based on uses of uninitialized memory. The list is not exhaustive by any means, and we keep finding them every day. The problem is, of course, that we don’t find all of them.

I know about the “but if you initialize then Valgrind won’t find the issue” argument. I love Valgrind and MSan - I use them daily, but we’re facing a trade-off here: I think the security benefits of avoiding uninitialized reads by initializing outweighs the discoverability benefits of not initializing.

sanjaykdragon commented at 4:55 pm on March 6, 2020: contributor

FWIW I still think it would be a good idea to consider moving towards a “initialize-to-zero-or-sane-value-by-default” policy. Realistically and empirically it seems like that is the only way we’ll permanently avoid the steady stream of uninitialized reads we’re seeing in the project.

I agree with this, would it be worth the time to start refactoring code to initialize to zero/sane?

Sjors commented at 5:19 pm on March 6, 2020: member

Concept ACK. Don’t forget to remove the original from line 2664. Can you also initialize nFeeRequired to 0?

Maybe instead of initializing inside CreateTransaction() you could assert that it’s zero?

I agree with this, would it be worth the time to start refactoring code to initialize to zero/sane?

Pure refactor PRs usually don’t work well. There is already a code guideline to do this in new places, and in code that’s being touched by a PR: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures

practicalswift commented at 6:21 pm on March 6, 2020: contributor

I agree with this, would it be worth the time to start refactoring code to initialize to zero/sane?

Pure refactor PRs usually don’t work well.

Can a type of change that is known to have prevented literally countless security bugs in other projects (had the type of change been in place) really be called a “pure refactoring”? :)

Perhaps mitigation is a better term.

MarcoFalke commented at 6:45 pm on March 6, 2020: member

NACK. I have a hard time following the discussion here and why people seem to agree that this is a good change. My rationale for the NACK:

Reading a return value in the case of failure, where it was not properly written to, is always illegal. Setting it to 0 or any other value does not make it legal to read it. IIUC, In this case it will print Error: This transaction requires a transaction fee of at least 0, which is obviously the wrong error message.
Blind Zero-or-otherwise initialization completely defeats the purpose of valgrind and other address sanitizers to find these bugs. When we remove the chance to fix these bugs, we might as well remove valgrind et al again.

So NACK on hiding the bug and not actually fixing it. If it is too hard to fix the bug without major code changes, we are lucky that C++ offers us many ways to safely return values from functions that may fail:

Optional, which is not initialized on error. Only initialized when successfully written to.
Return an enum to distinguish different error cases or maybe even pass out the error string, so that the caller can simply use that error string and does not have to guess.

MarcoFalke commented at 6:47 pm on March 6, 2020: member

I am pretty sure we have had this exact discussion about uninitialized reads the 7th time. I wonder if it helps to write up a policy about that.

MarcoFalke commented at 6:50 pm on March 6, 2020: member

FWIW I still think it would be a good idea to consider moving towards a “initialize-to-zero-or-sane-value-by-default” policy. Realistically and empirically it seems like that is the only way we’ll permanently avoid the steady stream of uninitialized reads we’re seeing in the project.

I agree with this, would it be worth the time to start refactoring code to initialize to zero/sane?

If we do this for all builds, we might as well remove valgrind et al (as I mentioned above). I think it could make sense to enable it for release builds, but I see no way where it could make sense to enable this for developer builds.

MarcoFalke commented at 6:57 pm on March 6, 2020: member

If we decide to do initialization everywhere, it should be done with the ftrivial-auto-var-init compiler flag (or similar), and not in the code.

ryanofsky commented at 7:10 pm on March 6, 2020: member

Tend to agree with Marco and think that a policy of initializing variables to 0 or -1 in cases where these values would still be bugs could be counterproductive and reduce our ability to find these bugs with fuzzing and static analysis.

A different rule to either declare every variable with a valid initial value or to declare it as std::optional could be preferable because it could remove nondeterminism while still allowing tools to detect unset values. It could also improve code review because the need to dereference would alert reviewers to the possibility that a variable might be unset.

laanwj commented at 10:59 pm on March 6, 2020: member

ACK b585873f252577697dafd1a3f8cd7ef82869377f This is clearly correct and fixes an actual (though minor) issue.

kallewoof force-pushed on Mar 7, 2020

kallewoof commented at 2:02 am on March 7, 2020: member

Pushed removal of duplicate nFeeRet = 0; statement.

kallewoof commented at 2:06 am on March 7, 2020: member

@MarcoFalke I see what you’re saying, but I’m not sure how trivial/hard it would be to actually track down the cases where the current “error message override” behavior is triggered. Also, I don’t think

Reading a return value in the case of failure, where it was not properly written to, is always illegal. Setting it to 0 or any other value does not make it legal to read it. IIUC, In this case it will print Error: This transaction requires a transaction fee of at least 0, which is obviously the wrong error message.

is correct, because nValue + nFeeRequired > curBalance will not be true (for nFeeRequired = 0 case, we already test this on L#339).

DrahtBot commented at 2:47 am on March 7, 2020: 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:

#18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)

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.

practicalswift commented at 9:43 am on March 7, 2020: contributor

@kallewoof

Are you sure there is no reasonable way to fix this at the call site instead? I think that would be much preferred. I’m a bit hesitant to ACK the code as currently written.

Generally, given bool f(T& o) I really think we should avoid assuming that f has written to o in the case that f(o) has returned false.

Context for reviewers - this is the call site:

 0    // Create and send the transaction                                                                                                                                                                      
 1    CAmount nFeeRequired;
 2    std::string strError;
 3    std::vector<CRecipient> vecSend;
 4    int nChangePosRet = -1;
 5    CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount};
 6    vecSend.push_back(recipient);
 7    CTransactionRef tx;
 8    if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strError, coin_control)) {
 9        if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance)
10            strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired));
11        throw JSONRPCError(RPC_WALLET_ERROR, strError);
12    }

AFAICT that is the only place in our code base where an uninitialized nFeeRequired parameter is passed to [cC]reateTransaction and subsequently read.

 0$ git grep -E '[Cc]reateTransaction\(' -- "*.h" "*.cpp"
 1src/interfaces/wallet.cpp:    CTransactionRef createTransaction(const std::vector<CRecipient>& recipients,
 2src/interfaces/wallet.cpp:        if (!m_wallet->CreateTransaction(*locked_chain, recipients, tx, fee, change_pos,
 3src/interfaces/wallet.h:    virtual CTransactionRef createTransaction(const std::vector<CRecipient>& recipients,
 4src/qt/walletmodel.cpp:        newTx = m_wallet->createTransaction(vecSend, coinControl, !privateKeysDisabled() /* sign */, nChangePosRet, nFeeRequired, strFailReason);
 5src/wallet/feebumper.cpp:    if (!wallet.CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
 6src/wallet/rpcwallet.cpp:    if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strError, coin_control)) {
 7src/wallet/rpcwallet.cpp:    bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strFailReason, coin_control);
 8src/wallet/test/wallet_tests.cpp:            BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, fee, changePos, error, dummy));
 9src/wallet/wallet.cpp:    if (!CreateTransaction(*locked_chain, vecSend, tx_new, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) {
10src/wallet/wallet.cpp:bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet,
11src/wallet/wallet.h:     * calling CreateTransaction();
12src/wallet/wallet.h:    bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut,

kallewoof commented at 10:38 am on March 7, 2020: member

@practicalswift Yeah, either way works, to be honest, but since we are currently using the result in one place, there’s no guarantee we won’t use it in another in the future. That’s my only reasoning, really.

promag commented at 1:13 am on March 9, 2020: member

+1 on fixing at the call site. A refactor could come next IMO.

kallewoof force-pushed on Mar 9, 2020

kallewoof renamed this:
~~wallet: set nFeeRet to 0 to avoid garbage value upon error~~
rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure
on Mar 9, 2020

rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure a652ba6293

kallewoof commented at 1:22 am on March 9, 2020: member

I have changed the code & description of the PR to fix in rpcwallet.cpp instead.

promag commented at 1:25 am on March 9, 2020: member

ACK a652ba6293ef8d144935dc882b5f0003c987fa22.

meshcollider commented at 7:14 am on March 9, 2020: contributor

utACK a652ba6293ef8d144935dc882b5f0003c987fa22

practicalswift commented at 9:28 am on March 9, 2020: contributor

ACK a652ba6293ef8d144935dc882b5f0003c987fa22 – patch looks correct

Sjors commented at 10:35 am on March 9, 2020: member

utACK a652ba6293ef8d144935dc882b5f0003c987fa22

Generally, given bool f(T& o) I really think we should avoid assuming that f has written to o in the case that f(o) has returned false.

That sounds reasonable, and this fix is in line with that.

fanquake merged this on Mar 9, 2020

fanquake closed this on Mar 9, 2020

kallewoof deleted the branch on Mar 12, 2020

MarkLTZ referenced this in commit 3ec6a5c547 on Apr 9, 2020

Fabcien referenced this in commit bb22523cfa on Jan 6, 2021

DrahtBot locked this on Feb 15, 2022

rpc/wallet: initialize nFeeRequired to avoid using garbage value on failure #18274

Conflicts