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.

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?

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

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.

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.

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.

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.

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.

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.

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

Pushed removal of duplicate nFeeRet = 0; statement.

@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).

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• refactor: consolidate sendmany and sendtoaddress code  #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.

@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:

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

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

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

@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.

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

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

ACK a652ba6 -- patch looks correct

utACK a652ba6

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.