From cd27bba060d91995da706bc03cd2ef39b79b49b1 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 16 Jun 2015 03:46:36 -0400 Subject: [PATCH 1/3] locking: teach Clang's -Wthread-safety to cope with our scoped lock macros This allows us to use function/variable/class attributes to specify locking requisites, allowing problems to be detected during static analysis. This works perfectly with newer Clang versions (tested with 3.3-3.7). For older versions (tested 3.2), it compiles fine but spews lots of false-positives. --- src/sync.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sync.h b/src/sync.h index 78b904347..705647e4a 100644 --- a/src/sync.h +++ b/src/sync.h @@ -101,7 +101,7 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine); /** Wrapper around boost::unique_lock */ template -class CMutexLock +class SCOPED_LOCKABLE CMutexLock { private: boost::unique_lock lock; @@ -129,7 +129,7 @@ private: } public: - CMutexLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) : lock(mutexIn, boost::defer_lock) + CMutexLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : lock(mutexIn, boost::defer_lock) { if (fTry) TryEnter(pszName, pszFile, nLine); @@ -137,7 +137,7 @@ public: Enter(pszName, pszFile, nLine); } - CMutexLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) + CMutexLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn) { if (!pmutexIn) return; @@ -148,7 +148,7 @@ public: Enter(pszName, pszFile, nLine); } - ~CMutexLock() + ~CMutexLock() UNLOCK_FUNCTION() { if (lock.owns_lock()) LeaveCritical(); From 2b890dd424b32320be6fc0333e67e2d7c9616065 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 16 Jun 2015 04:02:25 -0400 Subject: [PATCH 2/3] locking: fix a few small issues uncovered by -Wthread-safety - rpcwallet: No need to lock twice here - openssl: Clang doesn't understand selective lock/unlock here. Ignore it. - CNode: Fix a legitimate (though very unlikely) locking bug. --- src/net.cpp | 4 +++- src/util.cpp | 2 +- src/wallet/rpcwallet.cpp | 1 - 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 42ac0e50e..adf89554d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2032,8 +2032,10 @@ void CNode::EndMessage() UNLOCK_FUNCTION(cs_vSend) Fuzz(GetArg("-fuzzmessagestest", 10)); if (ssSend.size() == 0) + { + LEAVE_CRITICAL_SECTION(cs_vSend); return; - + } // Set the size unsigned int nSize = ssSend.size() - CMessageHeader::HEADER_SIZE; WriteLE32((uint8_t*)&ssSend[CMessageHeader::MESSAGE_SIZE_OFFSET], nSize); diff --git a/src/util.cpp b/src/util.cpp index da5821e53..1097878b2 100644 --- a/src/util.cpp +++ b/src/util.cpp @@ -113,7 +113,7 @@ CTranslationInterface translationInterface; /** Init OpenSSL library multithreading support */ static CCriticalSection** ppmutexOpenSSL; -void locking_callback(int mode, int i, const char* file, int line) +void locking_callback(int mode, int i, const char* file, int line) NO_THREAD_SAFETY_ANALYSIS { if (mode & CRYPTO_LOCK) { ENTER_CRITICAL_SECTION(*ppmutexOpenSSL[i]); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5404dd4aa..b32f1f6b0 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -476,7 +476,6 @@ UniValue listaddressgroupings(const UniValue& params, bool fHelp) addressInfo.push_back(CBitcoinAddress(address).ToString()); addressInfo.push_back(ValueFromAmount(balances[address])); { - LOCK(pwalletMain->cs_wallet); if (pwalletMain->mapAddressBook.find(CBitcoinAddress(address).Get()) != pwalletMain->mapAddressBook.end()) addressInfo.push_back(pwalletMain->mapAddressBook.find(CBitcoinAddress(address).Get())->second.name); } From a794284e61988a226ea39327449be9906a1b5abd Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 16 Jun 2015 04:08:26 -0400 Subject: [PATCH 3/3] locking: add a quick example of GUARDED_BY This was chosen not because it's necessarily helpful, but because its locking assumptions were already correct. --- src/main.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 0be54ebd4..f198edffe 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -72,9 +72,9 @@ struct COrphanTx { CTransaction tx; NodeId fromPeer; }; -map mapOrphanTransactions; -map > mapOrphanTransactionsByPrev; -void EraseOrphansFor(NodeId peer); +map mapOrphanTransactions GUARDED_BY(cs_main);; +map > mapOrphanTransactionsByPrev GUARDED_BY(cs_main);; +void EraseOrphansFor(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * Returns true if there are nRequired or more blocks of minVersion or above @@ -523,7 +523,7 @@ CBlockTreeDB *pblocktree = NULL; // mapOrphanTransactions // -bool AddOrphanTx(const CTransaction& tx, NodeId peer) +bool AddOrphanTx(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { uint256 hash = tx.GetHash(); if (mapOrphanTransactions.count(hash)) @@ -553,7 +553,7 @@ bool AddOrphanTx(const CTransaction& tx, NodeId peer) return true; } -void static EraseOrphanTx(uint256 hash) +void static EraseOrphanTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { map::iterator it = mapOrphanTransactions.find(hash); if (it == mapOrphanTransactions.end()) @@ -587,7 +587,7 @@ void EraseOrphansFor(NodeId peer) } -unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) +unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { unsigned int nEvicted = 0; while (mapOrphanTransactions.size() > nMaxOrphans) @@ -3783,7 +3783,7 @@ std::string GetWarnings(const std::string& strFor) // -bool static AlreadyHave(const CInv& inv) +bool static AlreadyHave(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { switch (inv.type) {