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

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

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

2. fanquake added the label Wallet on Jun 27, 2018
3. 
   MarcoFalke commented at 4:22 PM on June 27, 2018: member

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

4. 
   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?

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

6. 
   DrahtBot commented at 9:30 AM on June 28, 2018: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->No more conflicts as of last run.

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

8. 
   practicalswift commented at 6:05 AM on June 29, 2018: contributor

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

9. 
   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? :-)

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

11. 
    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?

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

13. DrahtBot closed this on Aug 25, 2018

    ---

14. 
    DrahtBot commented at 8:55 PM on August 25, 2018: contributor

    <!--5d09a71f8925f3f132321140b44b946d-->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.

15. DrahtBot reopened this on Aug 25, 2018

    ---

16. practicalswift force-pushed on Aug 25, 2018
17. practicalswift cross-referenced this on Aug 31, 2018 from issue wallet: Remove unused local variable old_label by practicalswift
18. DrahtBot cross-referenced this on Sep 7, 2018 from issue Replace coin selection fallback strategy with Single Random Draw by achow101
19. 
    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

20. practicalswift cross-referenced this on Sep 18, 2018 from issue build: Run functional tests and benchmarks under the undefined behaviour sanitizer (UBSan) by practicalswift
21. 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?

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

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

26. 
    MarcoFalke commented at 12:14 PM on September 19, 2018: member

    utACK a23a7f60aa07de52d23ff1f2034fc43926ec3520

27. MarcoFalke requested review from achow101 on Sep 19, 2018
28. 
    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?

29. achow101 approved
30. 
    achow101 commented at 3:11 AM on September 20, 2018: member

    utACK a23a7f60aa07de52d23ff1f2034fc43926ec3520

31. practicalswift cross-referenced this on Sep 20, 2018 from issue functional tests fail --with-sanitizers=undefined by MarcoFalke
32. 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
33. 
    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 :-)

34. 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
35. MarcoFalke removed this from the milestone 0.18.0 on Sep 24, 2018
36. MarcoFalke merged this on Sep 24, 2018
37. MarcoFalke closed this on Sep 24, 2018

    ---

38. MarcoFalke referenced this in commit e798ae41e0 on Sep 24, 2018
39. MarcoFalke added this to the milestone 0.17.1 on Sep 24, 2018
40. MarcoFalke added the label Needs backport on Sep 24, 2018
41. MarcoFalke removed the label Needs backport on Sep 26, 2018
42. MarcoFalke referenced this in commit a42f62bbc8 on Sep 26, 2018
43. Bushstar cross-referenced this on Oct 11, 2018 from issue Updates from bitcoin/master by Bushstar
44. MarcoFalke referenced this in commit 98b5dcaa00 on Oct 28, 2018
45. MarcoFalke referenced this in commit 1ba5583646 on Nov 5, 2018
46. MarcoFalke referenced this in commit 784fcd49e6 on Nov 19, 2018
47. MarcoFalke referenced this in commit 91fa15aaeb on Nov 28, 2018
48. practicalswift cross-referenced this on Oct 22, 2019 from issue tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. by practicalswift
49. Sjors cross-referenced this on Nov 25, 2019 from issue wallet: fix when sufficient preset inputs and subtractFeeFromOutputs by achow101
50. practicalswift cross-referenced this on Mar 10, 2020 from issue build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory by practicalswift
51. jasonbcox referenced this in commit a78b85c81e on Jul 28, 2020
52. practicalswift cross-referenced this on Oct 1, 2020 from issue signet: Fix uninitialized read in validation by MarcoFalke
53. practicalswift cross-referenced this on Nov 11, 2020 from issue ci: Remove redundant valgrind fuzz task by MarcoFalke
54. practicalswift deleted the branch on Apr 10, 2021
55. Munkybooty referenced this in commit 44f4cd1a66 on Jul 9, 2021
56. pravblockc referenced this in commit ab8e835289 on Jul 22, 2021
57. pravblockc referenced this in commit 1606ddea7d on Jul 23, 2021
58. PastaPastaPasta referenced this in commit 1197a1f392 on Aug 16, 2021
59. PastaPastaPasta referenced this in commit 857fae9277 on Aug 16, 2021
60. PastaPastaPasta referenced this in commit 009f0059b9 on Aug 17, 2021
61. PastaPastaPasta referenced this in commit b98e643250 on Aug 18, 2021
62. gades referenced this in commit 7c4d74079c on Apr 20, 2022
63. bitcoin locked this on Aug 18, 2022

Contributors

practicalswiftMarcoFalkepromagDrahtBotachow101

Milestone
0.17.1