wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...) #13546

practicalswift · 2018-06-27T16:12:13Z

Avoid use of uninitialized value bnb_used in CWallet::CreateTransaction(...).

maflcko · 2018-06-27T16:22:31Z

This is a return value, so it is never uninitialized.

practicalswift · 2018-06-27T16:26:38Z

@MarcoFalke In the case of !pick_new_inputs && nValueIn - nValueToSelect > 0 && !IsDust(newTxOut, discard_rate) then an uninitialized bnb_used is read?

promag · 2018-06-27T16:26:57Z

src/wallet/wallet.cpp

@@ -2888,7 +2888,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
                }

                // Choose coins to use
-                bool bnb_used;


If pick_new_inputs == false then below bnb_used can be used right? Other output vars (nValueIn and setCoins) are also initialized here.

I think the correct fix is to move this out of the loop scope, so it doesn't loose the value since the last coin selection iteration. So move here:

bitcoin/src/wallet/wallet.cpp

Lines 2835 to 2843 in fad42e8

nFeeRet = 0;

bool pick_new_inputs = true;

CAmount nValueIn = 0;

// BnB selector is the only selector used when this is true.

// That should only happen on the first pass through the loop.

coin_selection_params.use_bnb = nSubtractFeeFromAmount == 0; // If we are doing subtract fee from recipient, then don't use BnB

// Start with no fee and loop until there is enough fee

while (true)

Because there are 2 cases that skip "pick new inputs":

bitcoin/src/wallet/wallet.cpp

Lines 2987 to 2991 in fad42e8

if (nFeeRet >= fee_needed_with_change + minimum_value_for_change) {

pick_new_inputs = false;

nFeeRet = fee_needed_with_change;

continue;

}

bitcoin/src/wallet/wallet.cpp

Lines 3024 to 3028 in fad42e8

// If subtracting fee from recipients, we now know what fee we

// need to subtract, we have no reason to reselect inputs

if (nSubtractFeeFromAmount > 0) {

pick_new_inputs = false;

}

And when that happens use_bnb can be used uninitialized.

DrahtBot · 2018-06-28T09:30:13Z

No more conflicts as of last run.

promag · 2018-06-28T10:11:46Z

FYI also not initialized in

bitcoin/src/bench/coin_selection.cpp

Line 48 in d96bdd7

bool bnb_used;

But there it should be ok.

practicalswift · 2018-06-29T06:05:01Z

@MarcoFalke Is my reasoning correct or do your comment still stand? :-)

practicalswift · 2018-06-29T06:08:40Z

@promag Thanks for reviewing. Anything changes needed to get an utACK or Concept ACK from you? :-)

maflcko · 2018-06-29T09:07:58Z

We really should have a test case that exercises the logic if !pick_new_inputs. Otherwise it is hard to reason about the correctness of this patch.

practicalswift · 2018-06-29T09:26:01Z

@MarcoFalke I was asking about the statement "it is never uninitialized". That was a misunderstanding?

maflcko · 2018-06-29T09:33:31Z

Whenever it is returned, it is initialized. However, if it is not returned, we shouldn't be reading from it, so I was asking about a test case that exercises that exact code path.

DrahtBot · 2018-08-25T20:55:09Z

The last travis run for this pull request was 59 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

practicalswift · 2018-09-18T09:31:05Z

@MarcoFalke I originally found this issue by using static analysis but I rediscovered it using dynamic analysis as well. It turns out that this is triggered simply running the test suite under UBSan :-)

wallet/wallet.cpp:2757:59: runtime error: load of value 112, which is not a valid value for type 'bool' !=

See https://travis-ci.org/bitcoin/bitcoin/jobs/429944903#L3960

maflcko · 2018-09-19T00:03:28Z

src/wallet/wallet.cpp

@@ -2826,7 +2826,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
                }

                // Choose coins to use
-                bool bnb_used;
+                bool bnb_used = false;


Imo this shouldn't be initialized to false. This just silences the bug without fixing it. Imo it should be set to false only when !pick_new_inputs?

…:CreateTransaction(...)

practicalswift · 2018-09-19T12:12:01Z

@MarcoFalke Thanks for reviewing! Addressed feedback. Please re-review :-)

maflcko · 2018-09-19T12:14:52Z

utACK a23a7f6

maflcko · 2018-09-19T13:09:47Z

Interesting, I couldn't reproduce this locally with ./configure --with-sanitizers=bool CC=clang CXX=clang++ && make -j 16 src/bitcoind && ./test/functional/rpc_fundrawtransaction.py. Could someone else try this?

achow101

utACK a23a7f6

practicalswift · 2018-09-24T09:07:01Z

Removed "potential" in the title since this use of uninitialized value has been observed also during run-time :-)

…let::CreateTransaction(...) a23a7f6 wallet: Avoid potential use of unitialized value bnb_used in CWallet::CreateTransaction(...) (practicalswift) Pull request description: Avoid use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)`. Tree-SHA512: 22faf0711ae35af44d9a0ab7f251bc01661ac88b40ad7b0a87a510427b46bbc8caf16868cab2e0a05e7d8518e93ce666d6bd1d48d3707d37bab2c0fb56a0a4a2

…defined behaviour sanitizer (UBSan) 9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift) Pull request description: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan). This will make Travis automatically detect issues such as: * #14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)` * #14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet) * #13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)` Addresses issue #14059. Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4

…:CreateTransaction(...) Github-Pull: bitcoin#13546 Rebased-From: a23a7f6

…Wallet::CreateTransaction(...) Summary: wallet: Avoid potential use of unitialized value bnb_used in CWallet::CreateTransaction(...) (practicalswift) Pull request description: Avoid use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)`. bitcoin/bitcoin@a23a7f6 --- Backport of Core [[bitcoin/bitcoin#13546 | PR13546]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7060

…in CWallet::CreateTransaction(...) a23a7f6 wallet: Avoid potential use of unitialized value bnb_used in CWallet::CreateTransaction(...) (practicalswift) Pull request description: Avoid use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)`. Tree-SHA512: 22faf0711ae35af44d9a0ab7f251bc01661ac88b40ad7b0a87a510427b46bbc8caf16868cab2e0a05e7d8518e93ce666d6bd1d48d3707d37bab2c0fb56a0a4a2 # Conflicts: # src/wallet/wallet.cpp

… in CWallet::CreateTransaction(...) a23a7f6 wallet: Avoid potential use of unitialized value bnb_used in CWallet::CreateTransaction(...) (practicalswift) Pull request description: Avoid use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)`. Tree-SHA512: 22faf0711ae35af44d9a0ab7f251bc01661ac88b40ad7b0a87a510427b46bbc8caf16868cab2e0a05e7d8518e93ce666d6bd1d48d3707d37bab2c0fb56a0a4a2

… the undefined behaviour sanitizer (UBSan) 9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift) Pull request description: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan). This will make Travis automatically detect issues such as: * bitcoin#14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)` * bitcoin#14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet) * bitcoin#13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)` Addresses issue bitcoin#14059. Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4

… the undefined behaviour sanitizer (UBSan) 9f49db7 Enable functional tests in UBSAN job. Enable -fsanitize=integer (part of UBSAN). Merge UBSAN Travis job with no depends. (practicalswift) Pull request description: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan). This will make Travis automatically detect issues such as: * bitcoin#14242: Avoid triggering undefined behaviour (`std::memset(nullptr, 0, 0)`) if an invalid string is passed to `DecodeSecret(...)` * bitcoin#14239: Avoid dividing by zero (undefined behaviour) in `EstimateMedianVal` (policy)/`ConnectTip` (validation)/`CreateTransaction` (wallet) * bitcoin#13546: wallet: Avoid potential use of uninitialized value `bnb_used` in `CWallet::CreateTransaction(...)` Addresses issue bitcoin#14059. Tree-SHA512: 285e1542b36c582516c47938ce8d999fd89ba6c867bc0976e7306e7c949b8b84ffbfa43dbc679dd97ae639b086092e7d799d8e1c903c66a37d529ce61d5c64b4 continued 14252 Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

fanquake added the Wallet label Jun 27, 2018

promag reviewed Jun 27, 2018

View reviewed changes

DrahtBot closed this Aug 25, 2018

DrahtBot reopened this Aug 25, 2018

practicalswift force-pushed the uninitialized-bnb_used branch from ce81385 to 126561a Compare August 25, 2018 21:06

practicalswift mentioned this pull request Aug 31, 2018

wallet: Remove unused local variable old_label #14117

Closed

DrahtBot mentioned this pull request Sep 7, 2018

Replace coin selection fallback strategy with Single Random Draw #13307

Closed

practicalswift mentioned this pull request Sep 18, 2018

build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) #14252

Merged

maflcko reviewed Sep 19, 2018

View reviewed changes

maflcko added this to the 0.18.0 milestone Sep 19, 2018

wallet: Avoid potential use of unitialized value bnb_used in CWallet:…

a23a7f6

…:CreateTransaction(...)

practicalswift force-pushed the uninitialized-bnb_used branch from 126561a to a23a7f6 Compare September 19, 2018 12:11

maflcko requested a review from achow101 September 19, 2018 12:15

achow101 approved these changes Sep 20, 2018

View reviewed changes

practicalswift mentioned this pull request Sep 20, 2018

functional tests fail --with-sanitizers=undefined #14059

Closed

practicalswift changed the title ~~wallet: Avoid potential use of uninitialized value bnb_used in CWallet::CreateTransaction(...)~~ wallet: Avoid use of uninitialized value bnb_used in CWallet::CreateTransaction(...) Sep 24, 2018

practicalswift changed the title ~~wallet: Avoid use of uninitialized value bnb_used in CWallet::CreateTransaction(...)~~ wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...) Sep 24, 2018

maflcko removed this from the 0.18.0 milestone Sep 24, 2018

maflcko merged commit a23a7f6 into bitcoin:master Sep 24, 2018

maflcko added this to the 0.17.1 milestone Sep 24, 2018

maflcko added Needs backport and removed Needs backport labels Sep 24, 2018

Bushstar mentioned this pull request Oct 11, 2018

Updates from bitcoin/master FeatherCoin/Feathercoin#366

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 28, 2018

wallet: Avoid potential use of unitialized value bnb_used in CWallet:…

91fa15a

…:CreateTransaction(...) Github-Pull: bitcoin#13546 Rebased-From: a23a7f6

practicalswift mentioned this pull request Oct 22, 2019

tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. #17209

Merged

Sjors mentioned this pull request Nov 25, 2019

wallet: fix when sufficient preset inputs and subtractFeeFromOutputs #17568

Merged

practicalswift mentioned this pull request Mar 10, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

practicalswift mentioned this pull request Oct 1, 2020

signet: Fix uninitialized read in validation #20035

Merged

practicalswift mentioned this pull request Nov 11, 2020

ci: Remove redundant valgrind fuzz task #20368

Merged

practicalswift deleted the uninitialized-bnb_used branch April 10, 2021 19:36

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...) #13546

wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...) #13546

practicalswift commented Jun 27, 2018 •

edited

Loading

maflcko commented Jun 27, 2018

practicalswift commented Jun 27, 2018 •

edited

Loading

promag Jun 27, 2018

promag Jul 11, 2018 •

edited

Loading

DrahtBot commented Jun 28, 2018 •

edited

Loading

promag commented Jun 28, 2018

practicalswift commented Jun 29, 2018

practicalswift commented Jun 29, 2018 •

edited

Loading

maflcko commented Jun 29, 2018

practicalswift commented Jun 29, 2018 •

edited

Loading

maflcko commented Jun 29, 2018

DrahtBot commented Aug 25, 2018

practicalswift commented Sep 18, 2018 •

edited

Loading

maflcko Sep 19, 2018 •

edited

Loading

practicalswift commented Sep 19, 2018

maflcko commented Sep 19, 2018

maflcko commented Sep 19, 2018

achow101 left a comment

practicalswift commented Sep 24, 2018

	nFeeRet = 0;
	bool pick_new_inputs = true;
	CAmount nValueIn = 0;

	// BnB selector is the only selector used when this is true.
	// That should only happen on the first pass through the loop.
	coin_selection_params.use_bnb = nSubtractFeeFromAmount == 0; // If we are doing subtract fee from recipient, then don't use BnB
	// Start with no fee and loop until there is enough fee
	while (true)

	if (nFeeRet >= fee_needed_with_change + minimum_value_for_change) {
	pick_new_inputs = false;
	nFeeRet = fee_needed_with_change;
	continue;
	}

	// If subtracting fee from recipients, we now know what fee we
	// need to subtract, we have no reason to reselect inputs
	if (nSubtractFeeFromAmount > 0) {
	pick_new_inputs = false;
	}

wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...) #13546

wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...) #13546

Conversation

practicalswift commented Jun 27, 2018 • edited Loading

maflcko commented Jun 27, 2018

practicalswift commented Jun 27, 2018 • edited Loading

promag Jun 27, 2018

Choose a reason for hiding this comment

promag Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

DrahtBot commented Jun 28, 2018 • edited Loading

promag commented Jun 28, 2018

practicalswift commented Jun 29, 2018

practicalswift commented Jun 29, 2018 • edited Loading

maflcko commented Jun 29, 2018

practicalswift commented Jun 29, 2018 • edited Loading

maflcko commented Jun 29, 2018

DrahtBot commented Aug 25, 2018

practicalswift commented Sep 18, 2018 • edited Loading

maflcko Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

practicalswift commented Sep 19, 2018

maflcko commented Sep 19, 2018

maflcko commented Sep 19, 2018

achow101 left a comment

Choose a reason for hiding this comment

practicalswift commented Sep 24, 2018

practicalswift commented Jun 27, 2018 •

edited

Loading

practicalswift commented Jun 27, 2018 •

edited

Loading

promag Jul 11, 2018 •

edited

Loading

DrahtBot commented Jun 28, 2018 •

edited

Loading

practicalswift commented Jun 29, 2018 •

edited

Loading

practicalswift commented Jun 29, 2018 •

edited

Loading

practicalswift commented Sep 18, 2018 •

edited

Loading

maflcko Sep 19, 2018 •

edited

Loading