diff --git a/src/sync.cpp b/src/sync.cpp index 7de8439d6f..d020b4e334 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -138,17 +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 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 static void push_lock(MutexType* c, const CLockLocation& locklocation) { + constexpr bool is_recursive_mutex = + std::is_base_of::value || + std::is_base_of::value; + LockData& lockdata = GetLockData(); std::lock_guard 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)) 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 #include +#include + +#include namespace { template @@ -29,6 +32,38 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2) BOOST_CHECK(!error_thrown); #endif } + +#ifdef DEBUG_LOCKORDER +template +void TestDoubleLock2(MutexType& m) +{ + ENTER_CRITICAL_SECTION(m); + LEAVE_CRITICAL_SECTION(m); +} + +template +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(true /* should throw */); +} + +BOOST_AUTO_TEST_CASE(double_lock_boost_mutex) +{ + TestDoubleLock(true /* should throw */); +} + +BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex) +{ + TestDoubleLock(false /* should not throw */); +} +#endif /* DEBUG_LOCKORDER */ + BOOST_AUTO_TEST_SUITE_END()