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

Author: theuni

src/node/interfaces.cpp

@@ -443,7 +443,7 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec
     if (block.m_locator) { *block.m_locator = GetLocator(index); }
     if (block.m_next_block) FillBlock(active[index->nHeight] == 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;

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 (...) {

src/sync.h

@@ -14,6 +14,7 @@
 #include <threadsafety.h> // IWYU pragma: export
 #include <util/macros.h>
 
+#include <cassert>
 #include <condition_variable>
 #include <mutex>
 #include <string>
@@ -212,16 +213,19 @@ class SCOPED_LOCKABLE UniqueLock : public MutexType::unique_lock
     /**
      * 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 @@ class SCOPED_LOCKABLE UniqueLock : public MutexType::unique_lock
      friend class reverse_lock;
 };
 
-#define REVERSE_LOCK(g) typename std::decay<decltype(g)>::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<decltype(g)>::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

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

src/validationinterface.cpp

@@ -83,7 +83,7 @@ class ValidationSignalsImpl
         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);