qt: add missing cs_wallet locks #3413

pull laanwj wants to merge 4 commits into bitcoin:master from laanwj:2013_12_gui_wallet_mutex changing 3 files +23 −20

laanwj commented at 3:49 PM on December 13, 2013: member

The duplicate check in AddressTableModel::setData accesses wallet data structure without proper LOCK, fix this.

in src/qt/addresstablemodel.cpp:None in 6ca44a4bc8 outdated

 290 | @@ -286,7 +291,7 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
 291 |                      wallet->SetAddressBook(CBitcoinAddress(value.toString().toStdString()).Get(), rec->label.toStdString(), strPurpose);
 292 |                  }
 293 |              }
 294 | -            break;
 295 | +            } break;

sipa commented at 12:56 PM on December 15, 2013:

That's strange style...?

laanwj commented at 12:58 PM on December 15, 2013:

So, how would you propose doing it? I didn't want to double-indent the entire block either.

in src/qt/addresstablemodel.cpp:None in 3e321180e2 outdated

 243 | @@ -244,33 +244,34 @@ bool AddressTableModel::setData(const QModelIndex &index, const QVariant &value,
 244 |  
 245 |      if(role == Qt::EditRole)
 246 |      {
 247 | -        switch(index.column())
 248 | +        LOCK(wallet->cs_wallet); /* For SetAddressBook / DelAddressBook */

laanwj commented at 2:10 PM on December 15, 2013:

@sipa I've refactored the function a bit do prevent the strange style anywhere and make it easier to read

BitcoinPullTester commented at 2:37 PM on December 15, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3e321180e2d559da67f23afae62beeee401d5ef2 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

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.

4757e92318

qt: Add missing LOCKs for locked coin functions

These don't aquire the wallet lock internally, so the caller has to do
it.

aaf8d15708

qt: protect SetAddressBook with cs_wallet lock everywhere 28352af060

qt: Add missing lock in WalletModel::listCoins

Another problem detected by cs_wallet lock detection (#3401).

d31ad26550

in src/qt/addresstablemodel.cpp:None in 3e321180e2 outdated
```
 260 | -            break;
 261 | -        case Address:
 262 | -            // Do nothing, if old address == new address
 263 | -            if(CBitcoinAddress(rec->address.toStdString()) == CBitcoinAddress(value.toString().toStdString()))
 264 | +            wallet->SetAddressBook(curAddress, value.toString().toStdString(), strPurpose);
 265 | +        } else if(index.column() == Address) {
```
Diapolo commented at 2:17 PM on December 23, 2013:

I find such else if's rather hard to read, when they are behind a }.

laanwj commented at 2:20 PM on December 23, 2013:

Let's just agree that this is better than the original? The point of this pull is to fix some serious locking issues, not to change the code into a work of art.

Diapolo commented at 2:26 PM on December 23, 2013:

Sure and agreed, but I'm allowed to give such feedback anyway :)? I consider myself of no help in code parts with locking and such, sorry...
laanwj referenced this in commit 37d30ec3cf on Jan 6, 2014
laanwj merged this on Jan 6, 2014
laanwj closed this on Jan 6, 2014
laanwj deleted the branch on Apr 9, 2014
DrahtBot locked this on Sep 8, 2021

Contributors