From 458992b06d80eb568141f60a33d38e12e894e27a Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 5 May 2020 08:07:47 +0300 Subject: [PATCH 1/6] Prevent UB in DeleteLock() function --- src/sync.cpp | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index b86c57e4981..afb5ad51b18 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -78,21 +78,16 @@ typedef std::map, LockStack> 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 RecursiveMutex destructors - // after LockData disappears. - bool available; - LockData() : available(true) {} - ~LockData() { available = false; } - LockOrders lockorders; InvLockOrders invlockorders; std::mutex dd_mutex; }; + LockData& GetLockData() { - static LockData lockdata; - return lockdata; + // This approach guarantees that the object is not destroyed until after its last use. + // The operating system automatically reclaims all the memory in a program's heap when that program exits. + static LockData& lock_data = *new LockData(); + return lock_data; } static thread_local LockStack g_lockstack; @@ -207,10 +202,6 @@ void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLi void DeleteLock(void* cs) { LockData& lockdata = GetLockData(); - if (!lockdata.available) { - // We're already shutting down. - return; - } std::lock_guard lock(lockdata.dd_mutex); std::pair item = std::make_pair(cs, nullptr); LockOrders::iterator it = lockdata.lockorders.lower_bound(item); From 8d8921abd35c3ac1b8ebacb11de8e1bbc7b28d66 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 18 May 2020 09:40:50 +0300 Subject: [PATCH 2/6] refactor: Add LockStackItem type alias --- src/sync.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index afb5ad51b18..35137a4b995 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -7,15 +7,17 @@ #endif #include -#include #include +#include #include #include #include #include #include +#include +#include #ifdef DEBUG_LOCKCONTENTION #if !defined(HAVE_THREAD_LOCAL) @@ -73,7 +75,8 @@ private: int sourceLine; }; -typedef std::vector > LockStack; +using LockStackItem = std::pair; +using LockStack = std::vector; typedef std::map, LockStack> LockOrders; typedef std::set > InvLockOrders; @@ -96,7 +99,7 @@ static void potential_deadlock_detected(const std::pair& mismatch, { LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("Previous lock order was:\n"); - for (const std::pair & i : s2) { + for (const LockStackItem& i : s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); /* Continued */ } @@ -106,7 +109,7 @@ static void potential_deadlock_detected(const std::pair& mismatch, LogPrintf(" %s\n", i.second.ToString()); } LogPrintf("Current lock order is:\n"); - for (const std::pair & i : s1) { + for (const LockStackItem& i : s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); /* Continued */ } @@ -129,7 +132,7 @@ static void push_lock(void* c, const CLockLocation& locklocation) g_lockstack.push_back(std::make_pair(c, locklocation)); - for (const std::pair& i : g_lockstack) { + for (const LockStackItem& i : g_lockstack) { if (i.first == c) break; @@ -175,14 +178,14 @@ void LeaveCritical() std::string LocksHeld() { std::string result; - for (const std::pair& i : g_lockstack) + for (const LockStackItem& i : g_lockstack) result += i.second.ToString() + std::string("\n"); return result; } void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) { - for (const std::pair& i : g_lockstack) + for (const LockStackItem& i : g_lockstack) if (i.first == cs) return; tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); @@ -191,7 +194,7 @@ void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) { - for (const std::pair& i : g_lockstack) { + for (const LockStackItem& i : g_lockstack) { if (i.first == cs) { tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); abort(); From f511f61dda4e860079153d5e51d64658cc265283 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 18 May 2020 10:00:08 +0300 Subject: [PATCH 3/6] refactor: Add LockPair type alias --- src/sync.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index 35137a4b995..cfbbb78c6b2 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -77,8 +77,10 @@ private: using LockStackItem = std::pair; using LockStack = std::vector; -typedef std::map, LockStack> LockOrders; -typedef std::set > InvLockOrders; + +using LockPair = std::pair; +using LockOrders = std::map; +using InvLockOrders = std::set; struct LockData { LockOrders lockorders; @@ -95,7 +97,7 @@ LockData& GetLockData() { static thread_local LockStack g_lockstack; -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"); @@ -136,12 +138,12 @@ static void push_lock(void* c, const CLockLocation& locklocation) 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.emplace(p1, g_lockstack); - 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]); @@ -206,16 +208,16 @@ void DeleteLock(void* cs) { LockData& lockdata = GetLockData(); std::lock_guard lock(lockdata.dd_mutex); - std::pair item = std::make_pair(cs, nullptr); + const LockPair item = std::make_pair(cs, nullptr); 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 58e6881bc5be002e8ddbc9b75422c0deae66a2df Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 18 May 2020 17:45:46 +0300 Subject: [PATCH 4/6] refactor: Refactor duplicated code into LockHeld() --- src/sync.cpp | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index cfbbb78c6b2..5817e22c86e 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -185,23 +185,27 @@ std::string LocksHeld() return result; } +static bool LockHeld(void* mutex) +{ + for (const LockStackItem& i : g_lockstack) { + if (i.first == mutex) return true; + } + + return false; +} + void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) { - for (const LockStackItem& i : g_lockstack) - if (i.first == cs) - return; + if (LockHeld(cs)) return; tfm::format(std::cerr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); abort(); } void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) { - for (const LockStackItem& i : g_lockstack) { - if (i.first == cs) { - tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); - abort(); - } - } + if (!LockHeld(cs)) return; + tfm::format(std::cerr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld()); + abort(); } void DeleteLock(void* cs) From 26c093a9957756f3743c2347fe0abd90f81159c4 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 18 May 2020 18:22:26 +0300 Subject: [PATCH 5/6] Replace thread_local g_lockstack with a mutex-protected map This change prevents UB in case of early g_lockstack destroying. Co-authored-by: Wladimir J. van der Laan --- src/sync.cpp | 51 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index 5817e22c86e..9b0878bbea2 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -16,6 +16,8 @@ #include #include #include +#include +#include #include #include @@ -77,12 +79,14 @@ private: using LockStackItem = std::pair; using LockStack = std::vector; +using LockStacks = std::unordered_map; using LockPair = std::pair; using LockOrders = std::map; using InvLockOrders = std::set; struct LockData { + LockStacks m_lock_stacks; LockOrders lockorders; InvLockOrders invlockorders; std::mutex dd_mutex; @@ -95,8 +99,6 @@ LockData& GetLockData() { return lock_data; } -static thread_local LockStack g_lockstack; - static void potential_deadlock_detected(const LockPair& mismatch, const LockStack& s1, const LockStack& s2) { LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); @@ -132,16 +134,16 @@ static void push_lock(void* c, const CLockLocation& locklocation) LockData& lockdata = GetLockData(); std::lock_guard lock(lockdata.dd_mutex); - g_lockstack.push_back(std::make_pair(c, locklocation)); - - for (const LockStackItem& i : g_lockstack) { + 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; const LockPair p1 = std::make_pair(i.first, c); if (lockdata.lockorders.count(p1)) continue; - lockdata.lockorders.emplace(p1, g_lockstack); + lockdata.lockorders.emplace(p1, lock_stack); const LockPair p2 = std::make_pair(c, i.first); lockdata.invlockorders.insert(p2); @@ -152,7 +154,14 @@ static void push_lock(void* c, const CLockLocation& locklocation) static void pop_lock() { - g_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) @@ -162,11 +171,17 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) { - if (!g_lockstack.empty()) { - const auto& lastlock = g_lockstack.back(); - if (lastlock.first == cs) { - lockname = lastlock.second.Name(); - return; + { + LockData& lockdata = GetLockData(); + std::lock_guard lock(lockdata.dd_mutex); + + const LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; + if (!lock_stack.empty()) { + const auto& lastlock = lock_stack.back(); + if (lastlock.first == cs) { + lockname = lastlock.second.Name(); + return; + } } } throw std::system_error(EPERM, std::generic_category(), strprintf("%s:%s %s was not most recent critical section locked", file, line, guardname)); @@ -179,15 +194,23 @@ 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; - for (const LockStackItem& i : g_lockstack) + for (const LockStackItem& i : lock_stack) result += i.second.ToString() + std::string("\n"); return result; } static bool LockHeld(void* mutex) { - for (const LockStackItem& i : g_lockstack) { + 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; } From 90eb027204f5a9d7c00fa97d4112243bd37a9012 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Fri, 22 May 2020 15:45:21 +0300 Subject: [PATCH 6/6] doc: Add and fix comments about never destroyed objects --- src/logging.cpp | 4 ++-- src/sync.cpp | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index eb9da06d9ba..56c44ae1ea4 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -22,8 +22,8 @@ BCLog::Logger& LogInstance() * access the logger. When the shutdown sequence is fully audited and tested, * explicit destruction of these objects can be implemented by changing this * from a raw pointer to a std::unique_ptr. - * Since the destructor is never called, the logger and all its members must - * have a trivial destructor. + * Since the ~Logger() destructor is never called, the Logger class and all + * its subclasses must have implicitly-defined destructors. * * This method of initialization was originally introduced in * ee3374234c60aba2cc4c5cd5cac1c0aefc2d817c. diff --git a/src/sync.cpp b/src/sync.cpp index 9b0878bbea2..c3312b5a00e 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -95,6 +95,8 @@ struct LockData { LockData& GetLockData() { // This approach guarantees that the object is not destroyed until after its last use. // The operating system automatically reclaims all the memory in a program's heap when that program exits. + // Since the ~LockData() destructor is never called, the LockData class and all + // its subclasses must have implicitly-defined destructors. static LockData& lock_data = *new LockData(); return lock_data; }