[skip ci] sync: Check precondition in LEAVE_CRITICAL_SECTION() macro

hebasto · hebasto · commit cb23fe01c125 · 2020-12-10T20:46:39.000+02:00
This change reveals a bug in the wallet_tests/CreateWalletFromFile test,
that will be fixed in the following commit.
diff --git a/src/sync.h b/src/sync.h
@@ -242,10 +242,12 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
         (cs).lock();                                          \
     }
 
-#define LEAVE_CRITICAL_SECTION(cs) \
-    {                              \
-        (cs).unlock();             \
-        LeaveCritical();           \
+#define LEAVE_CRITICAL_SECTION(cs)                                          \
+    {                                                                       \
+        std::string lockname;                                               \
+        CheckLastCritical((void*)(&cs), lockname, #cs, __FILE__, __LINE__); \
+        (cs).unlock();                                                      \
+        LeaveCritical();                                                    \
     }
 
 //! Run code while locking a mutex.
diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp
@@ -62,6 +62,19 @@ void TestDoubleLock(bool should_throw)
     g_debug_lockorder_abort = prev;
 }
 #endif /* DEBUG_LOCKORDER */
+
+template <typename MutexType>
+void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_THREAD_SAFETY_ANALYSIS
+{
+    ENTER_CRITICAL_SECTION(mutex1);
+    ENTER_CRITICAL_SECTION(mutex2);
+#ifdef DEBUG_LOCKORDER
+    BOOST_CHECK_EXCEPTION(LEAVE_CRITICAL_SECTION(mutex1), std::logic_error, HasReason("mutex1 was not most recent critical section locked"));
+#endif // DEBUG_LOCKORDER
+    LEAVE_CRITICAL_SECTION(mutex2);
+    LEAVE_CRITICAL_SECTION(mutex1);
+    BOOST_CHECK(LockStackEmpty());
+}
 } // namespace
 
 BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
@@ -108,4 +121,28 @@ BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex)
 }
 #endif /* DEBUG_LOCKORDER */
 
+BOOST_AUTO_TEST_CASE(inconsistent_lock_order_detected)
+{
+#ifdef DEBUG_LOCKORDER
+    bool prev = g_debug_lockorder_abort;
+    g_debug_lockorder_abort = false;
+#endif // DEBUG_LOCKORDER
+
+    RecursiveMutex rmutex1, rmutex2;
+    TestInconsistentLockOrderDetected(rmutex1, rmutex2);
+    // By checking lock order consistency (CheckLastCritical) before any unlocking (LeaveCritical)
+    // the lock tracking data must not have been broken by exception.
+    TestInconsistentLockOrderDetected(rmutex1, rmutex2);
+
+    Mutex mutex1, mutex2;
+    TestInconsistentLockOrderDetected(mutex1, mutex2);
+    // By checking lock order consistency (CheckLastCritical) before any unlocking (LeaveCritical)
+    // the lock tracking data must not have been broken by exception.
+    TestInconsistentLockOrderDetected(mutex1, mutex2);
+
+#ifdef DEBUG_LOCKORDER
+    g_debug_lockorder_abort = prev;
+#endif // DEBUG_LOCKORDER
+}
+
 BOOST_AUTO_TEST_SUITE_END()
