From 06e4040717ca3b0a1061e961d96f07c290f408e7 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Wed, 9 Aug 2023 13:19:19 -0400 Subject: [PATCH 1/3] refactor: remove DEBUG_LOCKORDER undefined behavior Removes undefined behavior by refactoring the way we destruct objects that track mutexes when debugging. 1. Prevent UB from LockData custom destructor by making it implicit 2. Replace the thread_specific_ptr holding the lock stack with a static map that stores it per thread. Also cleans up the aliasing to make the code better readable. Backported from: 458992b0 8d8921ab f511f61d 58e6881b 26c093a9 Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Co-authored-by: Wladimir J. van der Laan --- src/sync.cpp | 106 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 67 insertions(+), 39 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index fce57f1df..a4518b09c 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -11,6 +11,13 @@ #include #include +#include +#include +#include +#include +#include +#include +#include #ifdef DEBUG_LOCKCONTENTION void PrintLockContention(const char* pszName, const char* pszFile, int nLine) @@ -55,31 +62,31 @@ private: int sourceLine; }; -typedef std::vector > LockStack; -typedef std::map, LockStack> LockOrders; -typedef std::set > InvLockOrders; +typedef std::pair LockStackItem; +typedef std::vector LockStack; +typedef std::unordered_map LockStacks; + +typedef std::pair LockPair; +typedef std::map LockOrders; +typedef std::set InvLockOrders; struct LockData { - // Very ugly hack: as the global constructs and destructors run single - // threaded, we use this boolean to know whether LockData still exists, - // as DeleteLock can get called by global CCriticalSection destructors - // after LockData disappears. - bool available; - LockData() : available(true) {} - ~LockData() { available = false; } - + LockStacks m_lock_stacks; LockOrders lockorders; InvLockOrders invlockorders; - boost::mutex dd_mutex; -} static lockdata; + std::mutex dd_mutex; +}; -boost::thread_specific_ptr lockstack; +LockData& GetLockData() { + static LockData& lock_data = *new LockData(); + return lock_data; +} -static void potential_deadlock_detected(const std::pair& mismatch, const LockStack& s1, const LockStack& s2) +static void potential_deadlock_detected(const LockPair& mismatch, const LockStack& s1, const LockStack& s2) { LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { + BOOST_FOREACH (const LockStackItem& i, s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -89,7 +96,7 @@ static void potential_deadlock_detected(const std::pair& mismatch, LogPrintf(" %s\n", i.second.ToString()); } LogPrintf("Current lock order is:\n"); - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { + BOOST_FOREACH (const LockStackItem& i, s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -103,23 +110,22 @@ static void potential_deadlock_detected(const std::pair& mismatch, static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) { - if (lockstack.get() == NULL) - lockstack.reset(new LockStack); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); - boost::unique_lock lock(lockdata.dd_mutex); + LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + lock_stack.emplace_back(c, locklocation); - (*lockstack).push_back(std::make_pair(c, locklocation)); - - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { + BOOST_FOREACH (const LockStackItem& i, lock_stack) { if (i.first == c) break; - std::pair p1 = std::make_pair(i.first, c); + const LockPair p1 = std::make_pair(i.first, c); if (lockdata.lockorders.count(p1)) continue; - lockdata.lockorders[p1] = (*lockstack); + lockdata.lockorders[p1] = lock_stack; - std::pair p2 = std::make_pair(c, i.first); + const LockPair p2 = std::make_pair(c, i.first); lockdata.invlockorders.insert(p2); if (lockdata.lockorders.count(p2)) potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); @@ -128,7 +134,14 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) static void pop_lock() { - (*lockstack).pop_back(); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + lock_stack.pop_back(); + if (lock_stack.empty()) { + lockdata.m_lock_stacks.erase(std::this_thread::get_id()); + } } void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry) @@ -143,38 +156,53 @@ void LeaveCritical() std::string LocksHeld() { + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + std::string result; - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, *lockstack) + for (const LockStackItem& i : lock_stack) { result += i.second.ToString() + std::string("\n"); + } return result; } +static bool LockHeld(void* mutex) +{ + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + for (const LockStackItem& i : lock_stack) { + if (i.first == mutex) return true; + } + + return false; +} + void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) { - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, *lockstack) - if (i.first == cs) - return; + if (LockHeld(cs)) return; fprintf(stderr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str()); abort(); } void DeleteLock(void* cs) { - if (!lockdata.available) { - // We're already shutting down. - return; - } - boost::unique_lock lock(lockdata.dd_mutex); - std::pair item = std::make_pair(cs, (void*)0); + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockPair item = std::make_pair(cs, (void*)0); LockOrders::iterator it = lockdata.lockorders.lower_bound(item); while (it != lockdata.lockorders.end() && it->first.first == cs) { - std::pair invitem = std::make_pair(it->first.second, it->first.first); + const LockPair invitem = std::make_pair(it->first.second, it->first.first); lockdata.invlockorders.erase(invitem); lockdata.lockorders.erase(it++); } InvLockOrders::iterator invit = lockdata.invlockorders.lower_bound(item); while (invit != lockdata.invlockorders.end() && invit->first == cs) { - std::pair invinvitem = std::make_pair(invit->second, invit->first); + const LockPair invinvitem = std::make_pair(invit->second, invit->first); lockdata.lockorders.erase(invinvitem); lockdata.invlockorders.erase(invit++); } From a9129315f595acf9a76602a8644189ae00ebe712 Mon Sep 17 00:00:00 2001 From: Patrick Lodder Date: Fri, 11 Aug 2023 06:46:28 -0400 Subject: [PATCH 2/3] cleanup: remove unneccesary boost dependencies from sync.cpp Removes boost dependencies in sync.cpp in favor of standard c++ for-loops, improving readability. --- src/sync.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index a4518b09c..9eb2bceb4 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -9,8 +9,6 @@ #include -#include -#include #include #include #include @@ -86,7 +84,7 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac { LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); - BOOST_FOREACH (const LockStackItem& i, s2) { + for (const LockStackItem& i : s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -96,7 +94,7 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac LogPrintf(" %s\n", i.second.ToString()); } LogPrintf("Current lock order is:\n"); - BOOST_FOREACH (const LockStackItem& i, s1) { + for (const LockStackItem& i : s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); } @@ -116,7 +114,7 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; lock_stack.emplace_back(c, locklocation); - BOOST_FOREACH (const LockStackItem& i, lock_stack) { + for (const LockStackItem& i : lock_stack) { if (i.first == c) break; From 11f5dd6ea95b8f4436939e69aa385def82994ce9 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 8 Nov 2017 15:28:35 -0500 Subject: [PATCH 3/3] Add unit test for DEBUG_LOCKORDER code Cherry-picked from: 41b88e933 --- src/Makefile.test.include | 1 + src/sync.cpp | 8 +++++++- src/sync.h | 7 +++++++ src/test/sync_tests.cpp | 41 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/test/sync_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index d0dc3af6f..d83f96163 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -127,6 +127,7 @@ BITCOIN_TESTS =\ test/sigopcount_tests.cpp \ test/skiplist_tests.cpp \ test/streams_tests.cpp \ + test/sync_tests.cpp \ test/test_bitcoin.cpp \ test/test_bitcoin.h \ test/test_random.h \ diff --git a/src/sync.cpp b/src/sync.cpp index 9eb2bceb4..f9e26299b 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -103,7 +103,11 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac } LogPrintf(" %s\n", i.second.ToString()); } - assert(false); + if (g_debug_lockorder_abort) { + fprintf(stderr, "Assertion failed: detected inconsistent lock order at %s:%i, details in debug log.\n", __FILE__, __LINE__); + abort(); + } + throw std::logic_error("potential deadlock detected"); } static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) @@ -206,4 +210,6 @@ void DeleteLock(void* cs) } } +bool g_debug_lockorder_abort = true; + #endif /* DEBUG_LOCKORDER */ diff --git a/src/sync.h b/src/sync.h index 3b29050e0..c79245325 100644 --- a/src/sync.h +++ b/src/sync.h @@ -77,6 +77,13 @@ void LeaveCritical(); std::string LocksHeld(); void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); void DeleteLock(void* cs); + +/** + * Call abort() if a potential lock order deadlock bug is detected, instead of + * just logging information and throwing a logic_error. Defaults to true, and + * set to false in DEBUG_LOCKORDER unit tests. + */ +extern bool g_debug_lockorder_abort; #else void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} void static inline LeaveCritical() {} diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp new file mode 100644 index 000000000..2e0951c3a --- /dev/null +++ b/src/test/sync_tests.cpp @@ -0,0 +1,41 @@ +// Copyright (c) 2012-2017 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +#include + +BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(potential_deadlock_detected) +{ + #ifdef DEBUG_LOCKORDER + bool prev = g_debug_lockorder_abort; + g_debug_lockorder_abort = false; + #endif + + CCriticalSection mutex1, mutex2; + { + LOCK2(mutex1, mutex2); + } + bool error_thrown = false; + try { + LOCK2(mutex2, mutex1); + } catch (const std::logic_error& e) { + BOOST_CHECK_EQUAL(e.what(), "potential deadlock detected"); + error_thrown = true; + } + #ifdef DEBUG_LOCKORDER + BOOST_CHECK(error_thrown); + #else + BOOST_CHECK(!error_thrown); + #endif + + #ifdef DEBUG_LOCKORDER + g_debug_lockorder_abort = prev; + #endif +} + +BOOST_AUTO_TEST_SUITE_END()