rpc: Work around Clang thread safety analysis quirks #11734

practicalswift commented at 1:18 PM on November 20, 2017: contributor

The conditional lock ...

LOCK2(cs_main, pwallet ? &pwallet->cs_wallet : nullptr)

... confuses Clang's thread safety analysis (it complains about not holding the mutex pwallet->cs_wallet even in the case when pwallet is non-NULL).

So does the access to pwallet->mapKeyMetadata via meta (it complains about not holding the mutex cs_wallet despite pwallet->cs_wallet being held).

This commit introduces locking that Clang's thread safety analysis is able to comprehend.

fanquake added the label RPC/REST/ZMQ on Nov 20, 2017

fanquake commented at 1:31 PM on November 20, 2017: member

This looks like #11691 without the mining.cpp changes. Can you incorporate the changes suggested in comments from that PR?

in src/rpc/misc.cpp:161 in 0e7edca1cb outdated

 157 | @@ -158,11 +158,8 @@ UniValue validateaddress(const JSONRPCRequest& request)
 158 |  
 159 |  #ifdef ENABLE_WALLET
 160 |      CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
 161 | -
 162 | -    LOCK2(cs_main, pwallet ? &pwallet->cs_wallet : nullptr);
 163 | -#else
 164 | -    LOCK(cs_main);
 165 |  #endif
 166 | +    LOCK(cs_main);

promag commented at 3:18 PM on November 20, 2017:

How about not changing behaviour:

LOCK(cs_main);
#ifdef ENABLE_WALLET
if (pwallet) LOCK(pwallet->cs_wallet);
#endif

As it is, the wallet can change between the cs_main lock and cs_wallet lock. This way the diff is also shorter.

Edit: ops, it doesn't work.

practicalswift commented at 5:27 PM on November 20, 2017:

@promag Regarding your edit - do you mean that you ran into Clang thread safety analysis warnings?

promag commented at 3:18 PM on November 20, 2017: member

Concept ACK.

practicalswift force-pushed on Nov 20, 2017

practicalswift commented at 6:10 PM on November 20, 2017: contributor

@fanquake Done!

practicalswift force-pushed on Nov 21, 2017

practicalswift commented at 8:29 AM on November 21, 2017: contributor

@promag Would you mind re-reviewing? One way to make the Clang thread safety analysis machinery happy without changing behaviour seems to be to repeat the locking of the recursive mutex pwallet->cs_wallet with a lock in the if (pwallet) scope as well. That inner scope lock is technically redundant. Let me know if you find a cleaner way to do it. (change reverted)

practicalswift force-pushed on Nov 21, 2017

TheBlueMatt commented at 12:00 AM on November 22, 2017: member

It probably makes more sense to focus more on splitting out the wallet RPC stuff (ala @achow101's work) and then you can just include the wallet lock in a wallet-specific deprecated function.

On November 20, 2017 5:18:15 AM PST, practicalswift notifications@github.com wrote:

The conditional lock ...
LOCK2(cs_main, pwallet ? &pwallet->cs_wallet : nullptr)
... confuses Clang's thread safety analysis (it complains about not holding the mutex pwallet->cs_wallet even in the case when pwallet is non-NULL).

So does the access to pwallet->mapKeyMetadata via meta (it complains about not holding the mutex cs_wallet despite pwallet->cs_wallet being held).

This commit introduces locking that Clang's thread safety analysis is able to comprehend. You can view, comment on, or merge this pull request online at:

#11734

-- Commit Summary --

rpc: Work around Clang thread safety analysis quirks

-- File Changes --

M src/rpc/misc.cpp (21)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/11734.patch https://github.com/bitcoin/bitcoin/pull/11734.diff

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11734

in src/rpc/misc.cpp:163 in 38afca9ca0 outdated

 157 | @@ -158,8 +158,10 @@ UniValue validateaddress(const JSONRPCRequest& request)
 158 |  
 159 |  #ifdef ENABLE_WALLET
 160 |      CWallet * const pwallet = GetWalletForJSONRPCRequest(request);
 161 | -
 162 | -    LOCK2(cs_main, pwallet ? &pwallet->cs_wallet : nullptr);
 163 | +    LOCK(cs_main);
 164 | +    if (pwallet) {
 165 | +        LOCK(pwallet->cs_wallet);

sipa commented at 2:08 AM on November 22, 2017:

This has no effect, the lock is released at the closing brace.

practicalswift commented at 7:09 AM on November 22, 2017:

Oh, of course! I don't know what I was thinking there :-\ Fixing!

rpc: Work around Clang thread safety analysis quirks

The conditional lock ...

```
LOCK2(cs_main, pwallet ? &pwallet->cs_wallet : nullptr)
```

... confuses Clang's thread safety analysis (it complains
about not holding the mutex pwallet->cs_wallet even in the
case when pwallet is non-NULL).

So does the access to `pwallet->mapKeyMetadata` via `meta`
(it complains about not holding the mutex `cs_wallet`
despite `pwallet->cs_wallet` being held).

This commit introduces locking that Clang's thread safety
analysis is able to understand correctly.

6a9bd4e638

practicalswift force-pushed on Nov 22, 2017

practicalswift commented at 7:35 AM on November 22, 2017: contributor

@sipa Thanks for reviewing! New version pushed. Would you mind re-reviewing? :-)

practicalswift closed this on Dec 13, 2017

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022