From 4757e923185d48c3169bc2d216db88484f036455 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 12 Dec 2013 10:12:27 +0100 Subject: [PATCH 1/4] qt: add missing cs_wallet lock in AddressTableModel::setData duplicate check in AddressTableModel::setData accesses wallet data structure as well as SetAddressBook without proper LOCK, fix this. --- src/qt/addresstablemodel.cpp | 37 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/src/qt/addresstablemodel.cpp b/src/qt/addresstablemodel.cpp index 5e7d8e617..2987e5fdd 100644 --- a/src/qt/addresstablemodel.cpp +++ b/src/qt/addresstablemodel.cpp @@ -244,33 +244,34 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value, if(role == Qt::EditRole) { - switch(index.column()) + LOCK(wallet->cs_wallet); /* For SetAddressBook / DelAddressBook */ + CTxDestination curAddress = CBitcoinAddress(rec->address.toStdString()).Get(); + if(index.column() == Label) { - case Label: // Do nothing, if old label == new label if(rec->label == value.toString()) { editStatus = NO_CHANGES; return false; } - wallet->SetAddressBook(CBitcoinAddress(rec->address.toStdString()).Get(), value.toString().toStdString(), strPurpose); - break; - case Address: - // Do nothing, if old address == new address - if(CBitcoinAddress(rec->address.toStdString()) == CBitcoinAddress(value.toString().toStdString())) + wallet->SetAddressBook(curAddress, value.toString().toStdString(), strPurpose); + } else if(index.column() == Address) { + CTxDestination newAddress = CBitcoinAddress(value.toString().toStdString()).Get(); + // Refuse to set invalid address, set error status and return false + if(boost::get(&newAddress)) { - editStatus = NO_CHANGES; + editStatus = INVALID_ADDRESS; return false; } - // Refuse to set invalid address, set error status and return false - else if(!walletModel->validateAddress(value.toString())) + // Do nothing, if old address == new address + else if(newAddress == curAddress) { - editStatus = INVALID_ADDRESS; + editStatus = NO_CHANGES; return false; } // Check for duplicate addresses to prevent accidental deletion of addresses, if you try // to paste an existing address over another address (with a different label) - else if(wallet->mapAddressBook.count(CBitcoinAddress(value.toString().toStdString()).Get())) + else if(wallet->mapAddressBook.count(newAddress)) { editStatus = DUPLICATE_ADDRESS; return false; @@ -278,15 +279,11 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value, // Double-check that we're not overwriting a receiving address else if(rec->type == AddressTableEntry::Sending) { - { - LOCK(wallet->cs_wallet); - // Remove old entry - wallet->DelAddressBook(CBitcoinAddress(rec->address.toStdString()).Get()); - // Add new entry with new address - wallet->SetAddressBook(CBitcoinAddress(value.toString().toStdString()).Get(), rec->label.toStdString(), strPurpose); - } + // Remove old entry + wallet->DelAddressBook(curAddress); + // Add new entry with new address + wallet->SetAddressBook(newAddress, rec->label.toStdString(), strPurpose); } - break; } return true; } From aaf8d157082ec0aec03c734ab3368fa8e2c4556c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sun, 15 Dec 2013 13:37:10 +0100 Subject: [PATCH 2/4] qt: Add missing LOCKs for locked coin functions These don't aquire the wallet lock internally, so the caller has to do it. --- src/qt/walletmodel.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index f08342b83..78bc17062 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -531,20 +531,24 @@ void WalletModel::listCoins(std::map >& mapCoins) bool WalletModel::isLockedCoin(uint256 hash, unsigned int n) const { + LOCK(wallet->cs_wallet); return wallet->IsLockedCoin(hash, n); } void WalletModel::lockCoin(COutPoint& output) { + LOCK(wallet->cs_wallet); wallet->LockCoin(output); } void WalletModel::unlockCoin(COutPoint& output) { + LOCK(wallet->cs_wallet); wallet->UnlockCoin(output); } void WalletModel::listLockedCoins(std::vector& vOutpts) { + LOCK(wallet->cs_wallet); wallet->ListLockedCoins(vOutpts); } From 28352af0606cea00cd80e8f353bda02fadf2222c Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Sun, 15 Dec 2013 13:54:07 +0100 Subject: [PATCH 3/4] qt: protect SetAddressBook with cs_wallet lock everywhere --- src/qt/paymentserver.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index ba5c06064..cfa87c16b 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -548,6 +548,7 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, SendCoinsRecipient recipien else { CPubKey newKey; if (wallet->GetKeyFromPool(newKey)) { + LOCK(wallet->cs_wallet); // SetAddressBook CKeyID keyID = newKey.GetID(); wallet->SetAddressBook(keyID, strAccount, "refund"); From d31ad26550403d9ed8d5aedd0f65d610b55d9497 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Mon, 6 Jan 2014 12:58:06 +0100 Subject: [PATCH 4/4] qt: Add missing lock in WalletModel::listCoins Another problem detected by cs_wallet lock detection (#3401). --- src/qt/walletmodel.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 78bc17062..6f3e3b0aa 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -502,6 +502,7 @@ void WalletModel::listCoins(std::map >& mapCoins) std::vector vCoins; wallet->AvailableCoins(vCoins); + LOCK(wallet->cs_wallet); // ListLockedCoins, mapWallet std::vector vLockedCoins; wallet->ListLockedCoins(vLockedCoins);