wallet: Erase wtxOrderd wtx pointer on removeprunedfunds by maflcko · Pull Request #13437 · bitcoin/bitcoin

maflcko · 2018-06-11T18:09:49Z

This prevents segfaults, when reading from the freed memory.

practicalswift · 2018-06-11T18:32:38Z

Oh, nice find! How was this issue found?

maflcko · 2018-06-11T18:55:47Z

@practicalswift

Importprunedfunds
removeprunedfunds
listtransactions -> invalid read

See also (related?) issue #9729

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

Empact · 2018-06-12T08:48:15Z

 {
    uint256 hash = wtxIn.GetHash();
-    CWalletTx& wtx = mapWallet.emplace(hash, wtxIn).first->second;
+    const auto& ins = mapWallet.emplace(hash, wtxIn);


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

jonasschnelli · 2018-06-12T09:04:25Z

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

maflcko · 2018-06-12T12:14:49Z

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.

promag

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

promag · 2018-06-12T07:15:24Z

    DBErrors nZapSelectTxRet = WalletBatch(*database,"cr+").ZapSelectTx(vHashIn, vHashOut);
-    for (uint256 hash : vHashOut)
-        mapWallet.erase(hash);
+    for (uint256 hash : vHashOut) {


nit, could change to const uint256&.

promag · 2018-06-12T15:19:56Z

    char fFromMe;
    std::string strFromAccount;
    int64_t nOrderPos; //!< position in ordered transaction list
+    std::multimap<int64_t, std::pair<CWalletTx*, CAccountingEntry*>>::const_iterator m_it_wtxOrdered;


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

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

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

promag · 2018-06-12T15:21:59Z

-        mapWallet.erase(hash);
+    for (uint256 hash : vHashOut) {
+        const auto& it = mapWallet.find(hash);
+        wtxOrdered.erase(it->second.m_it_wtxOrdered);


Something like:

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

promag · 2018-06-14T00:57:35Z

Should backport?

DrahtBot · 2018-06-14T20:20:47Z

Note to reviewers: This pull request conflicts with the following ones:

Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. #13249 (Make objects in range declarations immutable by default by practicalswift)
Refactor: separate wallet from node #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 · 2018-06-18T15:33:25Z

utACK faa18ca

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.

faa18ca wallet: Erase wtxOrderd wtx pointer on removeprunedfunds (MarcoFalke) Pull request description: This prevents segfaults, when reading from the freed memory. Tree-SHA512: 04f8190dea7901cf1cc298d5db98c83b02858f27114c5ef4da738accd176d6647d6b81f3dc39f3d5912b1a981cf0599370fd391c4154ffbde97afc1fac389123

jnewbery · 2018-06-18T20:47:10Z

tested ACK faa18ca

Github-Pull: bitcoin#13437 Rebased-From: faa18ca

fanquake · 2018-07-13T23:57:59Z

Should be backported in #13644.

Github-Pull: bitcoin#13437 Rebased-From: faa18ca

Summary: faa18ca wallet: Erase wtxOrderd wtx pointer on removeprunedfunds (MarcoFalke) Pull request description: This prevents segfaults, when reading from the freed memory. Tree-SHA512: 04f8190dea7901cf1cc298d5db98c83b02858f27114c5ef4da738accd176d6647d6b81f3dc39f3d5912b1a981cf0599370fd391c4154ffbde97afc1fac389123 Backport of Core PR13437 bitcoin/bitcoin#13437 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4540

…nedfunds faa18ca wallet: Erase wtxOrderd wtx pointer on removeprunedfunds (MarcoFalke) Pull request description: This prevents segfaults, when reading from the freed memory. Tree-SHA512: 04f8190dea7901cf1cc298d5db98c83b02858f27114c5ef4da738accd176d6647d6b81f3dc39f3d5912b1a981cf0599370fd391c4154ffbde97afc1fac389123

Summary: faa18ca046e9043b2cf68cb1bd17cc8c60fe26d9 wallet: Erase wtxOrderd wtx pointer on removeprunedfunds (MarcoFalke) Pull request description: This prevents segfaults, when reading from the freed memory. Tree-SHA512: 04f8190dea7901cf1cc298d5db98c83b02858f27114c5ef4da738accd176d6647d6b81f3dc39f3d5912b1a981cf0599370fd391c4154ffbde97afc1fac389123 Backport of Core PR13437 bitcoin/bitcoin#13437 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D4540

wallet: Erase wtxOrderd wtx pointer on removeprunedfunds

faa18ca

maflcko added Wallet RPC/REST/ZMQ labels Jun 11, 2018

Empact reviewed Jun 12, 2018

View reviewed changes

promag reviewed Jun 12, 2018

View reviewed changes

laanwj mentioned this pull request Jun 18, 2018

too many removeprunedfunds and listtransactions , bitcoind will be crash #9729

Closed

laanwj merged commit faa18ca into bitcoin:master Jun 18, 2018

laanwj added this to the 0.16.2 milestone Jun 18, 2018

laanwj added the Needs backport label Jun 18, 2018

ryanofsky mentioned this pull request Jun 19, 2018

Remove CWalletTx merging logic from AddToWallet #9381

Merged

Bushstar mentioned this pull request Jun 20, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#348

Merged

ryanofsky mentioned this pull request Jun 26, 2018

Refactor: separate wallet from node #10973

Merged

maflcko deleted the Mf1806-walletPrunedFundsSegfault branch July 11, 2018 22:41

fanquake mentioned this pull request Jul 12, 2018

0.16: Remaining backports for 0.16.2 #13644

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jul 13, 2018

wallet: Erase wtxOrderd wtx pointer on removeprunedfunds

ed82e71

Github-Pull: bitcoin#13437 Rebased-From: faa18ca

fanquake removed the Needs backport label Jul 13, 2018

SmarterHomes pushed a commit to chaincoin/chaincoin that referenced this pull request Jan 11, 2019

wallet: Erase wtxOrderd wtx pointer on removeprunedfunds

aa9236b

Github-Pull: bitcoin#13437 Rebased-From: faa18ca

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Uh oh!

Conversation

maflcko commented Jun 11, 2018

Uh oh!

practicalswift commented Jun 11, 2018

Uh oh!

maflcko commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jun 12, 2018

Uh oh!

maflcko commented Jun 12, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

promag Jun 12, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Jun 14, 2018

Uh oh!

DrahtBot commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 18, 2018

Uh oh!

jnewbery commented Jun 18, 2018

Uh oh!

fanquake commented Jul 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

maflcko commented Jun 11, 2018 •

edited

Loading

DrahtBot commented Jun 14, 2018 •

edited

Loading