STDLOCK(mutex) instead of StdLockGuard guard(mutex) allows clang to propagate missing lock annotations backwards (for global locks or locks in the same class, anyway), and also avoids declaring a dummy name. Use this in logging.h, and also use it in sync.cpp, adding annotations around the internal structure.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
86+
87+ static inline StdMutex& CheckNotHeld(StdMutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
88 };
89
90+// Provide STDLOCK(..) wrapper around StdMutex::Guard that checks the lock is not already held
91+#define STDLOCK(cs) StdMutex::Guard UNIQUE_NAME(criticalblock){StdMutex::CheckNotHeld(cs)}
It’s worth noting that this is only required because we don’t use -Wthread-safety-negative. When using that, this warning is emitted without the need for CheckNotHeld().
Unfortunately, it also warns about all of our global locks. Arguably we want those warnings on, but it would require a ton of additional annotations. g_maplocalhost_mutex is especially annoying.
I don’t think it’s feasible without removing all global locks – every function that calls a function that takes a lock has to get an annotation (iirc including stdlib functions). eg:
0StdMutex x;
1int y GUARDED_BY(x);
2
3template<typename Fn>
4void g(Fn&& f) { f(9); }
5
6int main(void) EXCLUSIVE_LOCKS_REQUIRED(!x) {
7 auto f = [&](int i) EXCLUSIVE_LOCKS_REQUIRED(!x) { StdLockGuard xg{x}; ::y = i; };
8 g(f);
9 return 0;
10}
0negsafe.cpp:59:18: warning: calling function 'operator()' requires negative capability '!x' [-Wthread-safety-analysis]
1 59 | void g(Fn&& f) { f(9); }
2 | ^
3negsafe.cpp:63:5: note: in instantiation of function template specialization 'g<(lambda at negsafe.cpp:62:14) &>' requested here
4 63 | g(f);
5 | ^
61 warning generated.
class GM { Mutex mutex; } ? Would be a lot of churn when the macros we have already get us pretty much the same behaviour though.
Yeah, it wouldn’t be trivial. But FWIW, we’re currently vulnerable to the pattern you described, we just don’t have any checks for it.
Many of our “global” locks are actually contained to a single compilation unit. As you mentioned, those can safely be easily fixed by hiding them in a class and instantiating a static instance of the class instead.
Also, the negative warnings don’t apply to mutexes marked as re-entrant, so the nastiest ones (looking at you, cs_main) would still be exempt.
I have a patch for clang that adds an ASSUMES annotation for capabilities that means “assume I was unlocked at this point (trust me, bro), but enforce checks from here down”. The intended use-case was helping with migration away from nasty recursive mutexes (ala cs_main), but I’m realizing it’d be helpful in this case too. I should really get that upstreamed.
Several of the globals get in the way and are part of my todo list for my net refactor anyway. I didn’t mean to imply that we could turn on -Wthread-safety-negative any time soon, only that it would be a nice-to-have in the future.
But FWIW, we’re currently vulnerable to the pattern you described, we just don’t have any checks for it.
There’s no bug/vulnerability in the pattern I described, it’s just a limitation of clang’s thread safety pattern that it can’t sensibly track lock requirements when lock-sensitive functions are passed as arguments to generics, so you have to do it yourself. The same is true when you use std::function, which throws away annotations for the functions it wraps. Moving from global mutexes to non-private member mutexes (or mutexes exposed through GetMutex() helpers) just makes the checks weaker, so you get fewer false compile errors, but also get fewer checks.
Having a way to do “hierarchial mutexes” (every mutex gets assigned a level, attempting to acquire a mutex with level n fails if you have already acquired any other mutex with level m >= n) would be nice, particularly if we could derive the levels automatically from ACQUIRED_AFTER / ACQUIRED_BEFORE annotations or similar. I couldn’t figure out a way of doing that super conveniently, and we also have some cases where that’s not actually true currently (something like LOCK(A), LOCK(B) sometimes, LOCK(B), LOCK(C) at other times, and LOCK(C); LOCK(A) at other times, iirc; might have been cs_main, mempool.cs and m_tx_relay_mutex) which I think is a deficiency in our lock-order checking.
Nice improvements. crACK 913d28407b7d467bd4b74ccf232f926b259df74b.
It’d be nice to get #34639 in first though, so these changes are happening in a new header.
Also, I think it’s worth investigating doing the work to turn on Wthread-safety-negative and dropping our wrappers.
6d8bad26fa91b130efc1013b20867a8fb096abb5
StdLockGuard and clang’s thread safety annotations did not ensure that the lock it was taking was not already held.
How can I reproduce the failure described above? I made a naive attempt to trigger a double-lock on master using the following patch::
0--- a/src/logging.cpp
1+++ b/src/logging.cpp
2@@ -110,6 +110,7 @@ void BCLog::Logger::DisconnectTestLogger()
3
4 void BCLog::Logger::DisableLogging()
5 {
6+ StdLockGuard scoped_lock_0(m_cs);
7 {
8 StdLockGuard scoped_lock(m_cs);
9 assert(m_buffering);
However, this successfully triggers a -Wthread-safety-analysis warning:
0[52/134] Building CXX object src/util/CMakeFiles/bitcoin_util.dir/__/logging.cpp.o
1/home/hebasto/dev/bitcoin/src/logging.cpp:115:22: warning: acquiring 'm_cs' that is already held [-Wthread-safety-analysis]
2 115 | StdLockGuard scoped_lock(m_cs);
3 | ^
4/home/hebasto/dev/bitcoin/src/logging.cpp:113:18: note: acquired here
5 113 | StdLockGuard scoped_lock_0(m_cs);
6 | ^
71 warning generated.
If Clang is already catching this, what specific scenario was the commit message referring to?
@hebasto Removing the EXCLUSIVE_LOCKS_REQUIRED(!m_cs) from NumConnections() should now cause a warning.
So I think an undetected double-lock bug would’ve looked like:
0void BCLog::Logger::DisableLogging()
1{
2 StdLockGuard scoped_lock_0(m_cs);
3 auto foo = NumConnections();
4}
StdLockGuard and clang's thread safety annotations did not ensure that
the lock it was taking was not already held. Add a STDLOCK() macro which
uses an annotated StdMutex::CheckNotHeld() function to correct that.
-BEGIN VERIFY SCRIPT-
sed -i 's/StdLockGuard scoped_lock(\(.*\));/STDLOCK(\1);/' src/logging.h src/logging.cpp
-END VERIFY SCRIPT-
It’s left in to minimise interference / silent merge-conflicts with other PRs that might add uses of StdLockGuard, eg #34806.
That doesn’t make sense to me. When are we ever going to remove it then? In general, I don’t like people delaying because of potential merge conflicts. If multiple conflicting PRs are close to being merged, maintainers can coordinate to prioritize, but outside of that my experience is that it leads to weird dynamics
It’s left in to minimise interference / silent merge-conflicts with other PRs that might add uses of StdLockGuard, eg #34806.
That doesn’t make sense to me. When are we ever going to remove it then?
Are you kidding? It’s a one removal, so it is completely trivial to do at any time.
The time to do it is when there are no open PRs that would be broken by removing that line. If both #34809 and #34806 are merged as-is, then the PR removing that line would re-do the scripted-diff to change StdLockGuard to STDLOCK(). If this is merged first, and 34806 doesn’t have ACKs, I’d probably rebase it, and it could be merged in parallel with a PR removing that line. If 34806 is merged first, then I’d probably rebase this to update the scripted-diff, and, if all the other PRs using StdLockGuard are also rebased (or unmergable due to conflicts) would update this to also remove the code.
In general, I don’t like people delaying because of potential merge conflicts.
What are you doing now if not delaying?
What are you doing now if not delaying?
I’m trying to figure out why you are leaving a TODO and how I should judge these things in the future. I don’t think introducing a TODO because another PR of yours has a trivial conflict is a good thing. Besides I’d also like people to avoid TODO comments, because the LLM bots pick up on them and give us more work to deal with.
If this is merged first, and 34806 doesn’t have ACKs, I’d probably rebase it, and it could be merged in parallel with a PR removing that line.
Or we just do it here? I don’t like one-off follow ups. They command more time from reviewers too.
What are you doing now if not delaying?
I’m trying to figure out why you are leaving a TODO and how I should judge these things in the future. I don’t think introducing a TODO because another PR of yours has a trivial conflict is a good thing. Besides I’d also like people to avoid TODO comments, because the LLM bots pick up on them and give us more work to deal with.
If this is merged first, and 34806 doesn’t have ACKs, I’d probably rebase it, and it could be merged in parallel with a PR removing that line.
Or we just do it here? I don’t like one-off follow ups. They command more time from reviewers too.
I had these two sets of PRs sequenced in #34038, and was specifically asked to split them up. Now you’re complaining in the split up PR that they’re not sequenced, even though there is no need for them to be sequenced and doing so adds work, and artificially delays whichever PR is sequenced second, after this PR has already been arbitrary delayed for sequencing. Having this argument wastes time and energy as well, and I would expect substantially more than reviewing a one-line “delete deprecated declaration” PR.
ACK 8d2f06853acabdff0b5a1a99531f2b432daf3883
Having this argument wastes time and energy as well.
Agreed, thanks for elaborating.