Strengthen thread safety assertions #24931

ajtowns · 2022-04-20T09:25:52Z

This changes LOCK(mutex) for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.

This can't reasonably be used for globals, because clang would require every function to be annotated with EXCLUSIVE_LOCKS_REQUIRED(!g_mutex) for each global mutex; so this introduces a trivial GlobalMutex subclass of Mutex, and reduces the annotations for both GlobalMutex to LOCKS_EXCLUDED which only catches trivial errors (eg (LOCK(x); LOCK(x);).

ajtowns · 2022-04-20T09:30:22Z

This hopefully avoids bugs like #24157 (comment) and gets most of the benefits of -Wthread-safety-negative without introducing massive amounts of unhelpful warnings about cs_main and the like as per #20272. See also #21598.

jonatack · 2022-04-20T09:37:12Z

Concept ACK

DrahtBot · 2022-04-20T15:03:03Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)

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.

jamesob · 2022-04-20T15:24:05Z

Concept ACK

hebasto · 2022-04-20T15:44:23Z

Concept ACK. I had a similar perspective view since f8213c0 :)

Maybe split off changes related to GlobalMutex as they looks non-controversial and pretty straightforward to review?

jonatack

Is the goal of this change to allow building with -Wthread-safety-negative? Still seeing many warnings on this branch with ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' && make clean && make. A regular debug build on this branch is clean, however.

src/checkqueue.h

src/sync.h

Could be verified with $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' $ make clean $ make 2>&1 | grep m_tx_relay_mutex

hebasto · 2022-05-20T11:40:39Z

Looks like this PR has a silent merge conflict because of #22778 and #24062.

See #25175.

hebasto · 2022-05-20T11:44:47Z

This changes LOCK(mutex) for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.

How about WITH_LOCK macro?

ajtowns · 2022-05-20T14:24:39Z

Rebased onto #25175. The WITH_LOCK macro is based on LOCK, so also enforces these checks. LOCK2, TRY_LOCK and WAIT_LOCK are also updated.

hebasto · 2022-05-20T14:30:33Z

Rebased onto #25175. The WITH_LOCK macro is based on LOCK, so also enforces these checks. LOCK2, TRY_LOCK and WAIT_LOCK are also updated.

bitcoin/src/net_processing.cpp

Lines 294 to 297 in cd6ae69

    
           TxRelay* GetTxRelay() 
        
           { 
        
               return WITH_LOCK(m_tx_relay_mutex, return m_tx_relay.get()); 
        
           };

does not emit a warning about missed EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) annotation.

ajtowns · 2022-05-20T14:31:15Z

Though, on the other hand, WITH_LOCK is within a lambda (in order to be able to return the result of the expression) and the annotations aren't propogated to the lambda. I think this is probably okay: RecursiveMutex and GlobalMutex don't require propogation anyway, and Mutex are all within a class so the new lambda will be implicitly tagged with the negative capability anyway?

However, tagging the WITH_LOCK lambda with LOCKS_EXCLUDED(cs) could be done, just as a double check that it's not being used when the lock is already held?

-BEGIN VERIFY SCRIPT- sed -i -E -e '/^([a-z]+ )?Mutex [a-z]/ s/Mutex/GlobalMutex/' $(git grep -lE '^([a-z]+ )?Mutex [a-z]') -END VERIFY SCRIPT-

ajtowns · 2022-05-20T15:37:16Z

bitcoin/src/net_processing.cpp

Lines 294 to 297 in cd6ae69

TxRelay* GetTxRelay()

{

return WITH_LOCK(m_tx_relay_mutex, return m_tx_relay.get());

};

does not emit a warning about missed EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) annotation.

Updated with negative assertions propogated when WITH_LOCK is used, which caught the above case and another. Couldn't see a way to avoid having WITH_LOCK expand cs twice though.

… by propagating some negative capabilities 2b3373c refactor: Propagate negative `!m_tx_relay_mutex` capability (Hennadii Stepanov) 5a6e3c1 refactor: Propagate negative `!m_most_recent_block_mutex` capability (Hennadii Stepanov) Pull request description: This PR is a follow-up for bitcoin/bitcoin#22778 and bitcoin/bitcoin#24062, and it seems [required](bitcoin/bitcoin#24931 (comment)) for bitcoin/bitcoin#24931. See details in the commit messages. ACKs for top commit: ajtowns: ACK 2b3373c w0xlt: ACK bitcoin/bitcoin@2b3373c Tree-SHA512: 8a4bb9641af8d79824ec12e2d6bfce0e09524094b298a2edcdb2ab115fbaa21215b9c97a6b059f8aa023551071fd5798eca66ab8d262a3f97246a91d960850d0

hebasto

ACK ce893c0

hebasto · 2022-05-20T18:20:17Z

src/sync.h

+ *
+ * See: https://github.com/bitcoin/bitcoin/pull/20272#issuecomment-720755781
+ */
+class GlobalMutex : public Mutex { };


a559509

nit, clang-format-diff.py complains:

Suggested change

class GlobalMutex : public Mutex { };

class GlobalMutex : public Mutex

{

};

vasild · 2022-06-02T07:51:33Z

src/sync.h

-#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
+#define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }())


While playing with this I observed that it is possible to circumvent the thread safety warnings mechanism by wrapping the lock call in a lambda and calling this lambda immediately. That circumvention only works for mutexes that are a member of a class:

Mutex mutex_global; // Commented annotations would silence the warnings, clang 15 class C { public: Mutex mutex_member; void f1() // EXCLUSIVE_LOCKS_REQUIRED(!mutex_member) { // warning: calling function 'MaybeCheckNotHeld' requires negative capability '!mutex_member' [-Wthread-safety-analysis] LOCK(mutex_member); } void f2() { [&]() { LOCK(mutex_member); }(); // no need for annotations } void f3() // EXCLUSIVE_LOCKS_REQUIRED(!mutex_global) { // warning: calling function 'MaybeCheckNotHeld' requires negative capability '!mutex_global' [-Wthread-safety-analysis] LOCK(mutex_global); } void f4() // EXCLUSIVE_LOCKS_REQUIRED(!mutex_global) { // warning: calling function 'MaybeCheckNotHeld' requires negative capability '!mutex_global' [-Wthread-safety-analysis] [&]() /* EXCLUSIVE_LOCKS_REQUIRED(!mutex_global) */ { LOCK(mutex_global); }(); } };

Looks like the construct used for WITH_LOCK() in this PR relies on the f2() behavior from the example above. Is that behavior documented somewhere in the clang docs? If not, then it may be wiser to not depend on undocumented behavior.

You can avoid the thread safety warnings in a lot of ways if you create aliases for either the locks or the variables being guarded; eg:

int i GUARDED_BY(mutex_member) = 0; void f5() { int* i_alias = &i; *i_alias = 5; }

(Using [&] here is creating an implicit alias for all the member variables, though clang does a better job of analysing past the aliases there than in the above example...)

I think the best long term approach is to do away with any uses of both GlobalMutex and RecursiveMutex, at which point we can annotate the lambda in WITH_LOCK with EXCLUSIVE_LOCKS_REQUIRED(!cs) and we're done. Having it work in the meantime allows us to be more confident that we don't have any potential recursive cases when switching a RecursiveMutex to a regular Mutex, so seems like it aids that goal to me.

Ok, this explains why f2() needs no annotations. I guess at some point the compiler may become smarter, detect the reference and issue a warning.

I think the best long term approach is to do away with any uses of both GlobalMutex and RecursiveMutex, at which point we can annotate the lambda in WITH_LOCK with EXCLUSIVE_LOCKS_REQUIRED(!cs) and we're done.

I agree. Do you envision that we do not have mutexes in global scope, or we have ones that are of type Mutex?

Anytime you currently have:

GlobalMutex g_mut; int g_importantnum GUARDED_BY(g_mut) = 0; std::string g_importantstr GUARDED_BY(g_mut); void bump_importantint(int offset) { LOCK(g_mut); g_importantnum += offset; }

you can rewrite it as

class important_stuff { private: Mutex m_mut; int m_num GUARDED_BY(m_mut) = 0; std::string m_str GUARDED_BY(m_mut); public: void bump(int offset) EXCLUSIVE_LOCKS_REQUIRED(!m_mut); }; important_stuff g_important;

or similar (depending on whether you can move all the code that uses the mutex into the class, or if you have to expose the mutex), which doesn't require very large changes to the code, but does make for a better fit with the thread safety analysis. Might also make it easy to later change g_important to not need to be a global too.

Do you envision that we do not have mutexes in global scope, or we have ones that are of type Mutex

I guess your reply above favors the "we do not have mutexes in global scope" case, right?

g_important.m_mut is still technically in global scope. Just works better with clang's thread safety logic

maflcko · 2022-06-10T14:29:07Z

Maybe drop the merged commits from the broken GitHub web view?

git commit --allow-empty -m empty
git push ...
git reset --hard HEAD~
git push ... --force

maflcko · 2022-06-10T14:42:13Z

Nvm, the merge commit will have the correct commit list.

review ACK ce893c0 🐦

Show signature

Signature:

ajtowns mentioned this pull request Apr 20, 2022

p2p: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it #24157

Merged

ajtowns force-pushed the 202204-negative-annotations-assertnotheld-and-lock branch from 8f76596 to b6914e6 Compare April 20, 2022 10:30

DrahtBot added Consensus GUI P2P RPC/REST/ZMQ Utils/log/libs UTXO Db and Indexes Validation Wallet labels Apr 20, 2022

maflcko added Refactoring and removed GUI Wallet UTXO Db and Indexes RPC/REST/ZMQ P2P Validation Consensus Utils/log/libs labels Apr 20, 2022

jonatack reviewed Apr 20, 2022

View reviewed changes

src/checkqueue.h Outdated Show resolved Hide resolved

src/sync.h Show resolved Hide resolved

refactor: Propagate negative !m_tx_relay_mutex capability

2b3373c

Could be verified with $ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative' $ make clean $ make 2>&1 | grep m_tx_relay_mutex

hebasto mentioned this pull request May 20, 2022

refactor: Improve thread safety analysis by propagating some negative capabilities #25175

Merged

ajtowns force-pushed the 202204-negative-annotations-assertnotheld-and-lock branch from 3cc9538 to cd6ae69 Compare May 20, 2022 14:22

ajtowns added 6 commits May 21, 2022 01:22

net_processing: thread safety annotation for m_tx_relay_mutex

f24bd45

qt/clientmodel: thread safety annotation for m_cached_tip_mutex

be6aa72

sync.h: Add GlobalMutex type

a559509

scripted-diff: Convert global Mutexes to GlobalMutexes

bba87c0

-BEGIN VERIFY SCRIPT- sed -i -E -e '/^([a-z]+ )?Mutex [a-z]/ s/Mutex/GlobalMutex/' $(git grep -lE '^([a-z]+ )?Mutex [a-z]') -END VERIFY SCRIPT-

sync.h: Imply negative assertions when calling LOCK

d285291

doc: Update developer notes

ce893c0

ajtowns force-pushed the 202204-negative-annotations-assertnotheld-and-lock branch from cd6ae69 to ce893c0 Compare May 20, 2022 15:28

hebasto approved these changes May 20, 2022

View reviewed changes

ajtowns requested review from jonatack and jamesob June 1, 2022 01:26

vasild reviewed Jun 2, 2022

View reviewed changes

vasild mentioned this pull request Jun 7, 2022

refactor: Remove negative lock annotations from globals #21598

Merged

DrahtBot mentioned this pull request Jun 8, 2022

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

maflcko merged commit 8f3ab9a into bitcoin:master Jun 10, 2022

maflcko mentioned this pull request Jun 10, 2022

p2p: Prevent block index fingerprinting by sending additional getheaders messages #24571

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 13, 2022

Merge bitcoin#24931: Strengthen thread safety assertions

d8f5a94

vasild mentioned this pull request Jun 29, 2022

sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es #25390

Closed

bitcoin locked and limited conversation to collaborators Jun 10, 2023

		#define WITH_LOCK(cs, code) [&]() -> decltype(auto) { LOCK(cs); code; }()
		#define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }())

Strengthen thread safety assertions #24931

Strengthen thread safety assertions #24931

Uh oh!

Conversation

ajtowns commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Apr 20, 2022

Uh oh!

jonatack commented Apr 20, 2022

Uh oh!

DrahtBot commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jamesob commented Apr 20, 2022

Uh oh!

hebasto commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 20, 2022

Uh oh!

hebasto commented May 20, 2022

Uh oh!

ajtowns commented May 20, 2022

Uh oh!

hebasto commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented May 20, 2022

Uh oh!

ajtowns commented May 20, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

ajtowns Jun 2, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

ajtowns Jun 6, 2022

Choose a reason for hiding this comment

Uh oh!

vasild Jun 7, 2022

Choose a reason for hiding this comment

Uh oh!

ajtowns Jun 8, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 10, 2022

Uh oh!

maflcko commented Jun 10, 2022

Uh oh!

Uh oh!

ajtowns commented Apr 20, 2022 •

edited

Loading

DrahtBot commented Apr 20, 2022 •

edited

Loading

hebasto commented Apr 20, 2022 •

edited

Loading

hebasto commented May 20, 2022 •

edited

Loading

hebasto May 20, 2022 •

edited

Loading