From 832c57a53410870471a52647ce107454de3bc69c Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 7 May 2025 22:13:27 +0000 Subject: [PATCH 1/3] thread-safety: modernize thread safety macros Clang added new "capability"-based thread-safety attributes years ago, but the old ones remain supported for backwards-compatibility. However, while adding annotations for our reverse_lock, I noticed that there is a difference between the unlock_function and release_capability attributes. unlock_function actually maps to release_generic_capability, which does not work properly when implementing a scoped unlocker. To be consistent, the other capability-based attributes are updated here as well. To avoid having to update our macro usage throughout the codebase, I reused our existing ones. Additionally, SHARED_UNLOCK_FUNCTION is added here, as a subsequent PR will introduce annotations for shared_mutex and shared_lock. --- src/threadsafety.h | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/threadsafety.h b/src/threadsafety.h index 2e9a39bfc95..d897dd2464f 100644 --- a/src/threadsafety.h +++ b/src/threadsafety.h @@ -15,23 +15,24 @@ // See https://clang.llvm.org/docs/ThreadSafetyAnalysis.html // for documentation. The clang compiler can do advanced static analysis // of locking when given the -Wthread-safety option. -#define LOCKABLE __attribute__((lockable)) +#define LOCKABLE __attribute__((capability(""))) #define SCOPED_LOCKABLE __attribute__((scoped_lockable)) #define GUARDED_BY(x) __attribute__((guarded_by(x))) #define PT_GUARDED_BY(x) __attribute__((pt_guarded_by(x))) #define ACQUIRED_AFTER(...) __attribute__((acquired_after(__VA_ARGS__))) #define ACQUIRED_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__))) -#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((exclusive_lock_function(__VA_ARGS__))) -#define SHARED_LOCK_FUNCTION(...) __attribute__((shared_lock_function(__VA_ARGS__))) -#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((exclusive_trylock_function(__VA_ARGS__))) -#define SHARED_TRYLOCK_FUNCTION(...) __attribute__((shared_trylock_function(__VA_ARGS__))) -#define UNLOCK_FUNCTION(...) __attribute__((unlock_function(__VA_ARGS__))) +#define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__((acquire_capability(__VA_ARGS__))) +#define SHARED_LOCK_FUNCTION(...) __attribute__((acquire_shared_capability(__VA_ARGS__))) +#define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_capability(__VA_ARGS__))) +#define SHARED_TRYLOCK_FUNCTION(...) __attribute__((try_acquire_shared_capability(__VA_ARGS__))) +#define UNLOCK_FUNCTION(...) __attribute__((release_capability(__VA_ARGS__))) +#define SHARED_UNLOCK_FUNCTION(...) __attribute__((release_shared_capability(__VA_ARGS__))) #define LOCK_RETURNED(x) __attribute__((lock_returned(x))) #define LOCKS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__))) -#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__))) -#define SHARED_LOCKS_REQUIRED(...) __attribute__((shared_locks_required(__VA_ARGS__))) +#define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((requires_capability(__VA_ARGS__))) +#define SHARED_LOCKS_REQUIRED(...) __attribute__((requires_shared_capability(__VA_ARGS__))) #define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis)) -#define ASSERT_EXCLUSIVE_LOCK(...) __attribute__((assert_exclusive_lock(__VA_ARGS__))) +#define ASSERT_EXCLUSIVE_LOCK(...) __attribute__((assert_capability(__VA_ARGS__))) #else #define LOCKABLE #define SCOPED_LOCKABLE @@ -44,6 +45,7 @@ #define EXCLUSIVE_TRYLOCK_FUNCTION(...) #define SHARED_TRYLOCK_FUNCTION(...) #define UNLOCK_FUNCTION(...) +#define SHARED_UNLOCK_FUNCTION(...) #define LOCK_RETURNED(x) #define LOCKS_EXCLUDED(...) #define EXCLUSIVE_LOCKS_REQUIRED(...) From aeea5f0ec112f9ec29da37806925e961c4998365 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Thu, 8 May 2025 13:30:12 +0000 Subject: [PATCH 2/3] thread-safety: add missing lock annotation No warning is currently emitted because our reverse_lock does not enforce our thread-safety annotations. Once it is fixed, the unlock would cause a warning. --- src/node/interfaces.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 8aec2758f8b..3f9d14c3281 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -431,7 +431,7 @@ public: }; // NOLINTNEXTLINE(misc-no-recursion) -bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock& lock, const CChain& active, const BlockManager& blockman) +bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock& lock, const CChain& active, const BlockManager& blockman) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { if (!index) return false; if (block.m_hash) *block.m_hash = index->GetBlockHash(); From a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 7 May 2025 16:01:10 +0000 Subject: [PATCH 3/3] thread-safety: fix annotations with REVERSE_LOCK Without proper annotations, clang thinks that mutexes are still held for the duration of a reverse_lock. This could lead to subtle bugs as EXCLUSIVE_LOCKS_REQUIRED(foo) passes when it shouldn't. As mentioned in the docs [0], clang's thread-safety analyzer is unable to deal with aliases of mutexes, so it is not possible to use the lock's copy of the mutex for that purpose. Instead, the original mutex needs to be passed back to the reverse_lock for the sake of thread-safety analysis, but it is not actually used otherwise. [0]: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html --- src/node/interfaces.cpp | 2 +- src/scheduler.cpp | 2 +- src/sync.h | 16 ++++++++++++---- src/test/reverselock_tests.cpp | 12 ++++++------ src/validationinterface.cpp | 2 +- 5 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 3f9d14c3281..65a25ad8a1e 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -443,7 +443,7 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLocknHeight] == index ? active[index->nHeight + 1] : nullptr, *block.m_next_block, lock, active, blockman); if (block.m_data) { - REVERSE_LOCK(lock); + REVERSE_LOCK(lock, cs_main); if (!blockman.ReadBlock(*block.m_data, *index)) block.m_data->SetNull(); } block.found = true; diff --git a/src/scheduler.cpp b/src/scheduler.cpp index a1158e0c079..eb174a755ed 100644 --- a/src/scheduler.cpp +++ b/src/scheduler.cpp @@ -56,7 +56,7 @@ void CScheduler::serviceQueue() { // Unlock before calling f, so it can reschedule itself or another task // without deadlocking: - REVERSE_LOCK(lock); + REVERSE_LOCK(lock, newTaskMutex); f(); } } catch (...) { diff --git a/src/sync.h b/src/sync.h index b22956ef1ab..06c88365620 100644 --- a/src/sync.h +++ b/src/sync.h @@ -14,6 +14,7 @@ #include // IWYU pragma: export #include +#include #include #include #include @@ -212,16 +213,19 @@ public: /** * An RAII-style reverse lock. Unlocks on construction and locks on destruction. */ - class reverse_lock { + class SCOPED_LOCKABLE reverse_lock { public: - explicit reverse_lock(UniqueLock& _lock, const char* _guardname, const char* _file, int _line) : lock(_lock), file(_file), line(_line) { + explicit reverse_lock(UniqueLock& _lock, const MutexType& mutex, const char* _guardname, const char* _file, int _line) UNLOCK_FUNCTION(mutex) : lock(_lock), file(_file), line(_line) { + // Ensure that mutex passed back for thread-safety analysis is indeed the original + assert(std::addressof(mutex) == lock.mutex()); + CheckLastCritical((void*)lock.mutex(), lockname, _guardname, _file, _line); lock.unlock(); LeaveCritical(); lock.swap(templock); } - ~reverse_lock() { + ~reverse_lock() UNLOCK_FUNCTION() { templock.swap(lock); EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex()); lock.lock(); @@ -240,7 +244,11 @@ public: friend class reverse_lock; }; -#define REVERSE_LOCK(g) typename std::decay::type::reverse_lock UNIQUE_NAME(revlock)(g, #g, __FILE__, __LINE__) +// clang's thread-safety analyzer is unable to deal with aliases of mutexes, so +// it is not possible to use the lock's copy of the mutex for that purpose. +// Instead, the original mutex needs to be passed back to the reverse_lock for +// the sake of thread-safety analysis, but it is not actually used otherwise. +#define REVERSE_LOCK(g, cs) typename std::decay::type::reverse_lock UNIQUE_NAME(revlock)(g, cs, #g, __FILE__, __LINE__) // When locking a Mutex, require negative capability to ensure the lock // is not already held diff --git a/src/test/reverselock_tests.cpp b/src/test/reverselock_tests.cpp index 0a9ff5f294f..b308306bf3a 100644 --- a/src/test/reverselock_tests.cpp +++ b/src/test/reverselock_tests.cpp @@ -18,7 +18,7 @@ BOOST_AUTO_TEST_CASE(reverselock_basics) BOOST_CHECK(lock.owns_lock()); { - REVERSE_LOCK(lock); + REVERSE_LOCK(lock, mutex); BOOST_CHECK(!lock.owns_lock()); } BOOST_CHECK(lock.owns_lock()); @@ -33,9 +33,9 @@ BOOST_AUTO_TEST_CASE(reverselock_multiple) // Make sure undoing two locks succeeds { - REVERSE_LOCK(lock); + REVERSE_LOCK(lock, mutex); BOOST_CHECK(!lock.owns_lock()); - REVERSE_LOCK(lock2); + REVERSE_LOCK(lock2, mutex2); BOOST_CHECK(!lock2.owns_lock()); } BOOST_CHECK(lock.owns_lock()); @@ -54,7 +54,7 @@ BOOST_AUTO_TEST_CASE(reverselock_errors) g_debug_lockorder_abort = false; // Make sure trying to reverse lock a previous lock fails - BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock2), std::logic_error, HasReason("lock2 was not most recent critical section locked")); + BOOST_CHECK_EXCEPTION(REVERSE_LOCK(lock2, mutex2), std::logic_error, HasReason("lock2 was not most recent critical section locked")); BOOST_CHECK(lock2.owns_lock()); g_debug_lockorder_abort = prev; @@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(reverselock_errors) bool failed = false; try { - REVERSE_LOCK(lock); + REVERSE_LOCK(lock, mutex); } catch(...) { failed = true; } @@ -82,7 +82,7 @@ BOOST_AUTO_TEST_CASE(reverselock_errors) lock.lock(); BOOST_CHECK(lock.owns_lock()); { - REVERSE_LOCK(lock); + REVERSE_LOCK(lock, mutex); BOOST_CHECK(!lock.owns_lock()); } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index da2685d771c..f1ac9305ff2 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -83,7 +83,7 @@ public: for (auto it = m_list.begin(); it != m_list.end();) { ++it->count; { - REVERSE_LOCK(lock); + REVERSE_LOCK(lock, m_mutex); f(*it->callbacks); } it = --it->count ? std::next(it) : m_list.erase(it);