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: #19238 (review).
214 }
215+template void EnterCritical(const char*, const char*, int, Mutex*, bool);
216+template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool);
217+template void EnterCritical(const char*, const char*, int, std::mutex*, bool);
218+template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool);
219+template void EnterCritical(const char*, const char*, int, boost::mutex*, bool);
These instantiations are required even if tests do not exist. Without them it wouldn’t link with errors like:
0ld: error: undefined symbol: void EnterCritical<boost::mutex>(char const*, char const*, int, boost::mutex*, bool)
1>>> referenced by checkqueue.h:184 (src/checkqueue.h:184)
2>>> libbitcoin_server_a-validation.o:(CCheckQueueControl<CScriptCheck>::CCheckQueueControl(CCheckQueue<CScriptCheck>*)) in archive libbitcoin_server.a
boost::mutex, then this line can be removed: template void EnterCritical(const char*, const char*, int, boost::mutex*, bool);.
Tested 401d3b759e14a96332a033485257d2e993c2b944 with the following diff:
0--- a/src/addrman.h
1+++ b/src/addrman.h
2@@ -178,7 +178,7 @@ class CAddrMan
3 friend class CAddrManTest;
4 protected:
5 //! critical section to protect the inner data structures
6- mutable RecursiveMutex cs;
7+ mutable Mutex cs;
8
9 private:
10 //! last used nId
0$ ./src/qt/bitcoin-qt -regtest
1Assertion failed: detected double lock at sync.cpp:153, details in debug log.
2Aborted (core dumped)
Nice!
Some non-blocking nits:
static
Travis error:
It is reliably reproducible locally.
Travis error:
It is reliably reproducible locally. @vasild
idk exactly why butmoving thepotential_deadlock_detectedtest case to the end fixes the problem.
UPDATE: at the end of the potential_deadlock_detected test case the lock stack is not empty.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK
Nice!
* could place boost headers into a separate group
Done.
* in commit [401d3b7](https://github.com/bitcoin/bitcoin/commit/401d3b759e14a96332a033485257d2e993c2b944) message "... would produce an undefined behavior" -- mind rewording "would" -> "is"
Done.
* could use unnamed namespace instead of `static`
The rest of the functions in the file use static, so for consistency I also used static in the newly introduced function. Maybe replace all static with an unnamed namespace in sync.cpp? If yes, then that would go as a separate commit, or maybe even a separate PR.
Reviewers: this PR exposed an existent deficiency which is fixed in #19340.
The functions `EnterCritical()` and `push_lock()` take a pointer to a
mutex, but that pointer used to be of type `void*` because we use a few
different types for mutexes. This `void*` argument was not type safe
because somebody could have send a pointer to anything that is not a
mutex. Furthermore it wouldn't allow to check whether the passed mutex
is recursive or not.
Thus, change the functions to templated ones so that we can implement
stricter checks for non-recursive mutexes. This also simplifies the
callers of `EnterCritical()`.
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);
0 LEAVE_CRITICAL_SECTION(m);
1 BOOST_CHECK(LockStackEmpty());
36+#ifdef DEBUG_LOCKORDER
37+template <typename MutexType>
38+void TestDoubleLock2(MutexType& m)
39+{
40+ ENTER_CRITICAL_SECTION(m);
41+ LEAVE_CRITICAL_SECTION(m);
LOCK(m), since ENTER_CRITICAL_SECTION is mostly only used internally to the sync module and other code used LOCK
LOCK(m) does not compile if m is std::mutex:
0src/sync.h:132:59: error: no type named 'UniqueLock' in 'std::__1::mutex'
1template <typename Mutex, typename Base = typename Mutex::UniqueLock>
2 ~~~~~~~~~~~~~~~~^~~~~~~~~~
LOCK or even ENTER_CRITICAL_SECTION on std::mutex, so what is the point of testing for that? #19337 (comment)
46+{
47+ const bool prev = g_debug_lockorder_abort;
48+ g_debug_lockorder_abort = false;
49+
50+ MutexType m;
51+ ENTER_CRITICAL_SECTION(m);
0 {
1 LOCK(m);
Same here. Should be equivalent?
48+ g_debug_lockorder_abort = false;
49+
50+ MutexType m;
51+ ENTER_CRITICAL_SECTION(m);
52+ if (should_throw) {
53+ BOOST_CHECK_EXCEPTION(
#ifdef DEBUG_LOCKORDER to just this one line/statement? Benefit would be that BOOST_CHECK_NO_THROW will be run for non-debug builds as well.
Without DEBUG_LOCKORDER we would actually do the double lock and once executed then we are in “undefined behavior” state. We don’t want to do that because it may randomly crash the test executable, overwrite random portions of memory for the excitement of subsequent tests, etc.
152+ }
153+ // We throw or abort() here, never actually locking the mutex. So remove the entry
154+ // from the lock stack which we just added in push_lock(). Tests in sync_tests.cpp
155+ // continue executing past the exception and we don't want one test to leave a
156+ // bricked stack for a subsequent test that could execute in the same thread.
157+ lock_stack.pop_back();
potential_deadlock_detected and double_lock_detected helpers used the same style to pop_back the just added lock. (Either pass in a mutable reference or create a copy)
potential_deadlock_detected() in master - a copy.
155+ // continue executing past the exception and we don't want one test to leave a
156+ // bricked stack for a subsequent test that could execute in the same thread.
157+ lock_stack.pop_back();
158+ if (g_debug_lockorder_abort) {
159+ tfm::format(std::cerr, "Assertion failed: detected double lock at %s:%i, details in debug log.\n", __FILE__, __LINE__);
160+ abort();
0 std::abort();
Should be the same symbol, but slightly clearer.
186+ }
187+ // It is not a recursive mutex and it appears in the stack two times:
188+ // at position `j` and at the end (which we added just before this loop).
189+ // Can't allow locking the same (non-recursive) mutex two times from the
190+ // same thread as that results in an undefined behavior.
191+ double_lock_detected(c, lock_stack);
assert(false) after this? Or some other “unreachable” assertion. Below, after potential_deadlock_detected() there’s a comment, could also add the assertion there.
potential_deadlock_detected().
175 LockStack& lock_stack = lockdata.m_lock_stacks[std::this_thread::get_id()];
176 lock_stack.emplace_back(c, locklocation);
177- for (const LockStackItem& i : lock_stack) {
178- if (i.first == c)
179- break;
180+ for (size_t j = 0; j < lock_stack.size() - 1; ++j) {
lock_stack is changed the iteration stops.
double_lock_detected the last element should be skipped. But now lines L187-200 don’t run for the last element.
if (i.first == c) break;.
0diff --git a/src/sync.cpp b/src/sync.cpp
1index d020b4e334..55cc47191f 100644
2--- a/src/sync.cpp
3+++ b/src/sync.cpp
4@@ -220,9 +220,6 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexTyp
5 }
6 template void EnterCritical(const char*, const char*, int, Mutex*, bool);
7 template void EnterCritical(const char*, const char*, int, RecursiveMutex*, bool);
8-template void EnterCritical(const char*, const char*, int, std::mutex*, bool);
9-template void EnterCritical(const char*, const char*, int, std::recursive_mutex*, bool);
10-template void EnterCritical(const char*, const char*, int, boost::mutex*, bool);
11
12 void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line)
13 {
14diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp
15index 2ecc78bbed..9a3c276ed2 100644
16--- a/src/test/sync_tests.cpp
17+++ b/src/test/sync_tests.cpp
18@@ -33,37 +33,44 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
19 #endif
20 }
21
22-#ifdef DEBUG_LOCKORDER
23 template <typename MutexType>
24 void TestDoubleLock2(MutexType& m)
25 {
26- ENTER_CRITICAL_SECTION(m);
27- LEAVE_CRITICAL_SECTION(m);
28+ LOCK(m);
29 }
30
31 template <typename MutexType>
32 void TestDoubleLock(bool should_throw)
33 {
34+#ifdef DEBUG_LOCKORDER
35 const bool prev = g_debug_lockorder_abort;
36 g_debug_lockorder_abort = false;
37+#endif /* DEBUG_LOCKORDER */
38
39 MutexType m;
40- ENTER_CRITICAL_SECTION(m);
41- if (should_throw) {
42- BOOST_CHECK_EXCEPTION(
43- TestDoubleLock2(m), std::logic_error, [](const std::logic_error& e) {
44- return strcmp(e.what(), "double lock detected") == 0;
45- });
46- } else {
47- BOOST_CHECK_NO_THROW(TestDoubleLock2(m));
48+ {
49+ LOCK(m);
50+ if (should_throw) {
51+#ifdef DEBUG_LOCKORDER
52+ /* Double lock would produce an undefined behavior. Thus, we only do that if
53+ * DEBUG_LOCKORDER is activated to detect it. We don't want non-DEBUG_LOCKORDER
54+ * build to produce tests that exhibit known undefined behavior. */
55+ BOOST_CHECK_EXCEPTION(
56+ TestDoubleLock2(m), std::logic_error, [](const std::logic_error& e) {
57+ return strcmp(e.what(), "double lock detected") == 0;
58+ });
59+#endif /* DEBUG_LOCKORDER */
60+ } else {
61+ BOOST_CHECK_NO_THROW(TestDoubleLock2(m));
62+ }
63 }
64- LEAVE_CRITICAL_SECTION(m);
65
66 BOOST_CHECK(LockStackEmpty());
67
68+#ifdef DEBUG_LOCKORDER
69 g_debug_lockorder_abort = prev;
70-}
71 #endif /* DEBUG_LOCKORDER */
72+}
73 } // namespace
74
75 BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
76@@ -90,34 +97,14 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
77 #endif
78 }
79
80-/* Double lock would produce an undefined behavior. Thus, we only do that if
81- * DEBUG_LOCKORDER is activated to detect it. We don't want non-DEBUG_LOCKORDER
82- * build to produce tests that exhibit known undefined behavior. */
83-#ifdef DEBUG_LOCKORDER
84 BOOST_AUTO_TEST_CASE(double_lock_mutex)
85 {
86 TestDoubleLock<Mutex>(true /* should throw */);
87 }
88
89-BOOST_AUTO_TEST_CASE(double_lock_std_mutex)
90-{
91- TestDoubleLock<std::mutex>(true /* should throw */);
92-}
93-
94-BOOST_AUTO_TEST_CASE(double_lock_boost_mutex)
95-{
96- TestDoubleLock<boost::mutex>(true /* should throw */);
97-}
98-
99 BOOST_AUTO_TEST_CASE(double_lock_recursive_mutex)
100 {
101 TestDoubleLock<RecursiveMutex>(false /* should not throw */);
102 }
103
104-BOOST_AUTO_TEST_CASE(double_lock_std_recursive_mutex)
105-{
106- TestDoubleLock<std::recursive_mutex>(false /* should not throw */);
107-}
108-#endif /* DEBUG_LOCKORDER */
109-
110 BOOST_AUTO_TEST_SUITE_END()
@MarcoFalke the above patch does not compile for me (neither with clang 11 nor with gcc 10) with an error like
0ld: error: undefined symbol: void EnterCritical<std::mutex>(char const*, char const*, int, std::mutex*, bool)
I think you should also get it for ./configure --enable-debug when the explicit instantiation of EnterCritical<std::mutex>() is removed. Here is how we end up needing that instantiation even though we never use std::mutex directly:
0typedef AnnotatedMixin<std::mutex> Mutex;
1
2template <typename PARENT>
3class LOCKABLE AnnotatedMixin : public PARENT
4{
5...
6 using UniqueLock = std::unique_lock<PARENT>;
So when one defines Mutex m; this ends up being AnnotatedMixin<std::mutex> m; and AnnotatedMixin<std::mutex>::UniqueLock is std::unique_lock<std::mutex>.
LOCK(m) expands to
DebugLock<AnnotatedMixin<std::mutex>> criticalblock123(m, ...) which expands to
UniqueLock<AnnotatedMixin<std::mutex>> criticalblock123(m, ...) which expands to (with default 2nd template arg):
UniqueLock<AnnotatedMixin<std::mutex>, AnnotatedMixin<std::mutex>::UniqueLock> criticalblock123(m, ...) which expands to
UniqueLock<AnnotatedMixin<std::mutex>, std::unique_lock<std::mutex>> criticalblock123(m, ...).
Then the constructor of the above calls the Enter() method which calls
EnterCritical(name, file, line, Base::mutex()) which expands to
EnterCritical(name, file, line, std::unique_lock<std::mutex>::mutex()).
The mutex() method (4th argument) returns a pointer to std::mutex.
This is how we end up calling EnterCritical() with a 4th argument of type pointer to std::mutex.
Thus we need the explicit instantiation template void EnterCritical(const char*, const char*, int, std::mutex*, bool);. Same applies to std::recursive_mutex and boost::mutex.
--enable-debug (shame!). Please disregard the sync.cpp patch, but the other hunks should compile?
Double lock of the same (non-recursive) mutex from the same thread
is producing an undefined behavior. Detect this from DEBUG_LOCKORDER
and react similarly to the deadlock detection.
@MarcoFalke, I looked carefully at the diff above and took some of it but not all. Here are the reasons:
Indeed we never do LOCK() or ENTER_CRITICAL_SECTION() on std::mutex or std::recursive_mutex directly, so no need to test for that. Thus, I removed the tests double_lock_std_mutex and double_lock_std_recursive_mutex.
However we do call ENTER_CRITICAL_SECTION() on boost::mutex. So, I left the test double_lock_boost_mutex. So, TestDoubleLock2() cannot use LOCK(m) because it does not compile.
I do not think it makes sense to do these tests for non-DEBUG_LOCKORDER build because they will decay to:
should_throw == true0mutex lock
1if (should_throw) {
2 // nothing
3}
4mutex unlock
should_throw == false0recursive mutex lock
1recursive mutex lock
2recursive mutex unlock
3recursive mutex unlock
(with none of the code from sync.cpp exercised)
149+ LogPrintf(" (*)"); /* Continued */
150+ }
151+ LogPrintf(" %s\n", i.second.ToString());
152+ }
153+ if (g_debug_lockorder_abort) {
154+ tfm::format(std::cerr, "Assertion failed: detected double lock at %s:%i, details in debug log.\n", __FILE__, __LINE__);
This will always blame the bug on sync.cpp itself:
0detected double lock at sync.cpp:153
Right!
Addressed in https://github.com/bitcoin/bitcoin/pull/20507
181+ // It is not a recursive mutex and it appears in the stack two times:
182+ // at position `j` and at the end (which we added just before this loop).
183+ // Can't allow locking the same (non-recursive) mutex two times from the
184+ // same thread as that results in an undefined behavior.
185+ auto lock_stack_copy = lock_stack;
186+ lock_stack.pop_back();
0 auto lock_stack_copy = lock_stack;
1 lock_stack.pop_back();
2 double_lock_detected(c, lock_stack_copy);
That’s subtle:
double_lock_detected() can abort() in which case it does not matter, but it can also throw and tests catch this exception and continue working and executing other tests. So the lock stack needs to be correct after the throw because it is going to be reused by subsequent tests. Notice that we did not acquire the offending mutex that is at the top of the lock stack - we only detected a request to acquire it, which if executed would cause a double lock. So the topmost element needs to be removed before the throw.double_lock_detected() needs the full lock stack, including the offending-but-not-actually-acquired-mutex because it prints some of its elements. It will want to print that last one.