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);