From 41b88e93375d57db12da923f45f87b9a2db8e730 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 8 Nov 2017 15:28:35 -0500 Subject: [PATCH 1/4] Add unit test for DEBUG_LOCKORDER code --- src/Makefile.test.include | 1 + src/sync.cpp | 8 +++++++- src/sync.h | 7 +++++++ src/test/sync_tests.cpp | 41 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 src/test/sync_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 6f401636f52..516059917c7 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -81,6 +81,7 @@ BITCOIN_TESTS =\ test/sigopcount_tests.cpp \ test/skiplist_tests.cpp \ test/streams_tests.cpp \ + test/sync_tests.cpp \ test/timedata_tests.cpp \ test/torcontrol_tests.cpp \ test/transaction_tests.cpp \ diff --git a/src/sync.cpp b/src/sync.cpp index 28a4e37e68d..96ea61b211d 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -100,7 +100,11 @@ static void potential_deadlock_detected(const std::pair& mismatch, } LogPrintf(" %s\n", i.second.ToString()); } - assert(false); + if (g_debug_lockorder_abort) { + fprintf(stderr, "Assertion failed: detected inconsistent lock order at %s:%i, details in debug log.\n", __FILE__, __LINE__); + abort(); + } + throw std::logic_error("potential deadlock detected"); } static void push_lock(void* c, const CLockLocation& locklocation) @@ -189,4 +193,6 @@ void DeleteLock(void* cs) } } +bool g_debug_lockorder_abort = true; + #endif /* DEBUG_LOCKORDER */ diff --git a/src/sync.h b/src/sync.h index 882ad5dc4c0..b8cb84f79fe 100644 --- a/src/sync.h +++ b/src/sync.h @@ -77,6 +77,13 @@ std::string LocksHeld(); void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs); void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs); void DeleteLock(void* cs); + +/** + * Call abort() if a potential lock order deadlock bug is detected, instead of + * just logging information and throwing a logic_error. Defaults to true, and + * set to false in DEBUG_LOCKORDER unit tests. + */ +extern bool g_debug_lockorder_abort; #else void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} void static inline LeaveCritical() {} diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp new file mode 100644 index 00000000000..2e0951c3a95 --- /dev/null +++ b/src/test/sync_tests.cpp @@ -0,0 +1,41 @@ +// Copyright (c) 2012-2017 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include + +#include + +BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(potential_deadlock_detected) +{ + #ifdef DEBUG_LOCKORDER + bool prev = g_debug_lockorder_abort; + g_debug_lockorder_abort = false; + #endif + + CCriticalSection mutex1, mutex2; + { + LOCK2(mutex1, mutex2); + } + bool error_thrown = false; + try { + LOCK2(mutex2, mutex1); + } catch (const std::logic_error& e) { + BOOST_CHECK_EQUAL(e.what(), "potential deadlock detected"); + error_thrown = true; + } + #ifdef DEBUG_LOCKORDER + BOOST_CHECK(error_thrown); + #else + BOOST_CHECK(!error_thrown); + #endif + + #ifdef DEBUG_LOCKORDER + g_debug_lockorder_abort = prev; + #endif +} + +BOOST_AUTO_TEST_SUITE_END() From ba1f095aadf29bddb0bd8176d2e0b908f92a5623 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 7 Dec 2017 12:01:53 -0500 Subject: [PATCH 2/4] MOVEONLY Move AnnotatedMixin declaration Move AnnotatedMixin closer to where it's used, and after the DEBUG_LOCKORDER function declarations so it can call them. --- src/sync.h | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/sync.h b/src/sync.h index b8cb84f79fe..c9a950799e8 100644 --- a/src/sync.h +++ b/src/sync.h @@ -46,30 +46,6 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII // // /////////////////////////////// -/** - * Template mixin that adds -Wthread-safety locking - * annotations to a subset of the mutex API. - */ -template -class LOCKABLE AnnotatedMixin : public PARENT -{ -public: - void lock() EXCLUSIVE_LOCK_FUNCTION() - { - PARENT::lock(); - } - - void unlock() UNLOCK_FUNCTION() - { - PARENT::unlock(); - } - - bool try_lock() EXCLUSIVE_TRYLOCK_FUNCTION(true) - { - return PARENT::try_lock(); - } -}; - #ifdef DEBUG_LOCKORDER void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false); void LeaveCritical(); @@ -94,6 +70,30 @@ void static inline DeleteLock(void* cs) {} #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs) #define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs) +/** + * Template mixin that adds -Wthread-safety locking + * annotations to a subset of the mutex API. + */ +template +class LOCKABLE AnnotatedMixin : public PARENT +{ +public: + void lock() EXCLUSIVE_LOCK_FUNCTION() + { + PARENT::lock(); + } + + void unlock() UNLOCK_FUNCTION() + { + PARENT::unlock(); + } + + bool try_lock() EXCLUSIVE_TRYLOCK_FUNCTION(true) + { + return PARENT::try_lock(); + } +}; + /** * Wrapped mutex: supports recursive locking, but no waiting * TODO: We should move away from using the recursive lock by default. From 1382913e61f5db6ba849b1e261e8aefcd5a1ae68 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 8 Nov 2017 16:21:13 -0500 Subject: [PATCH 3/4] Make LOCK, LOCK2, TRY_LOCK work with CWaitableCriticalSection They should also work with any other mutex type which std::unique_lock supports. There is no change in behavior for current code that calls these macros with CCriticalSection mutexes. --- src/sync.h | 61 ++++++++++++++++++++++------------------- src/test/sync_tests.cpp | 29 ++++++++++++++------ 2 files changed, 53 insertions(+), 37 deletions(-) diff --git a/src/sync.h b/src/sync.h index c9a950799e8..339a7e2c187 100644 --- a/src/sync.h +++ b/src/sync.h @@ -71,13 +71,17 @@ void static inline DeleteLock(void* cs) {} #define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs) /** - * Template mixin that adds -Wthread-safety locking - * annotations to a subset of the mutex API. + * Template mixin that adds -Wthread-safety locking annotations and lock order + * checking to a subset of the mutex API. */ template class LOCKABLE AnnotatedMixin : public PARENT { public: + ~AnnotatedMixin() { + DeleteLock((void*)this); + } + void lock() EXCLUSIVE_LOCK_FUNCTION() { PARENT::lock(); @@ -92,19 +96,15 @@ public: { return PARENT::try_lock(); } + + using UniqueLock = std::unique_lock; }; /** * Wrapped mutex: supports recursive locking, but no waiting * TODO: We should move away from using the recursive lock by default. */ -class CCriticalSection : public AnnotatedMixin -{ -public: - ~CCriticalSection() { - DeleteLock((void*)this); - } -}; +typedef AnnotatedMixin CCriticalSection; /** Wrapped mutex: supports waiting but not recursive locking */ typedef AnnotatedMixin CWaitableCriticalSection; @@ -119,20 +119,19 @@ typedef std::unique_lock WaitableLock; void PrintLockContention(const char* pszName, const char* pszFile, int nLine); #endif -/** Wrapper around std::unique_lock */ -class SCOPED_LOCKABLE CCriticalBlock +/** Wrapper around std::unique_lock style lock for Mutex. */ +template +class SCOPED_LOCKABLE CCriticalBlock : public Base { private: - std::unique_lock lock; - void Enter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex())); + EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex())); #ifdef DEBUG_LOCKCONTENTION - if (!lock.try_lock()) { + if (!Base::try_lock()) { PrintLockContention(pszName, pszFile, nLine); #endif - lock.lock(); + Base::lock(); #ifdef DEBUG_LOCKCONTENTION } #endif @@ -140,15 +139,15 @@ private: bool TryEnter(const char* pszName, const char* pszFile, int nLine) { - EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()), true); - lock.try_lock(); - if (!lock.owns_lock()) + EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true); + Base::try_lock(); + if (!Base::owns_lock()) LeaveCritical(); - return lock.owns_lock(); + return Base::owns_lock(); } public: - CCriticalBlock(CCriticalSection& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : lock(mutexIn, std::defer_lock) + CCriticalBlock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock) { if (fTry) TryEnter(pszName, pszFile, nLine); @@ -156,11 +155,11 @@ public: Enter(pszName, pszFile, nLine); } - CCriticalBlock(CCriticalSection* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn) + CCriticalBlock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn) { if (!pmutexIn) return; - lock = std::unique_lock(*pmutexIn, std::defer_lock); + *static_cast(this) = Base(*pmutexIn, std::defer_lock); if (fTry) TryEnter(pszName, pszFile, nLine); else @@ -169,22 +168,28 @@ public: ~CCriticalBlock() UNLOCK_FUNCTION() { - if (lock.owns_lock()) + if (Base::owns_lock()) LeaveCritical(); } operator bool() { - return lock.owns_lock(); + return Base::owns_lock(); } }; +template +using DebugLock = CCriticalBlock::type>::type>; + #define PASTE(x, y) x ## y #define PASTE2(x, y) PASTE(x, y) -#define LOCK(cs) CCriticalBlock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__) -#define LOCK2(cs1, cs2) CCriticalBlock criticalblock1(cs1, #cs1, __FILE__, __LINE__), criticalblock2(cs2, #cs2, __FILE__, __LINE__) -#define TRY_LOCK(cs, name) CCriticalBlock name(cs, #cs, __FILE__, __LINE__, true) +#define LOCK(cs) DebugLock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__) +#define LOCK2(cs1, cs2) \ + DebugLock criticalblock1(cs1, #cs1, __FILE__, __LINE__); \ + DebugLock criticalblock2(cs2, #cs2, __FILE__, __LINE__); +#define TRY_LOCK(cs, name) DebugLock name(cs, #cs, __FILE__, __LINE__, true) +#define WAIT_LOCK(cs, name) DebugLock name(cs, #cs, __FILE__, __LINE__) #define ENTER_CRITICAL_SECTION(cs) \ { \ diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp index 2e0951c3a95..539e2ff3ab4 100644 --- a/src/test/sync_tests.cpp +++ b/src/test/sync_tests.cpp @@ -7,16 +7,10 @@ #include -BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) - -BOOST_AUTO_TEST_CASE(potential_deadlock_detected) +namespace { +template +void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2) { - #ifdef DEBUG_LOCKORDER - bool prev = g_debug_lockorder_abort; - g_debug_lockorder_abort = false; - #endif - - CCriticalSection mutex1, mutex2; { LOCK2(mutex1, mutex2); } @@ -32,6 +26,23 @@ BOOST_AUTO_TEST_CASE(potential_deadlock_detected) #else BOOST_CHECK(!error_thrown); #endif +} +} // namespace + +BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup) + +BOOST_AUTO_TEST_CASE(potential_deadlock_detected) +{ + #ifdef DEBUG_LOCKORDER + bool prev = g_debug_lockorder_abort; + g_debug_lockorder_abort = false; + #endif + + CCriticalSection rmutex1, rmutex2; + TestPotentialDeadLockDetected(rmutex1, rmutex2); + + CWaitableCriticalSection mutex1, mutex2; + TestPotentialDeadLockDetected(mutex1, mutex2); #ifdef DEBUG_LOCKORDER g_debug_lockorder_abort = prev; From 9c4dc597ddc66acfd58a945a5ab11f833731abba Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Wed, 8 Nov 2017 17:07:40 -0500 Subject: [PATCH 4/4] Use LOCK macros for non-recursive locks Instead of std::unique_lock. --- src/httpserver.cpp | 8 ++++---- src/init.cpp | 4 ++-- src/net.cpp | 4 ++-- src/net.h | 2 +- src/random.cpp | 7 ++++--- src/rpc/blockchain.cpp | 8 ++++---- src/rpc/mining.cpp | 2 +- src/sync.h | 3 --- src/threadinterrupt.cpp | 6 ++++-- src/threadinterrupt.h | 4 +++- src/validation.cpp | 2 +- 11 files changed, 26 insertions(+), 24 deletions(-) diff --git a/src/httpserver.cpp b/src/httpserver.cpp index 9daf3d19684..ceddc7c2c5b 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -69,7 +69,7 @@ class WorkQueue { private: /** Mutex protects entire object */ - std::mutex cs; + CWaitableCriticalSection cs; std::condition_variable cond; std::deque> queue; bool running; @@ -88,7 +88,7 @@ public: /** Enqueue a work item */ bool Enqueue(WorkItem* item) { - std::unique_lock lock(cs); + LOCK(cs); if (queue.size() >= maxDepth) { return false; } @@ -102,7 +102,7 @@ public: while (true) { std::unique_ptr i; { - std::unique_lock lock(cs); + WAIT_LOCK(cs, lock); while (running && queue.empty()) cond.wait(lock); if (!running) @@ -116,7 +116,7 @@ public: /** Interrupt and exit loops */ void Interrupt() { - std::unique_lock lock(cs); + LOCK(cs); running = false; cond.notify_all(); } diff --git a/src/init.cpp b/src/init.cpp index 21445929dc8..8ab5790aea5 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -566,7 +566,7 @@ static void BlockNotifyGenesisWait(bool, const CBlockIndex *pBlockIndex) { if (pBlockIndex != nullptr) { { - WaitableLock lock_GenesisWait(cs_GenesisWait); + LOCK(cs_GenesisWait); fHaveGenesis = true; } condvar_GenesisWait.notify_all(); @@ -1660,7 +1660,7 @@ bool AppInitMain() // Wait for genesis block to be processed { - WaitableLock lock(cs_GenesisWait); + WAIT_LOCK(cs_GenesisWait, lock); // We previously could hang here if StartShutdown() is called prior to // ThreadImport getting started, so instead we just wait on a timer to // check ShutdownRequested() regularly. diff --git a/src/net.cpp b/src/net.cpp index 5be91fd4f33..37b452a71e0 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2064,7 +2064,7 @@ void CConnman::ThreadMessageHandler() pnode->Release(); } - std::unique_lock lock(mutexMsgProc); + WAIT_LOCK(mutexMsgProc, lock); if (!fMoreWork) { condMsgProc.wait_until(lock, std::chrono::steady_clock::now() + std::chrono::milliseconds(100), [this] { return fMsgProcWake; }); } @@ -2346,7 +2346,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) flagInterruptMsgProc = false; { - std::unique_lock lock(mutexMsgProc); + LOCK(mutexMsgProc); fMsgProcWake = false; } diff --git a/src/net.h b/src/net.h index 36c2a4b8f50..5c47f9695c4 100644 --- a/src/net.h +++ b/src/net.h @@ -425,7 +425,7 @@ private: bool fMsgProcWake; std::condition_variable condMsgProc; - std::mutex mutexMsgProc; + CWaitableCriticalSection mutexMsgProc; std::atomic flagInterruptMsgProc; CThreadInterrupt interruptNet; diff --git a/src/random.cpp b/src/random.cpp index fee6c2d92aa..7f05bcf9a20 100644 --- a/src/random.cpp +++ b/src/random.cpp @@ -12,6 +12,7 @@ #include #endif #include // for LogPrint() +#include // for WAIT_LOCK #include // for GetTime() #include @@ -295,7 +296,7 @@ void RandAddSeedSleep() } -static std::mutex cs_rng_state; +static CWaitableCriticalSection cs_rng_state; static unsigned char rng_state[32] = {0}; static uint64_t rng_counter = 0; @@ -305,7 +306,7 @@ static void AddDataToRng(void* data, size_t len) { hasher.Write((const unsigned char*)data, len); unsigned char buf[64]; { - std::unique_lock lock(cs_rng_state); + WAIT_LOCK(cs_rng_state, lock); hasher.Write(rng_state, sizeof(rng_state)); hasher.Write((const unsigned char*)&rng_counter, sizeof(rng_counter)); ++rng_counter; @@ -337,7 +338,7 @@ void GetStrongRandBytes(unsigned char* out, int num) // Combine with and update state { - std::unique_lock lock(cs_rng_state); + WAIT_LOCK(cs_rng_state, lock); hasher.Write(rng_state, sizeof(rng_state)); hasher.Write((const unsigned char*)&rng_counter, sizeof(rng_counter)); ++rng_counter; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 46dec4ca6e6..3aa01914a66 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -49,7 +49,7 @@ struct CUpdatedBlock int height; }; -static std::mutex cs_blockchange; +static CWaitableCriticalSection cs_blockchange; static std::condition_variable cond_blockchange; static CUpdatedBlock latestblock; @@ -224,7 +224,7 @@ static UniValue waitfornewblock(const JSONRPCRequest& request) CUpdatedBlock block; { - std::unique_lock lock(cs_blockchange); + WAIT_LOCK(cs_blockchange, lock); block = latestblock; if(timeout) cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&block]{return latestblock.height != block.height || latestblock.hash != block.hash || !IsRPCRunning(); }); @@ -266,7 +266,7 @@ static UniValue waitforblock(const JSONRPCRequest& request) CUpdatedBlock block; { - std::unique_lock lock(cs_blockchange); + WAIT_LOCK(cs_blockchange, lock); if(timeout) cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&hash]{return latestblock.hash == hash || !IsRPCRunning();}); else @@ -309,7 +309,7 @@ static UniValue waitforblockheight(const JSONRPCRequest& request) CUpdatedBlock block; { - std::unique_lock lock(cs_blockchange); + WAIT_LOCK(cs_blockchange, lock); if(timeout) cond_blockchange.wait_for(lock, std::chrono::milliseconds(timeout), [&height]{return latestblock.height >= height || !IsRPCRunning();}); else diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index e751587dc71..59bf0c98ab5 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -470,7 +470,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request) { checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1); - WaitableLock lock(g_best_block_mutex); + WAIT_LOCK(g_best_block_mutex, lock); while (g_best_block == hashWatchedChain && IsRPCRunning()) { if (g_best_block_cv.wait_until(lock, checktxtime) == std::cv_status::timeout) diff --git a/src/sync.h b/src/sync.h index 339a7e2c187..7306e7285e2 100644 --- a/src/sync.h +++ b/src/sync.h @@ -112,9 +112,6 @@ typedef AnnotatedMixin CWaitableCriticalSection; /** Just a typedef for std::condition_variable, can be wrapped later if desired */ typedef std::condition_variable CConditionVariable; -/** Just a typedef for std::unique_lock, can be wrapped later if desired */ -typedef std::unique_lock WaitableLock; - #ifdef DEBUG_LOCKCONTENTION void PrintLockContention(const char* pszName, const char* pszFile, int nLine); #endif diff --git a/src/threadinterrupt.cpp b/src/threadinterrupt.cpp index 7da4e136ef5..1efc6f31143 100644 --- a/src/threadinterrupt.cpp +++ b/src/threadinterrupt.cpp @@ -5,6 +5,8 @@ #include +#include + CThreadInterrupt::CThreadInterrupt() : flag(false) {} CThreadInterrupt::operator bool() const @@ -20,7 +22,7 @@ void CThreadInterrupt::reset() void CThreadInterrupt::operator()() { { - std::unique_lock lock(mut); + LOCK(mut); flag.store(true, std::memory_order_release); } cond.notify_all(); @@ -28,7 +30,7 @@ void CThreadInterrupt::operator()() bool CThreadInterrupt::sleep_for(std::chrono::milliseconds rel_time) { - std::unique_lock lock(mut); + WAIT_LOCK(mut, lock); return !cond.wait_for(lock, rel_time, [this]() { return flag.load(std::memory_order_acquire); }); } diff --git a/src/threadinterrupt.h b/src/threadinterrupt.h index d373e3c371e..8ba6b123678 100644 --- a/src/threadinterrupt.h +++ b/src/threadinterrupt.h @@ -5,6 +5,8 @@ #ifndef BITCOIN_THREADINTERRUPT_H #define BITCOIN_THREADINTERRUPT_H +#include + #include #include #include @@ -28,7 +30,7 @@ public: private: std::condition_variable cond; - std::mutex mut; + CWaitableCriticalSection mut; std::atomic flag; }; diff --git a/src/validation.cpp b/src/validation.cpp index 702a8d7e058..2d6ac4b234f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2254,7 +2254,7 @@ void static UpdateTip(const CBlockIndex *pindexNew, const CChainParams& chainPar mempool.AddTransactionsUpdated(1); { - WaitableLock lock(g_best_block_mutex); + LOCK(g_best_block_mutex); g_best_block = pindexNew->GetBlockHash(); g_best_block_cv.notify_all(); }