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).
Concept ACK.
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);
no need to add dead code or code that is only used in tests
These instantiations are required even if tests do not exist. Without them it wouldn't link with errors like:
ld: error: undefined symbol: void EnterCritical<boost::mutex>(char const*, char const*, int, boost::mutex*, bool)
>>> referenced by checkqueue.h:184 (src/checkqueue.h:184)
>>> libbitcoin_server_a-validation.o:(CCheckQueueControl<CScriptCheck>::CCheckQueueControl(CCheckQueue<CScriptCheck>*)) in archive libbitcoin_server.a
So when these two PRs meet and if that was the last usage of boost::mutex, then this line can be removed: template void EnterCritical(const char*, const char*, int, boost::mutex*, bool);.
I tried compiling locally without it and it worked #19337 (comment)
Concept ACK
Tested 401d3b759e14a96332a033485257d2e993c2b944 with the following diff:
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -178,7 +178,7 @@ class CAddrMan
friend class CAddrManTest;
protected:
//! critical section to protect the inner data structures
- mutable RecursiveMutex cs;
+ mutable Mutex cs;
private:
//! last used nId
$ ./src/qt/bitcoin-qt -regtest
Assertion failed: detected double lock at sync.cpp:153, details in debug log.
Aborted (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 but~ moving the
potential_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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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
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.
Concept ACK, change LGTM.
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);
LEAVE_CRITICAL_SECTION(m);
BOOST_CHECK(LockStackEmpty());
Done!
ACK c269e0a22423294b81c7a60862bc20f6ccf80f1c, tested on Linux Mint 20 (x86_64).
re-ACK b1f94072a1cc3ca67ce13658bb981d03480ef38e
36 | +#ifdef DEBUG_LOCKORDER 37 | +template <typename MutexType> 38 | +void TestDoubleLock2(MutexType& m) 39 | +{ 40 | + ENTER_CRITICAL_SECTION(m); 41 | + LEAVE_CRITICAL_SECTION(m);
Couldn't this use 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:
src/sync.h:132:59: error: no type named 'UniqueLock' in 'std::__1::mutex'
template <typename Mutex, typename Base = typename Mutex::UniqueLock>
~~~~~~~~~~~~~~~~^~~~~~~~~~
Though, we never use 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);
{
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(
Could limit the #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.
I compiled and tested locally with sanitizers and it passed. Am I missing something? #19337 (comment)
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();
Would be nice if the 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)
Done - used the same as used when calling 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();
std::abort();
Should be the same symbol, but slightly clearer.
There's more cases of s/abort()/std::abort so a follow up to to do this is also fine.
ACK, only style and test nits. Feel free to ingore.
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);
Add assert(false) after this? Or some other "unreachable" assertion. Below, after potential_deadlock_detected() there's a comment, could also add the assertion there.
I added a comment, like done for 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) {
Could keep range loop? When lock_stack is changed the iteration stops.
We need to iterate on all but the last element.
Right, for double_lock_detected the last element should be skipped. But now lines L187-200 don't run for the last element.
Nevermind, the last element was already skipped with if (i.first == c) break;.
Addressed some review suggestions.
<details><summary>a diff</summary>
diff --git a/src/sync.cpp b/src/sync.cpp
index d020b4e334..55cc47191f 100644
--- a/src/sync.cpp
+++ b/src/sync.cpp
@@ -220,9 +220,6 @@ void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexTyp
}
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)
{
diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp
index 2ecc78bbed..9a3c276ed2 100644
--- a/src/test/sync_tests.cpp
+++ b/src/test/sync_tests.cpp
@@ -33,37 +33,44 @@ void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
#endif
}
-#ifdef DEBUG_LOCKORDER
template <typename MutexType>
void TestDoubleLock2(MutexType& m)
{
- ENTER_CRITICAL_SECTION(m);
- LEAVE_CRITICAL_SECTION(m);
+ LOCK(m);
}
template <typename MutexType>
void TestDoubleLock(bool should_throw)
{
+#ifdef DEBUG_LOCKORDER
const bool prev = g_debug_lockorder_abort;
g_debug_lockorder_abort = false;
+#endif /* DEBUG_LOCKORDER */
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));
+ {
+ LOCK(m);
+ if (should_throw) {
+#ifdef 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. */
+ BOOST_CHECK_EXCEPTION(
+ TestDoubleLock2(m), std::logic_error, [](const std::logic_error& e) {
+ return strcmp(e.what(), "double lock detected") == 0;
+ });
+#endif /* DEBUG_LOCKORDER */
+ } else {
+ BOOST_CHECK_NO_THROW(TestDoubleLock2(m));
+ }
}
- LEAVE_CRITICAL_SECTION(m);
BOOST_CHECK(LockStackEmpty());
+#ifdef DEBUG_LOCKORDER
g_debug_lockorder_abort = prev;
-}
#endif /* DEBUG_LOCKORDER */
+}
} // namespace
BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
@@ -90,34 +97,14 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
#endif
}
-/* 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_std_mutex)
-{
- TestDoubleLock<std::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 */);
}
-BOOST_AUTO_TEST_CASE(double_lock_std_recursive_mutex)
-{
- TestDoubleLock<std::recursive_mutex>(false /* should not throw */);
-}
-#endif /* DEBUG_LOCKORDER */
-
BOOST_AUTO_TEST_SUITE_END()
</details>
@MarcoFalke the above patch does not compile for me (neither with clang 11 nor with gcc 10) with an error like
ld: 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:
typedef AnnotatedMixin<std::mutex> Mutex;
template <typename PARENT>
class LOCKABLE AnnotatedMixin : public PARENT
{
...
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.
I pushed the patch to my travis instance and it breaks the build, similarly to what I observe locally: https://travis-ci.org/github/vasild/bitcoin/jobs/715483824#L3800
Obviously I didn't compile with --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 == truemutex lock
if (should_throw) {
// nothing
}
mutex unlock
should_throw == falserecursive mutex lock
recursive mutex lock
recursive mutex unlock
recursive mutex unlock
(with none of the code from sync.cpp exercised)
re-ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
@ajtowns @ryanofsky @practicalswift Mind looking into this?
Given how much of the current work on mutexes is converting recursive mutexes to non-resursive ones, it would be nice to get this in.
code review ACK 95975dd08d8fdaaeaf28e0d06b861ce2748c17b6
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:
detected double lock at sync.cpp:153
Right!
Addressed in https://github.com/bitcoin/bitcoin/pull/20507
review ack
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();
auto lock_stack_copy = lock_stack;
lock_stack.pop_back();
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.