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

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:uninitialized-bnb_used changing 1 files +2 −0

practicalswift commented at 4:12 PM on June 27, 2018: contributor

Avoid use of uninitialized value bnb_used in CWallet::CreateTransaction(...).
fanquake added the label Wallet on Jun 27, 2018
MarcoFalke commented at 4:22 PM on June 27, 2018: member

This is a return value, so it is never uninitialized.
practicalswift commented at 4:26 PM on June 27, 2018: contributor

@MarcoFalke In the case of !pick_new_inputs && nValueIn - nValueToSelect > 0 && !IsDust(newTxOut, discard_rate) then an uninitialized bnb_used is read?
in src/wallet/wallet.cpp:2829 in ce81385cd6 outdated
```
2887 | @@ -2888,7 +2888,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
2888 |                  }
2889 |  
2890 |                  // Choose coins to use
2891 | -                bool bnb_used;
```
promag commented at 4:26 PM on June 27, 2018:

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

promag commented at 12:41 AM on July 11, 2018:

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: https://github.com/bitcoin/bitcoin/blob/fad42e8c4a9d625146f82bab9a038d945d40ac4f/src/wallet/wallet.cpp#L2835-L2843 Because there are 2 cases that skip "pick new inputs": https://github.com/bitcoin/bitcoin/blob/fad42e8c4a9d625146f82bab9a038d945d40ac4f/src/wallet/wallet.cpp#L2987-L2991 https://github.com/bitcoin/bitcoin/blob/fad42e8c4a9d625146f82bab9a038d945d40ac4f/src/wallet/wallet.cpp#L3024-L3028 And when that happens use_bnb can be used uninitialized.
DrahtBot commented at 9:30 AM on June 28, 2018: member

No more conflicts as of last run.
promag commented at 10:11 AM on June 28, 2018: member

FYI also not initialized in https://github.com/bitcoin/bitcoin/blob/d96bdd78307bc5469cb8a4d5ca0e6cbc21fe4073/src/bench/coin_selection.cpp#L48 But there it should be ok.
practicalswift commented at 6:05 AM on June 29, 2018: contributor

@MarcoFalke Is my reasoning correct or do your comment still stand? :-)
practicalswift commented at 6:08 AM on June 29, 2018: contributor

@promag Thanks for reviewing. Anything changes needed to get an utACK or Concept ACK from you? :-)
MarcoFalke commented at 9:07 AM on June 29, 2018: member

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 commented at 9:26 AM on June 29, 2018: contributor

@MarcoFalke I was asking about the statement "it is never uninitialized". That was a misunderstanding?
MarcoFalke commented at 9:33 AM on June 29, 2018: member

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 closed this on Aug 25, 2018
DrahtBot commented at 8:55 PM on August 25, 2018: member

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.
DrahtBot reopened this on Aug 25, 2018
practicalswift force-pushed on Aug 25, 2018
practicalswift commented at 9:31 AM on September 18, 2018: contributor
@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

in src/wallet/wallet.cpp:2829 in 126561a171 outdated

2825 | @@ -2826,7 +2826,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
2826 |                  }
2827 |  
2828 |                  // Choose coins to use
2829 | -                bool bnb_used;
2830 | +                bool bnb_used = false;

MarcoFalke commented at 12:03 AM on September 19, 2018:

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?

MarcoFalke added this to the milestone 0.18.0 on Sep 19, 2018
wallet: Avoid potential use of unitialized value bnb_used in CWallet::CreateTransaction(...) a23a7f60aa
practicalswift force-pushed on Sep 19, 2018
practicalswift commented at 12:12 PM on September 19, 2018: contributor

@MarcoFalke Thanks for reviewing! Addressed feedback. Please re-review :-)
MarcoFalke commented at 12:14 PM on September 19, 2018: member

utACK a23a7f60aa07de52d23ff1f2034fc43926ec3520
MarcoFalke requested review from achow101 on Sep 19, 2018
MarcoFalke commented at 1:09 PM on September 19, 2018: member

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 approved
achow101 commented at 3:11 AM on September 20, 2018: member

utACK a23a7f60aa07de52d23ff1f2034fc43926ec3520
practicalswift renamed this:
~~wallet: Avoid potential use of uninitialized value bnb_used in CWallet::CreateTransaction(...)~~
wallet: Avoid use of uninitialized value bnb_used in CWallet::CreateTransaction(...)
on Sep 24, 2018
practicalswift commented at 9:07 AM on September 24, 2018: contributor

Removed "potential" in the title since this use of uninitialized value has been observed also during run-time :-)
practicalswift renamed this:
~~wallet: Avoid use of uninitialized value bnb_used in CWallet::CreateTransaction(...)~~
wallet: Fix use of uninitialized value bnb_used in CWallet::CreateTransaction(...)
on Sep 24, 2018
MarcoFalke removed this from the milestone 0.18.0 on Sep 24, 2018
MarcoFalke merged this on Sep 24, 2018
MarcoFalke closed this on Sep 24, 2018
MarcoFalke referenced this in commit e798ae41e0 on Sep 24, 2018
MarcoFalke added this to the milestone 0.17.1 on Sep 24, 2018
MarcoFalke added the label Needs backport on Sep 24, 2018
MarcoFalke removed the label Needs backport on Sep 26, 2018
MarcoFalke referenced this in commit a42f62bbc8 on Sep 26, 2018
MarcoFalke referenced this in commit 98b5dcaa00 on Oct 28, 2018
MarcoFalke referenced this in commit 1ba5583646 on Nov 5, 2018
MarcoFalke referenced this in commit 784fcd49e6 on Nov 19, 2018
MarcoFalke referenced this in commit 91fa15aaeb on Nov 28, 2018
jasonbcox referenced this in commit a78b85c81e on Jul 28, 2020
practicalswift deleted the branch on Apr 10, 2021
Munkybooty referenced this in commit 44f4cd1a66 on Jul 9, 2021
pravblockc referenced this in commit ab8e835289 on Jul 22, 2021
pravblockc referenced this in commit 1606ddea7d on Jul 23, 2021
PastaPastaPasta referenced this in commit 1197a1f392 on Aug 16, 2021
PastaPastaPasta referenced this in commit 857fae9277 on Aug 16, 2021
PastaPastaPasta referenced this in commit 009f0059b9 on Aug 17, 2021
PastaPastaPasta referenced this in commit b98e643250 on Aug 18, 2021
gades referenced this in commit 7c4d74079c on Apr 20, 2022
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Milestone
0.17.1

Linked (view graph)

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