Skip to content

Commit 5009159

Browse files
laanwjlaanwj
committed
Merge bitcoin#19337: sync: detect double lock from the same thread
95975dd sync: detect double lock from the same thread (Vasil Dimov) 4df6567 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: bitcoin#19238 (comment). ACKs for top commit: laanwj: code review ACK 95975dd hebasto: re-ACK 95975dd Tree-SHA512: 375c62db7819e348bfaecc3bd82a7907fcd8f5af24f7d637ac82f3f16789da9fc127dbd0e37158a08e0dcbba01a55c6635caf1d8e9e827cf5a3747f7690a498e
2 parents 19b8071 + 95975dd commit 5009159

File tree

3 files changed

+112
-11
lines changed

3 files changed

+112
-11
lines changed

‎src/sync.cpp‎

Lines changed: 49 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,14 @@
1313
#include <util/strencodings.h>
1414
#include <util/threadnames.h>
1515

16+
#include <boost/thread/mutex.hpp>
17+
1618
#include <map>
19+
#include <mutex>
1720
#include <set>
1821
#include <system_error>
1922
#include <thread>
23+
#include <type_traits>
2024
#include <unordered_map>
2125
#include <utility>
2226
#include <vector>
@@ -135,16 +139,50 @@ static void potential_deadlock_detected(const LockPair& mismatch, const LockStac
135139
throw std::logic_error(strprintf("potential deadlock detected: %s -> %s -> %s", mutex_b, mutex_a, mutex_b));
136140
}
137141

138-
static void push_lock(void* c, const CLockLocation& locklocation)
142+
static void double_lock_detected(const void* mutex, LockStack& lock_stack)
139143
{
144+
LogPrintf("DOUBLE LOCK DETECTED\n");
145+
LogPrintf("Lock order:\n");
146+
for (const LockStackItem& i : lock_stack) {
147+
if (i.first == mutex) {
148+
LogPrintf(" (*)"); /* Continued */
149+
}
150+
LogPrintf(" %s\n", i.second.ToString());
151+
}
152+
if (g_debug_lockorder_abort) {
153+
tfm::format(std::cerr, "Assertion failed: detected double lock at %s:%i, details in debug log.\n", __FILE__, __LINE__);
154+
abort();
155+
}
156+
throw std::logic_error("double lock detected");
157+
}
158+
159+
template <typename MutexType>
160+
static void push_lock(MutexType* c, const CLockLocation& locklocation)
161+
{
162+
constexpr bool is_recursive_mutex =
163+
std::is_base_of<RecursiveMutex, MutexType>::value ||
164+
std::is_base_of<std::recursive_mutex, MutexType>::value;
165+
140166
LockData& lockdata = GetLockData();
141167
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
142168

143169
LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
144170
lock_stack.emplace_back(c, locklocation);
145-
for (const LockStackItem& i : lock_stack) {
146-
if (i.first == c)
147-
break;
171+
for (size_t j = 0; j < lock_stack.size() - 1; ++j) {
172+
const LockStackItem& i = lock_stack[j];
173+
if (i.first == c) {
174+
if (is_recursive_mutex) {
175+
break;
176+
}
177+
// It is not a recursive mutex and it appears in the stack two times:
178+
// at position `j` and at the end (which we added just before this loop).
179+
// Can't allow locking the same (non-recursive) mutex two times from the
180+
// same thread as that results in an undefined behavior.
181+
auto lock_stack_copy = lock_stack;
182+
lock_stack.pop_back();
183+
double_lock_detected(c, lock_stack_copy);
184+
// double_lock_detected() does not return.
185+
}
148186

149187
const LockPair p1 = std::make_pair(i.first, c);
150188
if (lockdata.lockorders.count(p1))
@@ -175,10 +213,16 @@ static void pop_lock()
175213
}
176214
}
177215

178-
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry)
216+
template <typename MutexType>
217+
void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry)
179218
{
180219
push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry, util::ThreadGetInternalName()));
181220
}
221+
template void EnterCritical(const char*, const char*, int, Mutex*, bool);
222+
template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool);
223+
template void EnterCritical(const char*, const char*, int, std::mutex*, bool);
224+
template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool);
225+
template void EnterCritical(const char*, const char*, int, boost::mutex*, bool);
182226

183227
void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line)
184228
{

‎src/sync.h‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
4848
///////////////////////////////
4949

5050
#ifdef DEBUG_LOCKORDER
51-
void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
51+
template <typename MutexType>
52+
void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false);
5253
void LeaveCritical();
5354
void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
5455
std::string LocksHeld();
@@ -66,7 +67,8 @@ bool LockStackEmpty();
6667
*/
6768
extern bool g_debug_lockorder_abort;
6869
#else
69-
inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
70+
template <typename MutexType>
71+
inline void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false) {}
7072
inline void LeaveCritical() {}
7173
inline void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line) {}
7274
template <typename MutexType>
@@ -135,7 +137,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base
135137
private:
136138
void Enter(const char* pszName, const char* pszFile, int nLine)
137139
{
138-
EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()));
140+
EnterCritical(pszName, pszFile, nLine, Base::mutex());
139141
#ifdef DEBUG_LOCKCONTENTION
140142
if (!Base::try_lock()) {
141143
PrintLockContention(pszName, pszFile, nLine);
@@ -148,7 +150,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base
148150

149151
bool TryEnter(const char* pszName, const char* pszFile, int nLine)
150152
{
151-
EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true);
153+
EnterCritical(pszName, pszFile, nLine, Base::mutex(), true);
152154
Base::try_lock();
153155
if (!Base::owns_lock())
154156
LeaveCritical();
@@ -205,7 +207,7 @@ class SCOPED_LOCKABLE UniqueLock : public Base
205207

206208
~reverse_lock() {
207209
templock.swap(lock);
208-
EnterCritical(lockname.c_str(), file.c_str(), line, (void*)lock.mutex());
210+
EnterCritical(lockname.c_str(), file.c_str(), line, lock.mutex());
209211
lock.lock();
210212
}
211213

@@ -236,7 +238,7 @@ using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove
236238

237239
#define ENTER_CRITICAL_SECTION(cs) \
238240
{ \
239-
EnterCritical(#cs, __FILE__, __LINE__, (void*)(&cs)); \
241+
EnterCritical(#cs, __FILE__, __LINE__, &cs); \
240242
(cs).lock(); \
241243
}
242244

‎src/test/sync_tests.cpp‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@
66
#include <test/util/setup_common.h>
77

88
#include <boost/test/unit_test.hpp>
9+
#include <boost/thread/mutex.hpp>
10+
11+
#include <mutex>
912

1013
namespace {
1114
template <typename MutexType>
@@ -29,6 +32,38 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
2932
BOOST_CHECK(!error_thrown);
3033
#endif
3134
}
35+
36+
#ifdef DEBUG_LOCKORDER
37+
template <typename MutexType>
38+
void TestDoubleLock2(MutexType& m)
39+
{
40+
ENTER_CRITICAL_SECTION(m);
41+
LEAVE_CRITICAL_SECTION(m);
42+
}
43+
44+
template <typename MutexType>
45+
void TestDoubleLock(bool should_throw)
46+
{
47+
const bool prev = g_debug_lockorder_abort;
48+
g_debug_lockorder_abort = false;
49+
50+
MutexType m;
51+
ENTER_CRITICAL_SECTION(m);
52+
if (should_throw) {
53+
BOOST_CHECK_EXCEPTION(
54+
TestDoubleLock2(m), std::logic_error, [](const std::logic_error& e) {
55+
return strcmp(e.what(), "double lock detected") == 0;
56+
});
57+
} else {
58+
BOOST_CHECK_NO_THROW(TestDoubleLock2(m));
59+
}
60+
LEAVE_CRITICAL_SECTION(m);
61+
62+
BOOST_CHECK(LockStackEmpty());
63+
64+
g_debug_lockorder_abort = prev;
65+
}
66+
#endif /* DEBUG_LOCKORDER */
3267
} // namespace
3368

3469
BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
@@ -55,4 +90,24 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
5590
#endif
5691
}
5792

93+
/* Double lock would produce an undefined behavior. Thus, we only do that if
94+
* DEBUG_LOCKORDER is activated to detect it. We don't want non-DEBUG_LOCKORDER
95+
* build to produce tests that exhibit known undefined behavior. */
96+
#ifdef DEBUG_LOCKORDER
97+
BOOST_AUTO_TEST_CASE(double_lock_mutex)
98+
{
99+
TestDoubleLock<Mutex>(true /* should throw */);
100+
}
101+
102+
BOOST_AUTO_TEST_CASE(double_lock_boost_mutex)
103+
{
104+
TestDoubleLock<boost::mutex>(true /* should throw */);
105+
}
106+
107+
BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex)
108+
{
109+
TestDoubleLock<RecursiveMutex>(false /* should not throw */);
110+
}
111+
#endif /* DEBUG_LOCKORDER */
112+
58113
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)