From a201a99f8cf5ee5af0cd83cb2e1821e455d6eca7 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Wed, 7 May 2025 16:01:10 +0000 Subject: [PATCH] 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);