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
This commit is contained in:
Wladimir J. van der Laan 2020-11-25 17:01:57 +01:00 committed by PastaPastaPasta
parent c1b5ec6360
commit 8fee13ef70
3 changed files with 111 additions and 11 deletions

View File

@ -13,11 +13,14 @@
#include <util/strencodings.h> #include <util/strencodings.h>
#include <util/threadnames.h> #include <util/threadnames.h>
#include <boost/thread/mutex.hpp>
#include <map> #include <map>
#include <mutex>
#include <set> #include <set>
#include <system_error> #include <system_error>
#include <thread> #include <thread>
#include <type_traits>
#include <unordered_map> #include <unordered_map>
#include <utility> #include <utility>
#include <vector> #include <vector>
@ -139,16 +142,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)); 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(); LockData& lockdata = GetLockData();
std::lock_guard<std::mutex> lock(lockdata.dd_mutex); std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()]; LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
lock_stack.emplace_back(c, locklocation); lock_stack.emplace_back(c, locklocation);
for (const LockStackItem& i : lock_stack) { for (size_t j = 0; j < lock_stack.size() - 1; ++j) {
if (i.first == c) const LockStackItem& i = lock_stack[j];
break; 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); const LockPair p1 = std::make_pair(i.first, c);
if (lockdata.lockorders.count(p1)) if (lockdata.lockorders.count(p1))
@ -179,10 +216,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())); 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) void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line)
{ {

View File

@ -48,7 +48,8 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
/////////////////////////////// ///////////////////////////////
#ifdef DEBUG_LOCKORDER #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 LeaveCritical();
void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line); void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
std::string LocksHeld(); std::string LocksHeld();
@ -65,7 +66,8 @@ bool LockStackEmpty();
*/ */
extern bool g_debug_lockorder_abort; extern bool g_debug_lockorder_abort;
#else #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 LeaveCritical() {}
inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {} inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
template <typename MutexType> template <typename MutexType>
@ -133,7 +135,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base
private: private:
void Enter(const char* pszName, const char* pszFile, int nLine) 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 #ifdef DEBUG_LOCKCONTENTION
if (!Base::try_lock()) { if (!Base::try_lock()) {
PrintLockContention(pszName, pszFile, nLine); PrintLockContention(pszName, pszFile, nLine);
@ -146,7 +148,7 @@ private:
bool TryEnter(const char* pszName, const char* pszFile, int nLine) 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(); Base::try_lock();
if (!Base::owns_lock()) if (!Base::owns_lock())
LeaveCritical(); LeaveCritical();
@ -203,7 +205,7 @@ public:
~reverse_lock() { ~reverse_lock() {
templock.swap(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(); lock.lock();
} }
@ -234,7 +236,7 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
#define ENTER_CRITICAL_SECTION(cs) \ #define ENTER_CRITICAL_SECTION(cs) \
{ \ { \
EnterCritical(#cs, __FILE__, __LINE__, (void*)(&cs)); \ EnterCritical(#cs, __FILE__, __LINE__, &cs); \
(cs).lock(); \ (cs).lock(); \
} }

View File

@ -6,6 +6,9 @@
#include <test/util/setup_common.h> #include <test/util/setup_common.h>
#include <boost/test/unit_test.hpp> #include <boost/test/unit_test.hpp>
#include <boost/thread/mutex.hpp>
#include <mutex>
namespace { namespace {
template <typename MutexType> template <typename MutexType>
@ -42,6 +45,38 @@ void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_
LEAVE_CRITICAL_SECTION(mutex1); LEAVE_CRITICAL_SECTION(mutex1);
BOOST_CHECK(LockStackEmpty()); BOOST_CHECK(LockStackEmpty());
} }
#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 } // namespace
BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
@ -92,4 +127,24 @@ BOOST_AUTO_TEST_CASE(inconsistent_lock_order_detected)
#endif // DEBUG_LOCKORDER #endif // DEBUG_LOCKORDER
} }
/* 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() BOOST_AUTO_TEST_SUITE_END()