From 4df6567e4cbb4677e8048de2f8008612e1b860b9 Mon Sep 17 00:00:00 2001
From: Vasil Dimov <vd@FreeBSD.org>
Date: Fri, 19 Jun 2020 20:51:16 +0200
Subject: [PATCH] sync: make EnterCritical() & push_lock() type safe

The functions `EnterCritical()` and `push_lock()` take a pointer to a
mutex, but that pointer used to be of type `void*` because we use a few
different types for mutexes. This `void*` argument was not type safe
because somebody could have send a pointer to anything that is not a
mutex. Furthermore it wouldn't allow to check whether the passed mutex
is recursive or not.

Thus, change the functions to templated ones so that we can implement
stricter checks for non-recursive mutexes. This also simplifies the
callers of `EnterCritical()`.
---
 src/sync.cpp | 14 ++++++++++++--
 src/sync.h   | 14 ++++++++------
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/src/sync.cpp b/src/sync.cpp
index 4be13a3c48..7de8439d6f 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -13,7 +13,10 @@
 #include <util/strencodings.h>
 #include <util/threadnames.h>
 
+#include <boost/thread/mutex.hpp>
+
 #include <map>
+#include <mutex>
 #include <set>
 #include <system_error>
 #include <thread>
@@ -135,7 +138,8 @@ 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)
+template <typename MutexType>
+static void push_lock(MutexType* c, const CLockLocation& locklocation)
 {
     LockData& lockdata = GetLockData();
     std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
@@ -175,10 +179,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)
 {
diff --git a/src/sync.h b/src/sync.h
index 05ff2ee8a9..ef9ad4c545 100644
--- a/src/sync.h
+++ b/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();
@@ -65,7 +66,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>
@@ -133,7 +135,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);
@@ -146,7 +148,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();
@@ -203,7 +205,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();
         }
 
@@ -234,7 +236,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();                                          \
     }