diff options
-rw-r--r-- | src/sync.cpp | 54 | ||||
-rw-r--r-- | src/sync.h | 14 | ||||
-rw-r--r-- | src/test/sync_tests.cpp | 55 |
3 files changed, 112 insertions, 11 deletions
diff --git a/src/sync.cpp b/src/sync.cpp index 322198a852..2e431720e6 100644 --- a/src/sync.cpp +++ b/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) { diff --git a/src/sync.h b/src/sync.h index 41f4e43bdd..0948083c7f 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(); @@ -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(); \ } diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp index 19029ebd3c..6c14867211 100644 --- a/src/test/sync_tests.cpp +++ b/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() |