From f24bd45b37e1b2d19e5a053dbfefa30306c1d41a Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sat, 21 May 2022 01:08:31 +1000 Subject: [PATCH 1/6] net_processing: thread safety annotation for m_tx_relay_mutex --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7aa1c052412..b81119bdd6d 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -291,7 +291,7 @@ struct Peer { return m_tx_relay.get(); }; - TxRelay* GetTxRelay() + TxRelay* GetTxRelay() EXCLUSIVE_LOCKS_REQUIRED(!m_tx_relay_mutex) { return WITH_LOCK(m_tx_relay_mutex, return m_tx_relay.get()); }; From be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Sat, 21 May 2022 01:08:47 +1000 Subject: [PATCH 2/6] qt/clientmodel: thread safety annotation for m_cached_tip_mutex --- src/qt/clientmodel.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h index b10ffb4523b..81f03a58ec6 100644 --- a/src/qt/clientmodel.h +++ b/src/qt/clientmodel.h @@ -105,7 +105,7 @@ private: //! A thread to interact with m_node asynchronously QThread* const m_thread; - void TipChanged(SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress, bool header); + void TipChanged(SynchronizationState sync_state, interfaces::BlockTip tip, double verification_progress, bool header) EXCLUSIVE_LOCKS_REQUIRED(!m_cached_tip_mutex); void subscribeToCoreSignals(); void unsubscribeFromCoreSignals(); From a559509a0b8cade27199740212d7b589f71a0e3b Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 20 Apr 2022 17:09:20 +1000 Subject: [PATCH 3/6] sync.h: Add GlobalMutex type --- src/sync.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/sync.h b/src/sync.h index a1759261136..b094b5d2e2a 100644 --- a/src/sync.h +++ b/src/sync.h @@ -129,10 +129,22 @@ using RecursiveMutex = AnnotatedMixin; /** Wrapped mutex: supports waiting but not recursive locking */ using Mutex = AnnotatedMixin; +/** Different type to mark Mutex at global scope + * + * Thread safety analysis can't handle negative assertions about mutexes + * with global scope well, so mark them with a separate type, and + * eventually move all the mutexes into classes so they are not globally + * visible. + * + * See: https://github.com/bitcoin/bitcoin/pull/20272#issuecomment-720755781 + */ +class GlobalMutex : public Mutex { }; + #define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs) inline void AssertLockNotHeldInline(const char* name, const char* file, int line, Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) { AssertLockNotHeldInternal(name, file, line, cs); } inline void AssertLockNotHeldInline(const char* name, const char* file, int line, RecursiveMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); } +inline void AssertLockNotHeldInline(const char* name, const char* file, int line, GlobalMutex* cs) LOCKS_EXCLUDED(cs) { AssertLockNotHeldInternal(name, file, line, cs); } #define AssertLockNotHeld(cs) AssertLockNotHeldInline(#cs, __FILE__, __LINE__, &cs) /** Wrapper around std::unique_lock style lock for Mutex. */ From bba87c0553780eacf0317fbfec7330ea27aa02f8 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 20 Apr 2022 17:10:13 +1000 Subject: [PATCH 4/6] scripted-diff: Convert global Mutexes to GlobalMutexes -BEGIN VERIFY SCRIPT- sed -i -E -e '/^([a-z]+ )?Mutex [a-z]/ s/Mutex/GlobalMutex/' $(git grep -lE '^([a-z]+ )?Mutex [a-z]') -END VERIFY SCRIPT- --- src/init.cpp | 2 +- src/net.cpp | 2 +- src/net.h | 2 +- src/netbase.cpp | 2 +- src/rpc/blockchain.cpp | 2 +- src/rpc/server.cpp | 4 ++-- src/timedata.cpp | 2 +- src/util/system.cpp | 2 +- src/validation.cpp | 2 +- src/validation.h | 2 +- src/wallet/sqlite.cpp | 2 +- src/wallet/wallet.cpp | 4 ++-- src/warnings.cpp | 2 +- 13 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index e65af7e7ba1..e4b13cdf5be 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -600,7 +600,7 @@ void SetupServerArgs(ArgsManager& argsman) } static bool fHaveGenesis = false; -static Mutex g_genesis_wait_mutex; +static GlobalMutex g_genesis_wait_mutex; static std::condition_variable g_genesis_wait_cv; static void BlockNotifyGenesisWait(const CBlockIndex* pBlockIndex) diff --git a/src/net.cpp b/src/net.cpp index 676037a357d..8d1cff56203 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -113,7 +113,7 @@ static const uint64_t RANDOMIZER_ID_ADDRCACHE = 0x1cf2e4ddd306dda9ULL; // SHA256 // bool fDiscover = true; bool fListen = true; -Mutex g_maplocalhost_mutex; +GlobalMutex g_maplocalhost_mutex; std::map mapLocalHost GUARDED_BY(g_maplocalhost_mutex); static bool vfLimited[NET_MAX] GUARDED_BY(g_maplocalhost_mutex) = {}; std::string strSubVersion; diff --git a/src/net.h b/src/net.h index 3d1a2658c74..d6b605652b9 100644 --- a/src/net.h +++ b/src/net.h @@ -248,7 +248,7 @@ struct LocalServiceInfo { uint16_t nPort; }; -extern Mutex g_maplocalhost_mutex; +extern GlobalMutex g_maplocalhost_mutex; extern std::map mapLocalHost GUARDED_BY(g_maplocalhost_mutex); extern const std::string NET_MESSAGE_TYPE_OTHER; diff --git a/src/netbase.cpp b/src/netbase.cpp index 8ff3b7a68c8..030f462ed97 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -30,7 +30,7 @@ #endif // Settings -static Mutex g_proxyinfo_mutex; +static GlobalMutex g_proxyinfo_mutex; static Proxy proxyInfo[NET_MAX] GUARDED_BY(g_proxyinfo_mutex); static Proxy nameProxy GUARDED_BY(g_proxyinfo_mutex); int nConnectTimeout = DEFAULT_CONNECT_TIMEOUT; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 0148544dc98..3292f6cc3af 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -66,7 +66,7 @@ struct CUpdatedBlock int height; }; -static Mutex cs_blockchange; +static GlobalMutex cs_blockchange; static std::condition_variable cond_blockchange; static CUpdatedBlock latestblock GUARDED_BY(cs_blockchange); diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index af00acdc9f7..66ed18045e6 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -19,14 +19,14 @@ #include #include -static Mutex g_rpc_warmup_mutex; +static GlobalMutex g_rpc_warmup_mutex; static std::atomic g_rpc_running{false}; static bool fRPCInWarmup GUARDED_BY(g_rpc_warmup_mutex) = true; static std::string rpcWarmupStatus GUARDED_BY(g_rpc_warmup_mutex) = "RPC server started"; /* Timer-creating functions */ static RPCTimerInterface* timerInterface = nullptr; /* Map of name to timer. */ -static Mutex g_deadline_timers_mutex; +static GlobalMutex g_deadline_timers_mutex; static std::map > deadlineTimers GUARDED_BY(g_deadline_timers_mutex); static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler); diff --git a/src/timedata.cpp b/src/timedata.cpp index 06659871e5e..d881aa8043a 100644 --- a/src/timedata.cpp +++ b/src/timedata.cpp @@ -16,7 +16,7 @@ #include #include -static Mutex g_timeoffset_mutex; +static GlobalMutex g_timeoffset_mutex; static int64_t nTimeOffset GUARDED_BY(g_timeoffset_mutex) = 0; /** diff --git a/src/util/system.cpp b/src/util/system.cpp index 6a07139d937..ed90874aca6 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -96,7 +96,7 @@ const char * const BITCOIN_SETTINGS_FILENAME = "settings.json"; ArgsManager gArgs; /** Mutex to protect dir_locks. */ -static Mutex cs_dir_locks; +static GlobalMutex cs_dir_locks; /** A map that contains all the currently held directory locks. After * successful locking, these will be held here until the global destructor * cleans them up and thus automatically unlocks them, or ReleaseDirectoryLocks diff --git a/src/validation.cpp b/src/validation.cpp index a7cdf63a2ac..62bfb88b8a8 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -122,7 +122,7 @@ static constexpr int PRUNE_LOCK_BUFFER{10}; */ RecursiveMutex cs_main; -Mutex g_best_block_mutex; +GlobalMutex g_best_block_mutex; std::condition_variable g_best_block_cv; uint256 g_best_block; bool g_parallel_script_checks{false}; diff --git a/src/validation.h b/src/validation.h index 04745a6e36b..1b80d0ccf40 100644 --- a/src/validation.h +++ b/src/validation.h @@ -114,7 +114,7 @@ enum class SynchronizationState { }; extern RecursiveMutex cs_main; -extern Mutex g_best_block_mutex; +extern GlobalMutex g_best_block_mutex; extern std::condition_variable g_best_block_cv; /** Used to notify getblocktemplate RPC of new tips. */ extern uint256 g_best_block; diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp index 2515df31776..053fb8f9834 100644 --- a/src/wallet/sqlite.cpp +++ b/src/wallet/sqlite.cpp @@ -23,7 +23,7 @@ namespace wallet { static constexpr int32_t WALLET_SCHEMA_VERSION = 0; -static Mutex g_sqlite_mutex; +static GlobalMutex g_sqlite_mutex; static int g_sqlite_count GUARDED_BY(g_sqlite_mutex) = 0; static void ErrorLogCallback(void* arg, int code, const char* msg) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6c333c709b7..7d9f8b05bbf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -173,8 +173,8 @@ void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr& } } -static Mutex g_loading_wallet_mutex; -static Mutex g_wallet_release_mutex; +static GlobalMutex g_loading_wallet_mutex; +static GlobalMutex g_wallet_release_mutex; static std::condition_variable g_wallet_release_cv; static std::set g_loading_wallet_set GUARDED_BY(g_loading_wallet_mutex); static std::set g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex); diff --git a/src/warnings.cpp b/src/warnings.cpp index 60388cc7137..dabb194ce14 100644 --- a/src/warnings.cpp +++ b/src/warnings.cpp @@ -12,7 +12,7 @@ #include -static Mutex g_warnings_mutex; +static GlobalMutex g_warnings_mutex; static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex); static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false; From d2852917eecad6ab422a7b2c9892d351a7f0cc96 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 20 Apr 2022 17:11:07 +1000 Subject: [PATCH 5/6] sync.h: Imply negative assertions when calling LOCK --- src/sync.h | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/sync.h b/src/sync.h index b094b5d2e2a..7ec4b668acd 100644 --- a/src/sync.h +++ b/src/sync.h @@ -244,12 +244,26 @@ public: template using DebugLock = UniqueLock::type>::type>; -#define LOCK(cs) DebugLock UNIQUE_NAME(criticalblock)(cs, #cs, __FILE__, __LINE__) +// When locking a Mutex, require negative capability to ensure the lock +// is not already held +inline Mutex& MaybeCheckNotHeld(Mutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; } +inline Mutex* MaybeCheckNotHeld(Mutex* cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; } + +// When locking a GlobalMutex, just check it is not locked in the surrounding scope +inline GlobalMutex& MaybeCheckNotHeld(GlobalMutex& cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } +inline GlobalMutex* MaybeCheckNotHeld(GlobalMutex* cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } + +// When locking a RecursiveMutex, it's okay to already hold the lock +// but check that it is not known to be locked in the surrounding scope anyway +inline RecursiveMutex& MaybeCheckNotHeld(RecursiveMutex& cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } +inline RecursiveMutex* MaybeCheckNotHeld(RecursiveMutex* cs) LOCKS_EXCLUDED(cs) LOCK_RETURNED(cs) { return cs; } + +#define LOCK(cs) DebugLock UNIQUE_NAME(criticalblock)(MaybeCheckNotHeld(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__) + DebugLock criticalblock1(MaybeCheckNotHeld(cs1), #cs1, __FILE__, __LINE__); \ + DebugLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__); +#define TRY_LOCK(cs, name) DebugLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true) +#define WAIT_LOCK(cs, name) DebugLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__) #define ENTER_CRITICAL_SECTION(cs) \ { \ @@ -288,7 +302,7 @@ using DebugLock = UniqueLock decltype(auto) { LOCK(cs); code; }() +#define WITH_LOCK(cs, code) (MaybeCheckNotHeld(cs), [&]() -> decltype(auto) { LOCK(cs); code; }()) class CSemaphore { From ce893c0497fc9b8ab9752153dfcc77c9f427545e Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 20 Apr 2022 17:48:14 +1000 Subject: [PATCH 6/6] doc: Update developer notes --- doc/developer-notes.md | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index ddbbd0709b1..ea08c4f1a8e 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -902,14 +902,19 @@ Threads and synchronization - Prefer `Mutex` type to `RecursiveMutex` one. - Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to - get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with - run-time asserts in function definitions: + get compile-time warnings about potential race conditions or deadlocks in code. - In functions that are declared separately from where they are defined, the thread safety annotations should be added exclusively to the function declaration. Annotations on the definition could lead to false positives (lack of compile failure) at call sites between the two. + - Prefer locks that are in a class rather than global, and that are + internal to a class (private or protected) rather than public. + + - Combine annotations in function declarations with run-time asserts in + function definitions: + ```C++ // txmempool.h class CTxMemPool @@ -933,21 +938,37 @@ void CTxMemPool::UpdateTransactionsFromBlock(...) ```C++ // validation.h -class ChainstateManager +class CChainState { +protected: + ... + Mutex m_chainstate_mutex; + ... public: ... - bool ProcessNewBlock(...) LOCKS_EXCLUDED(::cs_main); + bool ActivateBestChain( + BlockValidationState& state, + std::shared_ptr pblock = nullptr) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); + ... + bool PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) + EXCLUSIVE_LOCKS_REQUIRED(!m_chainstate_mutex) + LOCKS_EXCLUDED(::cs_main); ... } // validation.cpp -bool ChainstateManager::ProcessNewBlock(...) +bool CChainState::PreciousBlock(BlockValidationState& state, CBlockIndex* pindex) { + AssertLockNotHeld(m_chainstate_mutex); AssertLockNotHeld(::cs_main); - ... - LOCK(::cs_main); - ... + { + LOCK(cs_main); + ... + } + + return ActivateBestChain(state, std::shared_ptr()); } ```