rpc: Add missing lock in getblocktemplate(...). Work around Clang thread safety analysis quirks. #11691

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:rpc-locking changing 2 files +20 −17

practicalswift commented at 9:30 AM on November 15, 2017: contributor
Commit 1: Add missing lock in getblocktemplate(...)

Reading the variable chainActive requires holding the mutex cs_main.

Prior to this commit the cs_main mutex was not held when accessing chainActive in:
```
while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
```
Commit 2: 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 – which is the correct mutex – is being held).

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

rpc: Add missing lock in getblocktemplate(...)

Reading the variable `chainActive` requires holding the mutex `cs_main`.

Prior to this commit the `cs_main` mutex was not held when accessing
`chainActive` in:

```
while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
```

1c05f6d449

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 locked).

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

44bc37c5cf

fanquake added the label RPC/REST/ZMQ on Nov 15, 2017
luke-jr commented at 12:38 PM on November 15, 2017: member

NACK... locks do more than just data integrity; they're used here to ensure a consistent state at the right points.
in src/rpc/misc.cpp:162 in 44bc37c5cf
```
 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 12:43 PM on November 15, 2017:

@luke-jr but agree with this change?

luke-jr commented at 12:52 PM on November 15, 2017:

Does IsMine not require the lock? If you're going to do this, I suggest moving GetWalletForJSONRPCRequest closer to the lock as well.

promag commented at 1:03 PM on November 15, 2017:

Does IsMine not require the lock

I don't think so.

in src/rpc/misc.cpp:185 in 44bc37c5cf

 184 | -            ret.push_back(Pair("account", pwallet->mapAddressBook[dest].name));
 185 | -        }
 186 |          if (pwallet) {
 187 | -            const auto& meta = pwallet->mapKeyMetadata;
 188 | +            LOCK(pwallet->cs_wallet);
 189 | +            if (pwallet->mapAddressBook.count(dest)) {

promag commented at 12:44 PM on November 15, 2017:

Could use find to avoid 2nd lookup below.

practicalswift closed this on Nov 15, 2017
practicalswift commented at 3:13 PM on November 15, 2017: contributor

@luke-jr Thanks for reviewing. Closing this PR. I'll probably create a new PR with a subset of the getblocktemplate changes. More precisely I'll let the cs_main remain as-is but add additional locking where required for chainActive.
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels