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
  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 0: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: member
  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: member
  15. DrahtBot reopened this on Aug 25, 2018
  16. practicalswift force-pushed on Aug 25, 2018
  17. 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 :-)

    0wallet/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

  18. 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 0: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?
  19. MarcoFalke added this to the milestone 0.18.0 on Sep 19, 2018
  20. wallet: Avoid potential use of unitialized value bnb_used in CWallet::CreateTransaction(...) a23a7f60aa
  21. practicalswift force-pushed on Sep 19, 2018
  22. practicalswift commented at 12:12 pm on September 19, 2018: contributor
    @MarcoFalke Thanks for reviewing! Addressed feedback. Please re-review :-)
  23. MarcoFalke commented at 12:14 pm on September 19, 2018: member
    utACK a23a7f60aa07de52d23ff1f2034fc43926ec3520
  24. MarcoFalke requested review from achow101 on Sep 19, 2018
  25. 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?
  26. achow101 approved
  27. achow101 commented at 3:11 am on September 20, 2018: member
    utACK a23a7f60aa07de52d23ff1f2034fc43926ec3520
  28. 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
  29. 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 :-)
  30. 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
  31. MarcoFalke removed this from the milestone 0.18.0 on Sep 24, 2018
  32. MarcoFalke merged this on Sep 24, 2018
  33. MarcoFalke closed this on Sep 24, 2018
  34. MarcoFalke referenced this in commit e798ae41e0 on Sep 24, 2018
  35. MarcoFalke added this to the milestone 0.17.1 on Sep 24, 2018
  36. MarcoFalke added the label Needs backport on Sep 24, 2018
  37. MarcoFalke removed the label Needs backport on Sep 26, 2018
  38. MarcoFalke referenced this in commit a42f62bbc8 on Sep 26, 2018
  39. MarcoFalke referenced this in commit 98b5dcaa00 on Oct 28, 2018
  40. MarcoFalke referenced this in commit 1ba5583646 on Nov 5, 2018
  41. MarcoFalke referenced this in commit 784fcd49e6 on Nov 19, 2018
  42. MarcoFalke referenced this in commit 91fa15aaeb on Nov 28, 2018
  43. jasonbcox referenced this in commit a78b85c81e on Jul 28, 2020
  44. practicalswift deleted the branch on Apr 10, 2021
  45. Munkybooty referenced this in commit 44f4cd1a66 on Jul 9, 2021
  46. pravblockc referenced this in commit ab8e835289 on Jul 22, 2021
  47. pravblockc referenced this in commit 1606ddea7d on Jul 23, 2021
  48. PastaPastaPasta referenced this in commit 1197a1f392 on Aug 16, 2021
  49. PastaPastaPasta referenced this in commit 857fae9277 on Aug 16, 2021
  50. PastaPastaPasta referenced this in commit 009f0059b9 on Aug 17, 2021
  51. PastaPastaPasta referenced this in commit b98e643250 on Aug 18, 2021
  52. gades referenced this in commit 7c4d74079c on Apr 20, 2022
  53. DrahtBot locked this on Aug 18, 2022


practicalswift MarcoFalke promag DrahtBot achow101

Labels
Wallet

Milestone
0.17.1

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me