wallet: Erase wtxOrderd wtx pointer on removeprunedfunds #13437

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1806-walletPrunedFundsSegfault changing 2 files +13 −7
  1. MarcoFalke commented at 6:09 PM on June 11, 2018: member

    This prevents segfaults, when reading from the freed memory.

  2. wallet: Erase wtxOrderd wtx pointer on removeprunedfunds faa18ca046
  3. MarcoFalke added the label Wallet on Jun 11, 2018
  4. MarcoFalke added the label RPC/REST/ZMQ on Jun 11, 2018
  5. practicalswift commented at 6:32 PM on June 11, 2018: contributor

    Oh, nice find! How was this issue found?

  6. MarcoFalke commented at 6:55 PM on June 11, 2018: member

    @practicalswift

    • Importprunedfunds
    • removeprunedfunds
    • listtransactions -> invalid read

    See also (related?) issue #9729

    I can reproduce on 0.15 and master, haven't checked 0.16.

  7. in src/wallet/wallet.cpp:3211 in faa18ca046 
    3207 | @@ -3206,8 +3208,11 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
3208 |  {
3209 |      AssertLockHeld(cs_wallet); // mapWallet
3210 |      DBErrors nZapSelectTxRet = WalletBatch(*database,"cr+").ZapSelectTx(vHashIn, vHashOut);
3211 | -    for (uint256 hash : vHashOut)
3212 | -        mapWallet.erase(hash);
3213 | +    for (uint256 hash : vHashOut) {
    promag commented at 7:15 AM on June 12, 2018:

    nit, could change to const uint256&.

  8. in src/wallet/wallet.cpp:1000 in faa18ca046 
     996 | @@ -998,9 +997,12 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose)
 997 |  bool CWallet::LoadToWallet(const CWalletTx& wtxIn)
 998 |  {
 999 |      uint256 hash = wtxIn.GetHash();
1000 | -    CWalletTx& wtx = mapWallet.emplace(hash, wtxIn).first->second;
1001 | +    const auto& ins = mapWallet.emplace(hash, wtxIn);
    Empact commented at 8:48 AM on June 12, 2018:

    May be a good place to use std::tie for more self-documenting code. http://en.cppreference.com/w/cpp/utility/tuple/tie

  9. jonasschnelli commented at 9:04 AM on June 12, 2018: contributor

    @MarcoFalke: can you elaborate a bit more and/or add some source code documentations. A class self-referencing multimap in CWalletTx seems not ideal and I wonder if there are other ways to prevent the invalid access.

  10. MarcoFalke commented at 12:14 PM on June 12, 2018: member

    If someone finds a more elegant solution to erase from both (mapWallet and wtxOrdered) in ZapSelectTx, which is the only behaviour change in this pull, I am all for it.

  11. in src/wallet/wallet.h:342 in faa18ca046 
     338 | @@ -339,6 +339,7 @@ class CWalletTx : public CMerkleTx
 339 |      char fFromMe;
 340 |      std::string strFromAccount;
 341 |      int64_t nOrderPos; //!< position in ordered transaction list
 342 | +    std::multimap<int64_t, std::pair<CWalletTx*, CAccountingEntry*>>::const_iterator m_it_wtxOrdered;
    promag commented at 3:19 PM on June 12, 2018:

    Remove, nOrderPos should be enough for fast lookup in wtxOrdered.

    MarcoFalke commented at 4:06 PM on June 12, 2018:

    @promag IIRC, nOrderPos is updated while the cache keeps the old value...

    If I do this, I'd have to open a new pull request, since this is a different fix.

    promag commented at 5:39 PM on June 12, 2018:

    Either way storing a map/multimap iterator is safe (while the entry exists) and this is the most performant solution AFAIK.

  12. in src/wallet/wallet.cpp:3213 in faa18ca046 
    3207 | @@ -3206,8 +3208,11 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
3208 |  {
3209 |      AssertLockHeld(cs_wallet); // mapWallet
3210 |      DBErrors nZapSelectTxRet = WalletBatch(*database,"cr+").ZapSelectTx(vHashIn, vHashOut);
3211 | -    for (uint256 hash : vHashOut)
3212 | -        mapWallet.erase(hash);
3213 | +    for (uint256 hash : vHashOut) {
3214 | +        const auto& it = mapWallet.find(hash);
3215 | +        wtxOrdered.erase(it->second.m_it_wtxOrdered);
    promag commented at 3:21 PM on June 12, 2018:

    Something like:

    for (const auto& it_wtxOrdered = wtxOrdered.lower_bound(it->second.nOrderPos; ...) {
    if (&it_wtxOrdered->second.first == &it->second) {
    }
}
  13. promag commented at 3:27 PM on June 12, 2018: member

    Concept ACK. Agree on @jonasschnelli, feels weird but not entirely bad. Alternative suggestion below.

  14. promag commented at 12:57 AM on June 14, 2018: member

    Should backport?

  15. DrahtBot commented at 8:20 PM on June 14, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->Note to reviewers: This pull request conflicts with the following ones:

    • #13249 (Make objects in range declarations immutable by default by practicalswift)
    • #10973 (Refactor: separate wallet from node by ryanofsky)

    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.

  16. laanwj commented at 3:33 PM on June 18, 2018: member

    utACK faa18ca046e9043b2cf68cb1bd17cc8c60fe26d9

    If someone finds a more elegant solution to erase from both (mapWallet and wtxOrdered) in ZapSelectTx, which is the only behaviour change in this pull, I am all for it.

    I kind of like this solution of storing the iterator to be able to remove it later.

  17. laanwj cross-referenced this on Jun 18, 2018 from issue too many removeprunedfunds and listtransactions , bitcoind will be crash by JokerCatz
  18. laanwj merged this on Jun 18, 2018
  19. laanwj closed this on Jun 18, 2018
  20. laanwj added this to the milestone 0.16.2 on Jun 18, 2018
  21. laanwj referenced this in commit 0882406854 on Jun 18, 2018
  22. laanwj added the label Needs backport on Jun 18, 2018
  23. jnewbery commented at 8:47 PM on June 18, 2018: member

    tested ACK faa18ca046e9043b2cf68cb1bd17cc8c60fe26d9

  24. ryanofsky cross-referenced this on Jun 19, 2018 from issue Remove CWalletTx merging logic from AddToWallet by ryanofsky
  25. Bushstar cross-referenced this on Jun 20, 2018 from issue commits from bitcoin/master by Bushstar
  26. ryanofsky cross-referenced this on Jun 26, 2018 from issue Refactor: separate wallet from node by ryanofsky
  27. MarcoFalke deleted the branch on Jul 11, 2018
  28. fanquake cross-referenced this on Jul 12, 2018 from issue 0.16: Remaining backports for 0.16.2 by MarcoFalke
  29. MarcoFalke referenced this in commit 32fce4e3a3 on Jul 13, 2018
  30. MarcoFalke referenced this in commit ed82e7176d on Jul 13, 2018
  31. fanquake removed the label Needs backport on Jul 13, 2018
  32. fanquake commented at 11:57 PM on July 13, 2018: member

    Should be backported in #13644.

  33. HashUnlimited referenced this in commit aa9236b7ad on Jan 11, 2019
  34. jasonbcox referenced this in commit 558347b3d9 on Dec 6, 2019
  35. PastaPastaPasta referenced this in commit e39605d284 on Apr 12, 2020
  36. PastaPastaPasta referenced this in commit 23704daaf0 on Apr 16, 2020
  37. jonspock referenced this in commit fe471acffe on Sep 29, 2020
  38. jonspock referenced this in commit 9e300d0098 on Sep 29, 2020
  39. jonspock referenced this in commit ef88abd391 on Oct 10, 2020
  40. ckti referenced this in commit 5598c38af9 on Mar 28, 2021
  41. bitcoin locked this on Sep 8, 2021
Contributors
MarcoFalkepracticalswiftpromagEmpactjonasschnelliDrahtBotlaanwjjnewberyfanquake
Labels
WalletRPC/REST/ZMQ

Milestone
0.16.2

Linked (view graph)
#9381 Remove CWalletTx merging logic from AddToWallet#9729 too many removeprunedfunds and listtransactions , bitcoind will be crash#10973 Refactor: separate wallet from node#13644 0.16: Remaining backports for 0.16.2
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: 2026-05-20 06:55 UTC