Merge #19337: sync: detect double lock from the same thread

95975dd08d sync: detect double lock from the same thread (Vasil Dimov)
4df6567e4c sync: make EnterCritical() & push_lock() type safe (Vasil Dimov)

Pull request description:

  Double lock of the same (non-recursive) mutex from the same thread would produce an undefined behavior. Detect this from `DEBUG_LOCKORDER` and react similarly to the deadlock detection.

  This came up during discussion in another, related PR: https://github.com/bitcoin/bitcoin/pull/19238#discussion_r442394521.

ACKs for top commit:
  laanwj:
    code review ACK 95975dd08d
  hebasto:
    re-ACK 95975dd08d

Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e

src/sync.cpp

@@ -13,10 +13,14 @@
 #include <util/strencodings.h>
 #include <util/threadnames.h>
 
+#include <boost/thread/mutex.hpp>
+
 #include <map>
+#include <mutex>
 #include <set>
 #include <system_error>
 #include <thread>
+#include <type_traits>
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -135,16 +139,50 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac
     throw std::logic_error(strprintf("potential deadlock detected: %s -> %s -> %s", mutex_b, mutex_a, mutex_b));
 }
 
-static void push_lock(void* c, const CLockLocation& locklocation)
+static void double_lock_detected(const void* mutex, LockStack& lock_stack)
 {
+    LogPrintf("DOUBLE LOCK DETECTED\n");
+    LogPrintf("Lock order:\n");
+    for (const LockStackItem& i : lock_stack) {
+        if (i.first == mutex) {
+            LogPrintf(" (*)"); /* Continued */
+        }
+        LogPrintf(" %s\n", i.second.ToString());
+    }
+    if (g_debug_lockorder_abort) {
+        tfm::format(std::cerr, "Assertion failed: detected double lock at %s:%i, details in debug log.\n", __FILE__, __LINE__);
+        abort();
+    }
+    throw std::logic_error("double lock detected");
+}
+
+template <typename MutexType>
+static void push_lock(MutexType* c, const CLockLocation& locklocation)
+{
+    constexpr bool is_recursive_mutex =
+        std::is_base_of<RecursiveMutex, MutexType>::value ||
+        std::is_base_of<std::recursive_mutex, MutexType>::value;
+
     LockData& lockdata = GetLockData();
     std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
 
     LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
     lock_stack.emplace_back(c, locklocation);
-    for (const LockStackItem& i : lock_stack) {
-        if (i.first == c)
-            break;
+    for (size_t j = 0; j < lock_stack.size() - 1; ++j) {
+        const LockStackItem& i = lock_stack[j];
+        if (i.first == c) {
+            if (is_recursive_mutex) {
+                break;
+            }
+            // It is not a recursive mutex and it appears in the stack two times:
+            // at position `j` and at the end (which we added just before this loop).
+            // Can't allow locking the same (non-recursive) mutex two times from the
+            // same thread as that results in an undefined behavior.
+            auto lock_stack_copy = lock_stack;
+            lock_stack.pop_back();
+            double_lock_detected(c, lock_stack_copy);
+            // double_lock_detected() does not return.
+        }
 
         const LockPair p1 = std::make_pair(i.first, c);
         if (lockdata.lockorders.count(p1))
@@ -175,10 +213,16 @@ static void pop_lock()
     }
 }
 
-void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry)
+template <typename MutexType>
+void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry)
 {
     push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry, util::ThreadGetInternalName()));
 }
+template void EnterCritical(const char*, const char*, int, Mutex*, bool);
+template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool);
+template void EnterCritical(const char*, const char*, int, std::mutex*, bool);
+template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool);
+template void EnterCritical(const char*, const char*, int, boost::mutex*, bool);
 
 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line)
 {

src/sync.h

@@ -48,7 +48,8 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
 ///////////////////////////////
 
 #ifdef DEBUG_LOCKORDER
-void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
+template <typename MutexType>
+void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false);
 void LeaveCritical();
 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
 std::string LocksHeld();
@@ -66,7 +67,8 @@ bool LockStackEmpty();
  */
 extern bool g_debug_lockorder_abort;
 #else
-inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
+template <typename MutexType>
+inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false) {}
 inline void LeaveCritical() {}
 inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
 template <typename MutexType>
@@ -135,7 +137,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base
 private:
     void Enter(const char* pszName, const char* pszFile, int nLine)
     {
-        EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()));
+        EnterCritical(pszName, pszFile, nLine, Base::mutex());
 #ifdef DEBUG_LOCKCONTENTION
         if (!Base::try_lock()) {
             PrintLockContention(pszName, pszFile, nLine);
@@ -148,7 +150,7 @@ private:
 
     bool TryEnter(const char* pszName, const char* pszFile, int nLine)
     {
-        EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true);
+        EnterCritical(pszName, pszFile, nLine, Base::mutex(), true);
         Base::try_lock();
         if (!Base::owns_lock())
             LeaveCritical();
@@ -205,7 +207,7 @@ public:
 
         ~reverse_lock() {
             templock.swap(lock);
-            EnterCritical(lockname.c_str(), file.c_str(), line, (void*)lock.mutex());
+            EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex());
             lock.lock();
         }
 
@@ -236,7 +238,7 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
 
 #define ENTER_CRITICAL_SECTION(cs)                            \
     {                                                         \
-        EnterCritical(#cs, __FILE__, __LINE__, (void*)(&cs)); \
+        EnterCritical(#cs, __FILE__, __LINE__, &cs); \
         (cs).lock();                                          \
     }
 

src/test/sync_tests.cpp

@@ -6,6 +6,9 @@
 #include <test/util/setup_common.h>
 
 #include <boost/test/unit_test.hpp>
+#include <boost/thread/mutex.hpp>
+
+#include <mutex>
 
 namespace {
 template <typename MutexType>
@@ -29,6 +32,38 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
     BOOST_CHECK(!error_thrown);
     #endif
 }
+
+#ifdef DEBUG_LOCKORDER
+template <typename MutexType>
+void TestDoubleLock2(MutexType& m)
+{
+    ENTER_CRITICAL_SECTION(m);
+    LEAVE_CRITICAL_SECTION(m);
+}
+
+template <typename MutexType>
+void TestDoubleLock(bool should_throw)
+{
+    const bool prev = g_debug_lockorder_abort;
+    g_debug_lockorder_abort = false;
+
+    MutexType m;
+    ENTER_CRITICAL_SECTION(m);
+    if (should_throw) {
+        BOOST_CHECK_EXCEPTION(
+            TestDoubleLock2(m), std::logic_error, [](const std::logic_error& e) {
+                return strcmp(e.what(), "double lock detected") == 0;
+            });
+    } else {
+        BOOST_CHECK_NO_THROW(TestDoubleLock2(m));
+    }
+    LEAVE_CRITICAL_SECTION(m);
+
+    BOOST_CHECK(LockStackEmpty());
+
+    g_debug_lockorder_abort = prev;
+}
+#endif /* DEBUG_LOCKORDER */
 } // namespace
 
 BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
@@ -55,4 +90,24 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
     #endif
 }
 
+/* Double lock would produce an undefined behavior. Thus, we only do that if
+ * DEBUG_LOCKORDER is activated to detect it. We don't want non-DEBUG_LOCKORDER
+ * build to produce tests that exhibit known undefined behavior. */
+#ifdef DEBUG_LOCKORDER
+BOOST_AUTO_TEST_CASE(double_lock_mutex)
+{
+    TestDoubleLock<Mutex>(true /* should throw */);
+}
+
+BOOST_AUTO_TEST_CASE(double_lock_boost_mutex)
+{
+    TestDoubleLock<boost::mutex>(true /* should throw */);
+}
+
+BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex)
+{
+    TestDoubleLock<RecursiveMutex>(false /* should not throw */);
+}
+#endif /* DEBUG_LOCKORDER */
+
 BOOST_AUTO_TEST_SUITE_END()