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

MarcoFalke commented at 6:09 pm on June 11, 2018: member

This prevents segfaults, when reading from the freed memory.
wallet: Erase wtxOrderd wtx pointer on removeprunedfunds faa18ca046
MarcoFalke added the label Wallet on Jun 11, 2018
MarcoFalke added the label RPC/REST/ZMQ on Jun 11, 2018
practicalswift commented at 6:32 pm on June 11, 2018: contributor

Oh, nice find! How was this issue found?
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.

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

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

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

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:

0for (const auto& it_wtxOrdered = wtxOrdered.lower_bound(it->second.nOrderPos; ...) {
1    if (&it_wtxOrdered->second.first == &it->second) {
2    }
3}

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.
promag commented at 0:57 am on June 14, 2018: member

Should backport?
DrahtBot commented at 8:20 pm on June 14, 2018: member
- #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.
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.
laanwj merged this on Jun 18, 2018
laanwj closed this on Jun 18, 2018
laanwj added this to the milestone 0.16.2 on Jun 18, 2018
laanwj referenced this in commit 0882406854 on Jun 18, 2018
laanwj added the label Needs backport on Jun 18, 2018
jnewbery commented at 8:47 pm on June 18, 2018: member

tested ACK faa18ca046e9043b2cf68cb1bd17cc8c60fe26d9
MarcoFalke deleted the branch on Jul 11, 2018
MarcoFalke referenced this in commit 32fce4e3a3 on Jul 13, 2018
MarcoFalke referenced this in commit ed82e7176d on Jul 13, 2018
fanquake removed the label Needs backport on Jul 13, 2018
fanquake commented at 11:57 pm on July 13, 2018: member

Should be backported in #13644.
HashUnlimited referenced this in commit aa9236b7ad on Jan 11, 2019
jasonbcox referenced this in commit 558347b3d9 on Dec 6, 2019
PastaPastaPasta referenced this in commit e39605d284 on Apr 12, 2020
PastaPastaPasta referenced this in commit 23704daaf0 on Apr 16, 2020
jonspock referenced this in commit fe471acffe on Sep 29, 2020
jonspock referenced this in commit 9e300d0098 on Sep 29, 2020
jonspock referenced this in commit ef88abd391 on Oct 10, 2020
ckti referenced this in commit 5598c38af9 on Mar 28, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors
MarcoFalke practicalswift promag Empact jonasschnelli DrahtBot laanwj jnewbery fanquake

Labels
Wallet RPC/REST/ZMQ

Milestone
0.16.2