add missing lock to crypter GetKeys() #10916

pull benma wants to merge 2 commits into bitcoin:master from benma:GetKeys changing 3 files +20 −29

benma commented at 9:41 PM on July 23, 2017: none

Issue: #10905

First commit makes GetKeys() return the result instead of writing to a reference to remove some useless lines.

keystore GetKeys(): return result instead of writing to reference

Issue: #10905

By returning the result, a few useless lines can be removed.

Return-value-optimization means there should be no copy.

5cb3da04b8

add missing lock to crypter GetKeys()
Issue: #10905
fe09b0197c
in src/wallet/crypter.h:163 in fe09b0197c
```
 161 | @@ -162,28 +162,26 @@ class CCryptoKeyStore : public CBasicKeyStore
 162 |      {
 163 |          {
```
benma commented at 9:43 PM on July 23, 2017:

Could this scope level be removed alongside with the return false in the end, or am I missing something about how LOCK works?

bytting commented at 11:23 PM on July 23, 2017:

It certainly looks like it. The lock goes out of scope either way, and that last return is unreachable as it stands now
jonasschnelli commented at 7:28 AM on July 24, 2017: contributor

I guess the setAddress pass by reference was a humble try to keep the memory control on the caller side and have it properly cleaned. But seems not to be done well.

The lock in GetKeys is definitively missing. Also, the IsCrypted() method is sometimes called without the cs_KeyStore so, fUseCrypto should be turned into an atomic.

It was never a problem because – at the moment – GetKeys get only called when holding cs_main/cs_wallet.

utACK fe09b0197c20dc3c0a614c1a94dac708ef206743
jonasschnelli added the label Refactoring on Jul 24, 2017
jonasschnelli added the label Wallet on Jul 24, 2017
TheBlueMatt commented at 9:36 PM on September 4, 2017: member

utACK fe09b0197c20dc3c0a614c1a94dac708ef206743. As @jonasschnelli points out we need to also fix fUseCrypto to either be atomic or under the lock.
jonasschnelli commented at 7:59 PM on September 7, 2017: contributor

Tested ACK fe09b0197c20dc3c0a614c1a94dac708ef206743 Ready for merge. Another pair of eyes would be good.
laanwj merged this on Sep 7, 2017
laanwj closed this on Sep 7, 2017
laanwj referenced this in commit e6ab88a452 on Sep 7, 2017
PastaPastaPasta referenced this in commit bd15d75b5f on Sep 23, 2019
PastaPastaPasta referenced this in commit cacb81c34e on Dec 21, 2019
PastaPastaPasta referenced this in commit 1781c7e4fe on Jan 2, 2020
PastaPastaPasta referenced this in commit e97e8e199f on Jan 4, 2020
PastaPastaPasta referenced this in commit 07704e56d1 on Jan 4, 2020
PastaPastaPasta referenced this in commit d3b6c0dff6 on Jan 10, 2020
PastaPastaPasta referenced this in commit 3884cfb3af on Jan 10, 2020
PastaPastaPasta referenced this in commit d7a2232103 on Jan 10, 2020
PastaPastaPasta referenced this in commit 3af9698d27 on Jan 12, 2020
LarryRuane referenced this in commit dfce32e683 on Mar 3, 2021
ckti referenced this in commit 44d3a64228 on Mar 28, 2021
LarryRuane referenced this in commit 2fd8809300 on Apr 13, 2021
zkbot referenced this in commit 0e36226271 on Apr 17, 2021
gades referenced this in commit 89b1ebbc51 on Jun 30, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#10905 setAddress.clear() without a lock in CCryptoKeyStore::GetKeys and CBasicKeyStore::GetKeys