rpc: Fix data race (UB) in InterruptRPC() #14993

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:rpc-datarace changing 2 files +4 −7

practicalswift commented at 11:50 AM on December 18, 2018: contributor

Fix data race (UB) in InterruptRPC().

Before:

$ ./configure --with-sanitizers=thread
$ make
$ test/functional/test_runner.py feature_shutdown.py
…
SUMMARY: ThreadSanitizer: data race rpc/server.cpp:314 in InterruptRPC()
…
ALL                 | ✖ Failed  | 2 s (accumulated)

After:

$ ./configure --with-sanitizers=thread
$ make
$ test/functional/test_runner.py feature_shutdown.py
…
ALL                 | ✓ Passed  | 3 s (accumulated)

fanquake added the label RPC/REST/ZMQ on Dec 18, 2018
laanwj commented at 1:56 PM on December 18, 2018: member

utACK 10f8244a777392f87c2671d743b14201d24351be

I don't think this can actually result in a data race but an atomic is certainly more appropriate here.
in src/rpc/server.cpp:27 in 10f8244a77 outdated
```
  23 | @@ -24,7 +24,7 @@
  24 |  #include <unordered_map>
  25 |  
  26 |  static CCriticalSection cs_rpcWarmup;
  27 | -static bool fRPCRunning = false;
  28 | +static std::atomic<bool> fRPCRunning{false};
```
promag commented at 2:36 PM on December 18, 2018:

This is the only change needed, why the other changes when there's assign and cast operators?

MarcoFalke commented at 5:36 PM on December 18, 2018:

Could rename to g_rpc_running to clarify that it is a global?
practicalswift force-pushed on Dec 18, 2018
promag commented at 2:57 PM on December 18, 2018: member

utACK 9ed8713.
practicalswift cross-referenced this on Dec 18, 2018 from issue travis: Enable functional tests in the ThreadSanitizer (TSan) build job by practicalswift
MarcoFalke commented at 5:37 PM on December 18, 2018: member

Remove from the suppressions file?
practicalswift force-pushed on Dec 18, 2018
rpc: Fix data race (UB) in InterruptRPC() 6c10037f72
practicalswift force-pushed on Dec 18, 2018
practicalswift commented at 5:52 PM on December 18, 2018: contributor

@MarcoFalke Done! Please re-review :-)
promag commented at 8:18 PM on December 18, 2018: member

utACK 6c10037.
laanwj merged this on Dec 19, 2018
laanwj closed this on Dec 19, 2018
laanwj referenced this in commit cb52cee29d on Dec 19, 2018
Sjors commented at 2:35 PM on December 21, 2018: member

I'm still getting feature_shutdown.py failures on Travis, see e.g. https://travis-ci.org/bitcoin/bitcoin/jobs/470997593#L3104
promag commented at 3:34 PM on December 21, 2018: member

@Sjors that's fixed by #14958 which depends on #14982.
ryanofsky cross-referenced this on Feb 1, 2019 from issue tests: Running with --usecli ~5x slower than without by fanquake
markblundeberg commented at 9:45 AM on February 6, 2020: none

(introduced in #14670)
markblundeberg referenced this in commit 835636d9b0 on Feb 7, 2020
practicalswift deleted the branch on Apr 10, 2021
LarryRuane referenced this in commit 21786296e3 on Apr 29, 2021
LarryRuane cross-referenced this on May 4, 2021 from issue Bitcoin 0.18 locking PRs by LarryRuane
LarryRuane referenced this in commit bf8b1826a4 on Jun 1, 2021
christiancfifi referenced this in commit 723c86cee9 on Aug 24, 2021
CharesFang cross-referenced this on Dec 8, 2021 from issue Backports from Bitcoin by CharesFang
CharesFang cross-referenced this on Dec 8, 2021 from issue Backports from Bitcoin by CharesFang
CharesFang cross-referenced this on Dec 9, 2021 from issue Backports from Bitcoin by CharesFang
CharesFang cross-referenced this on Dec 9, 2021 from issue Backports from Bitcoin by CharesFang
CharesFang cross-referenced this on Dec 9, 2021 from issue Backports from Bitcoin by CharesFang
CharesFang cross-referenced this on Dec 11, 2021 from issue Backports from Bitcoin by CharesFang
CharesFang cross-referenced this on Dec 12, 2021 from issue Backports from Bitcoin by CharesFang
CharesFang cross-referenced this on Dec 13, 2021 from issue [Backport] Backports from Bitcoin by CharesFang
Munkybooty referenced this in commit 5b9980c204 on Dec 28, 2021
Munkybooty referenced this in commit 7a76080da6 on Dec 28, 2021
Munkybooty referenced this in commit 6044be6546 on Dec 28, 2021
Munkybooty referenced this in commit 3b4b30f730 on Dec 28, 2021
Munkybooty referenced this in commit 52ae8804da on Dec 28, 2021
Munkybooty referenced this in commit 82aa95d096 on Jan 3, 2022
Munkybooty referenced this in commit e5a755c648 on Feb 1, 2022
Munkybooty referenced this in commit 086dcbda00 on Feb 16, 2022
Munkybooty referenced this in commit ad40f0699c on Feb 24, 2022
Munkybooty referenced this in commit 802627ecd2 on Feb 24, 2022
Munkybooty referenced this in commit f3dd6306a7 on Mar 6, 2022
Munkybooty referenced this in commit 3ea36c9d8b on Mar 20, 2022
Munkybooty referenced this in commit 5ee8d74b22 on Apr 21, 2022
Munkybooty referenced this in commit 755db74ca5 on Apr 26, 2022
Munkybooty referenced this in commit 969a5cedb7 on Apr 29, 2022
Munkybooty referenced this in commit 30b5874c44 on May 10, 2022
Munkybooty referenced this in commit 2374eddea1 on May 12, 2022
Munkybooty referenced this in commit 6412b22b94 on May 12, 2022
Munkybooty referenced this in commit 6626ee0fb3 on May 17, 2022
Munkybooty referenced this in commit 3dbc611e55 on May 23, 2022
bitcoin locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#14670 http: Fix HTTP server shutdown #14829 travis: Enable functional tests in the ThreadSanitizer (TSan) build job #14958 qa: Remove race between connecting and shutdown on separate connections #14982 rpc: Add getrpcinfo command #15309 tests: Running with --usecli ~5x slower than without