From d002f9d15d938e78360ad906f2d74a249c7e923e Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Mon, 20 Nov 2017 12:51:45 -0500 Subject: Disable CWalletTx copy constructor Disable copying of CWalletTx objects to prevent bugs where instances get copied in and out of the mapWallet map and fields are updated in the wrong copy. --- src/wallet/rpcwallet.cpp | 2 +- src/wallet/test/psbt_wallet_tests.cpp | 6 ++---- src/wallet/wallet.cpp | 4 ++-- src/wallet/wallet.h | 8 +++++++- src/wallet/walletdb.cpp | 13 +++++-------- src/wallet/walletdb.h | 4 ++-- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 91162d575d..677b799d06 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1582,7 +1582,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request) UniValue transactions(UniValue::VARR); for (const std::pair& pairWtx : pwallet->mapWallet) { - CWalletTx tx = pairWtx.second; + const CWalletTx& tx = pairWtx.second; if (depth == -1 || abs(tx.GetDepthInMainChain()) < depth) { ListTransactions(pwallet, tx, 0, true, transactions, filter, nullptr /* filter_label */); diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp index 35860577cd..b4c65a8665 100644 --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -22,14 +22,12 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test) CDataStream s_prev_tx1(ParseHex("0200000000010158e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd7501000000171600145f275f436b09a8cc9a2eb2a2f528485c68a56323feffffff02d8231f1b0100000017a914aed962d6654f9a2b36608eb9d64d2b260db4f1118700c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88702483045022100a22edcc6e5bc511af4cc4ae0de0fcd75c7e04d8c1c3a8aa9d820ed4b967384ec02200642963597b9b1bc22c75e9f3e117284a962188bf5e8a74c895089046a20ad770121035509a48eb623e10aace8bfd0212fdb8a8e5af3c94b0b133b95e114cab89e4f7965000000"), SER_NETWORK, PROTOCOL_VERSION); CTransactionRef prev_tx1; s_prev_tx1 >> prev_tx1; - CWalletTx prev_wtx1(&m_wallet, prev_tx1); - m_wallet.mapWallet.emplace(prev_wtx1.GetHash(), std::move(prev_wtx1)); + m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx1->GetHash()), std::forward_as_tuple(&m_wallet, prev_tx1)); CDataStream s_prev_tx2(ParseHex("0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f618765000000"), SER_NETWORK, PROTOCOL_VERSION); CTransactionRef prev_tx2; s_prev_tx2 >> prev_tx2; - CWalletTx prev_wtx2(&m_wallet, prev_tx2); - m_wallet.mapWallet.emplace(prev_wtx2.GetHash(), std::move(prev_wtx2)); + m_wallet.mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(prev_tx2->GetHash()), std::forward_as_tuple(&m_wallet, prev_tx2)); // Add scripts CScript rs1; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 505e394aa1..36f00b8959 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3096,7 +3096,7 @@ DBErrors CWallet::ZapSelectTx(std::vector& vHashIn, std::vector& vWtx) +DBErrors CWallet::ZapWalletTx(std::list& vWtx) { DBErrors nZapWalletTxRet = WalletBatch(*database,"cr+").ZapWalletTx(vWtx); if (nZapWalletTxRet == DBErrors::NEED_REWRITE) @@ -3708,7 +3708,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const std::string walletFile = WalletDataFilePath(location.GetPath()).string(); // needed to restore wallet transaction meta data after -zapwallettxes - std::vector vWtx; + std::list vWtx; if (gArgs.GetBoolArg("-zapwallettxes", false)) { chain.initMessage(_("Zapping all transactions from wallet...").translated); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 82f45e3d33..932e1a28b8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -553,6 +553,12 @@ public: const uint256& GetHash() const { return tx->GetHash(); } bool IsCoinBase() const { return tx->IsCoinBase(); } bool IsImmatureCoinBase() const; + + // Disable copying of CWalletTx objects to prevent bugs where instances get + // copied in and out of the mapWallet map, and fields are updated in the + // wrong copy. + CWalletTx(CWalletTx const &) = delete; + void operator=(CWalletTx const &x) = delete; }; class COutput @@ -1057,7 +1063,7 @@ public: void chainStateFlushed(const CBlockLocator& loc) override; DBErrors LoadWallet(bool& fFirstRunRet); - DBErrors ZapWalletTx(std::vector& vWtx); + DBErrors ZapWalletTx(std::list& vWtx); DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::string& purpose); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 303570e320..d4f060410b 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -738,7 +738,7 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) return result; } -DBErrors WalletBatch::FindWalletTx(std::vector& vTxHash, std::vector& vWtx) +DBErrors WalletBatch::FindWalletTx(std::vector& vTxHash, std::list& vWtx) { DBErrors result = DBErrors::LOAD_OK; @@ -776,12 +776,9 @@ DBErrors WalletBatch::FindWalletTx(std::vector& vTxHash, std::vector> hash; - - CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef()); - ssValue >> wtx; - vTxHash.push_back(hash); - vWtx.push_back(wtx); + vWtx.emplace_back(nullptr /* wallet */, nullptr /* tx */); + ssValue >> vWtx.back(); } } pcursor->close(); @@ -800,7 +797,7 @@ DBErrors WalletBatch::ZapSelectTx(std::vector& vTxHashIn, std::vector vTxHash; - std::vector vWtx; + std::list vWtx; DBErrors err = FindWalletTx(vTxHash, vWtx); if (err != DBErrors::LOAD_OK) { return err; @@ -834,7 +831,7 @@ DBErrors WalletBatch::ZapSelectTx(std::vector& vTxHashIn, std::vector& vWtx) +DBErrors WalletBatch::ZapWalletTx(std::list& vWtx) { // build list of wallet TXs std::vector vTxHash; diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 2701481c58..681ca10bb1 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -260,8 +260,8 @@ public: bool WriteActiveScriptPubKeyMan(uint8_t type, const uint256& id, bool internal); DBErrors LoadWallet(CWallet* pwallet); - DBErrors FindWalletTx(std::vector& vTxHash, std::vector& vWtx); - DBErrors ZapWalletTx(std::vector& vWtx); + DBErrors FindWalletTx(std::vector& vTxHash, std::list& vWtx); + DBErrors ZapWalletTx(std::list& vWtx); DBErrors ZapSelectTx(std::vector& vHashIn, std::vector& vHashOut); /* Try to (very carefully!) recover wallet database (with a possible key type filter) */ static bool Recover(const fs::path& wallet_path, void *callbackDataIn, bool (*recoverKVcallback)(void* callbackData, CDataStream ssKey, CDataStream ssValue), std::string& out_backup_filename); -- cgit v1.2.3