diff options
author | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-11-25 17:01:57 +0100 |
---|---|---|
committer | Wladimir J. van der Laan <laanwj@protonmail.com> | 2020-11-25 17:02:20 +0100 |
commit | 50091592dd875a1c94030dbed74112b003732d68 (patch) | |
tree | f78cf8531e2e7dc80b28307609d19165cdc67b88 /src/sync.cpp | |
parent | 19b8071eaeb32dbe39ccc771dbe6095dd9285779 (diff) | |
parent | 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 (diff) | |
download | bitcoin-50091592dd875a1c94030dbed74112b003732d68.tar.xz |
Merge #19337: sync: detect double lock from the same thread
95975dd08d8fdaaeaf28e0d06b861ce2748c17b6 sync: detect double lock from the same thread (Vasil Dimov)
4df6567e4cbb4677e8048de2f8008612e1b860b9 sync: make EnterCritical() & push_lock() type safe (Vasil Dimov)
Pull request description:
Double lock of the same (non-recursive) mutex from the same thread would produce an undefined behavior. Detect this from `DEBUG_LOCKORDER` and react similarly to the deadlock detection.
This came up during discussion in another, related PR: https://github.com/bitcoin/bitcoin/pull/19238#discussion_r442394521.
ACKs for top commit:
laanwj:
code review ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
hebasto:
re-ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e
Diffstat (limited to 'src/sync.cpp')
-rw-r--r-- | src/sync.cpp | 54 |
1 files changed, 49 insertions, 5 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) { |