sync: detect double lock from the same thread #19337

vasild · 2020-06-20T13:49:46Z

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 (comment).

hebasto · 2020-06-20T13:54:04Z

Concept ACK.

maflcko

Concept ACK

maflcko · 2020-06-20T13:54:46Z

src/sync.cpp

+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);


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

Yeah, this is solved in #18710 :)

EDIT: I mean boost::mutex

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)

Just for reference - the two PRs met in master and afterwards that line was removed in #21010.

Thanks, @fanquake!

hebasto · 2020-06-20T14:53:23Z

Travis error:

https://travis-ci.org/github/bitcoin/bitcoin/jobs/700351407#L4430

hebasto

Tested 401d3b7 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:

could place boost headers into a separate group
in commit 401d3b7 message "... would produce an undefined behavior" -- mind rewording "would" -> "is"
could use unnamed namespace instead of static

hebasto · 2020-06-20T15:33:40Z

Travis error:

https://travis-ci.org/github/bitcoin/bitcoin/jobs/700351407#L4430

It is reliably reproducible locally.

hebasto · 2020-06-20T15:48:57Z

Travis error:

https://travis-ci.org/github/bitcoin/bitcoin/jobs/700351407#L4430

It is reliably reproducible locally.

@vasild
~~idk exactly why but~~ moving the potential_deadlock_detected test case to the end fixes the problem.

UPDATE: at the end of the potential_deadlock_detected test case the lock stack is not empty.

hebasto · 2020-06-20T20:27:07Z

Travis error:

* https://travis-ci.org/github/bitcoin/bitcoin/jobs/700351407#L4430

Fixed in #19340.

DrahtBot · 2020-06-20T22:06:07Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Drop some TSan suppressions #19983 (Drop some TSan suppressions by hebasto)
test: Fix inconsistent lock order in wallet_tests/CreateWallet #19982 (test: Fix inconsistent lock order in wallet_tests/CreateWalletFromFile by hebasto)

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.

jnewbery · 2020-06-21T18:32:49Z

Concept ACK

practicalswift · 2020-06-22T09:00:36Z

Concept ACK

Nice!

vasild · 2020-06-22T10:17:20Z

* 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.

hebasto

ACK d32d855

But #19340 should be considering for reviewing before this PR merging.

vasild · 2020-06-22T11:04:25Z

I converted this PR to "draft" so that it does not get merged accidentally. CI tests passed this time! I guess the order in which the tests are executed matters. #19340 will fix that and should be merged before this PR.

promag

Concept ACK, change LGTM.

hebasto · 2020-08-04T15:06:35Z

@vasild

I converted this PR to "draft" so that it does not get merged accidentally. CI tests passed this time! I guess the order in which the tests are executed matters. #19340 will fix that and should be merged before this PR.

#19340 is merged :)

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()`.

vasild · 2020-08-05T07:51:24Z

Reopened this PR now that #19340 is merged and rebased to resolve a conflict.

hebasto

ACK c269e0a, tested on Linux Mint 20 (x86_64).

src/test/sync_tests.cpp

promag · 2020-08-05T09:14:43Z

src/sync.cpp

+    lock_stack.pop_back();
+    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();


There's more cases of s/abort()/std::abort so a follow up to to do this is also fine.

promag · 2020-08-05T09:22:53Z

src/sync.cpp

+            // 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.
+            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().

src/sync.cpp

vasild · 2020-08-05T13:01:36Z

Addressed some review suggestions.

maflcko · 2020-08-05T14:02:29Z

a diff

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()

vasild · 2020-08-06T12:21:26Z

@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.

vasild · 2020-08-06T12:59:37Z

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

maflcko · 2020-08-07T08:29:38Z

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.

vasild · 2020-08-10T16:43:31Z

@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:
- if should_throw == true
```
mutex lock
if (should_throw) {
    // nothing
}
mutex unlock
```
- if should_throw == false
```
recursive mutex lock
recursive mutex lock
recursive mutex unlock
recursive mutex unlock
```
(with none of the code from sync.cpp exercised)

hebasto

re-ACK 95975dd

hebasto · 2020-08-15T10:30:45Z

@ajtowns @ryanofsky @practicalswift Mind looking into this?

laanwj · 2020-11-23T22:14:12Z

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.

laanwj · 2020-11-25T15:44:49Z

code review ACK 95975dd

maflcko

review ack

maflcko · 2020-11-26T10:00:58Z

src/sync.cpp

+        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__);


This will always blame the bug on sync.cpp itself:

detected double lock at sync.cpp:153

Right!

Addressed in #20507

promag · 2020-12-02T09:38:33Z

src/sync.cpp

+            // 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();


@vasild why copy and pop here?

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.

After the merge of bitcoin#18710, the linter is warning: ```bash A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced: src/sync.cpp:#include <boost/thread/mutex.hpp> src/test/sync_tests.cpp:#include <boost/thread/mutex.hpp> ^---- failure generated from test/lint/lint-includes.sh ``` the interim bitcoin#19337 was merged, which introduced more `boost::mutex` usage. Given we no longer use `boost::mutex`, just remove the double lock test and remaining includes.

f827e15 refactor: remove straggling boost::mutex usage (fanquake) Pull request description: After the merge of bitcoin#18710, the linter is warning: ```bash A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced: src/sync.cpp:#include <boost/thread/mutex.hpp> src/test/sync_tests.cpp:#include <boost/thread/mutex.hpp> ^---- failure generated from test/lint/lint-includes.sh ``` bitcoin#18710 removed `boost/thread/mutex.hpp` from lint-includes, however in the interim bitcoin#19337 was merged, which introduced more `boost::mutex` usage. Given we no longer use `boost::mutex`, just remove the double lock test and remaining includes. ACKs for top commit: laanwj: Code review ACK f827e15 hebasto: ACK f827e15 Tree-SHA512: f738b12189fe5b39db3e8f8231e9002714413a962eaf98adc84a6614fa474df5616358cfb1c89b92a2b0564efa9b704a774c49d4a25dca18a0ccc3cd9eabfc0a

After the merge of bitcoin#18710, the linter is warning: ```bash A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced: src/sync.cpp:#include <boost/thread/mutex.hpp> src/test/sync_tests.cpp:#include <boost/thread/mutex.hpp> ^---- failure generated from test/lint/lint-includes.sh ``` the interim bitcoin#19337 was merged, which introduced more `boost::mutex` usage. Given we no longer use `boost::mutex`, just remove the double lock test and remaining includes.

vasild mentioned this pull request Jun 20, 2020

refactor: Make CAddrMan::cs non-recursive #19238

Merged

maflcko reviewed Jun 20, 2020

View reviewed changes

maflcko added Refactoring Utils/log/libs and removed Refactoring labels Jun 20, 2020

hebasto reviewed Jun 20, 2020

View reviewed changes

hebasto mentioned this pull request Jun 20, 2020

Preserve the LockData initial state if "potential deadlock detected" exception thrown #19340

Merged

vasild force-pushed the detect_double_lock branch from 401d3b7 to d32d855 Compare June 22, 2020 10:11

hebasto approved these changes Jun 22, 2020

View reviewed changes

vasild marked this pull request as draft June 22, 2020 11:02

DrahtBot mentioned this pull request Jun 22, 2020

Fix mistakenly swapped "previous" and "current" lock orders #19353

Merged

promag reviewed Jun 22, 2020

View reviewed changes

This was referenced Jul 28, 2020

Remove mempool global #19556

Merged

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766

Merged

vasild marked this pull request as ready for review August 5, 2020 07:36

vasild force-pushed the detect_double_lock branch from d32d855 to c269e0a Compare August 5, 2020 07:49

hebasto approved these changes Aug 5, 2020

View reviewed changes

src/test/sync_tests.cpp Show resolved Hide resolved

promag reviewed Aug 5, 2020

View reviewed changes

vasild force-pushed the detect_double_lock branch from b1f9407 to f6a1a91 Compare August 5, 2020 13:01

vasild added a commit to vasild/bitcoin that referenced this pull request Aug 6, 2020

patch from bitcoin#19337 (comment)

2d547b4

sync: detect double lock from the same thread

95975dd

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.

vasild force-pushed the detect_double_lock branch from f6a1a91 to 95975dd Compare August 10, 2020 16:43

hebasto approved these changes Aug 15, 2020

View reviewed changes

maflcko mentioned this pull request Aug 27, 2020

test: Data race detected when running rpc_blockchain.py #19712

Closed

hebasto mentioned this pull request Sep 9, 2020

scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865

Closed

This was referenced Sep 20, 2020

Drop some TSan suppressions #19983

Merged

test: Fix inconsistent lock order in wallet_tests/CreateWallet #19982

Merged

hebasto mentioned this pull request Oct 14, 2020

p2p, refactor: Use Mutex type for some mutexes in CNode class #19915

Merged

laanwj merged commit 5009159 into bitcoin:master Nov 25, 2020

vasild deleted the detect_double_lock branch November 25, 2020 16:04

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2020

Merge bitcoin#19337: sync: detect double lock from the same thread

e7388e9

maflcko reviewed Nov 26, 2020

View reviewed changes

promag reviewed Dec 2, 2020

View reviewed changes

fanquake mentioned this pull request Jan 26, 2021

refactor: remove straggling boost::mutex usage #21010

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

sync: detect double lock from the same thread #19337

sync: detect double lock from the same thread #19337

Uh oh!

Conversation

vasild commented Jun 20, 2020

Uh oh!

hebasto commented Jun 20, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 20, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 20, 2020

Uh oh!

hebasto commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jun 20, 2020

Uh oh!

DrahtBot commented Jun 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Jun 21, 2020

Uh oh!

practicalswift commented Jun 22, 2020

Uh oh!

vasild commented Jun 22, 2020

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasild commented Jun 22, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 4, 2020

Uh oh!

vasild commented Aug 5, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Aug 5, 2020

Uh oh!

maflcko commented Aug 5, 2020

Uh oh!

vasild commented Aug 6, 2020

Uh oh!

vasild commented Aug 6, 2020

Uh oh!

maflcko commented Aug 7, 2020

hebasto Jun 20, 2020 •

edited

Loading

hebasto commented Jun 20, 2020 •

edited

Loading

DrahtBot commented Jun 20, 2020 •

edited

Loading

hebasto left a comment •

edited

Loading