Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing lock to crypter GetKeys() #10916

Merged
merged 2 commits into from Sep 7, 2017

Conversation

Projects
None yet
5 participants
@benma
Copy link
Contributor

benma commented Jul 23, 2017

Issue: #10905

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

Marko Bencun added some commits Jul 23, 2017

Marko Bencun
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.
Marko Bencun
@@ -162,28 +162,26 @@ class CCryptoKeyStore : public CBasicKeyStore
{
{

This comment has been minimized.

@benma

benma Jul 23, 2017

Author Contributor

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

This comment has been minimized.

@bytting

bytting Jul 23, 2017

Contributor

It certainly looks like it. The lock goes out of scope either way, and that last return is unreachable as it stands now

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jul 24, 2017

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 fe09b01

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Sep 4, 2017

utACK fe09b01. As @jonasschnelli points out we need to also fix fUseCrypto to either be atomic or under the lock.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 7, 2017

Tested ACK fe09b01
Ready for merge. Another pair of eyes would be good.

@laanwj laanwj merged commit fe09b01 into bitcoin:master Sep 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Sep 7, 2017

Merge #10916: add missing lock to crypter GetKeys()
fe09b01 add missing lock to crypter GetKeys() (Marko Bencun)
5cb3da0 keystore GetKeys(): return result instead of writing to reference (Marko Bencun)

Pull request description:

  Issue: #10905

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

Tree-SHA512: bb51255b5a6cf5488c3d5dee89f539d41f0717f018441d120047f877e0a705a133fb3b7a97d1cf8f73b5d2ed93dd2dbdfcd6f394e40105af2a12e01d397cb402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.