Merge #9674: Always enforce strict lock ordering (try or not)

618ee92 Further-enforce lockordering by enforcing directly after TRY_LOCKs (Matt Corallo)
2a962d4 Fixup style a bit by moving { to the same line as if statements (Matt Corallo)
8465631 Always enforce lock strict lock ordering (try or not) (Matt Corallo)
fd13eca Lock cs_vSend and cs_inventory in a consistent order even in TRY (Matt Corallo)
This commit is contained in:
Wladimir J. van der Laan 2017-02-08 14:46:35 +01:00
commit dd163f5788
No known key found for this signature in database
GPG Key ID: 74810B012346C9A6
2 changed files with 20 additions and 48 deletions

View File

@ -1093,20 +1093,18 @@ void CConnman::ThreadSocketHandler()
BOOST_FOREACH(CNode* pnode, vNodesDisconnectedCopy) BOOST_FOREACH(CNode* pnode, vNodesDisconnectedCopy)
{ {
// wait until threads are done using it // wait until threads are done using it
if (pnode->GetRefCount() <= 0) if (pnode->GetRefCount() <= 0) {
{
bool fDelete = false; bool fDelete = false;
{ {
TRY_LOCK(pnode->cs_vSend, lockSend); TRY_LOCK(pnode->cs_inventory, lockInv);
if (lockSend) if (lockInv) {
{ TRY_LOCK(pnode->cs_vSend, lockSend);
TRY_LOCK(pnode->cs_inventory, lockInv); if (lockSend) {
if (lockInv) fDelete = true;
fDelete = true; }
} }
} }
if (fDelete) if (fDelete) {
{
vNodesDisconnected.remove(pnode); vNodesDisconnected.remove(pnode);
DeleteNode(pnode); DeleteNode(pnode);
} }

View File

@ -77,52 +77,28 @@ boost::thread_specific_ptr<LockStack> lockstack;
static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch, const LockStack& s1, const LockStack& s2) static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch, const LockStack& s1, const LockStack& s2)
{ {
// We attempt to not assert on probably-not deadlocks by assuming that
// a try lock will immediately have otherwise bailed if it had
// failed to get the lock
// We do this by, for the locks which triggered the potential deadlock,
// in either lockorder, checking that the second of the two which is locked
// is only a TRY_LOCK, ignoring locks if they are reentrant.
bool firstLocked = false;
bool secondLocked = false;
bool onlyMaybeDeadlock = false;
LogPrintf("POTENTIAL DEADLOCK DETECTED\n"); LogPrintf("POTENTIAL DEADLOCK DETECTED\n");
LogPrintf("Previous lock order was:\n"); LogPrintf("Previous lock order was:\n");
BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) {
if (i.first == mismatch.first) { if (i.first == mismatch.first) {
LogPrintf(" (1)"); LogPrintf(" (1)");
if (!firstLocked && secondLocked && i.second.fTry)
onlyMaybeDeadlock = true;
firstLocked = true;
} }
if (i.first == mismatch.second) { if (i.first == mismatch.second) {
LogPrintf(" (2)"); LogPrintf(" (2)");
if (!secondLocked && firstLocked && i.second.fTry)
onlyMaybeDeadlock = true;
secondLocked = true;
} }
LogPrintf(" %s\n", i.second.ToString()); LogPrintf(" %s\n", i.second.ToString());
} }
firstLocked = false;
secondLocked = false;
LogPrintf("Current lock order is:\n"); LogPrintf("Current lock order is:\n");
BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) {
if (i.first == mismatch.first) { if (i.first == mismatch.first) {
LogPrintf(" (1)"); LogPrintf(" (1)");
if (!firstLocked && secondLocked && i.second.fTry)
onlyMaybeDeadlock = true;
firstLocked = true;
} }
if (i.first == mismatch.second) { if (i.first == mismatch.second) {
LogPrintf(" (2)"); LogPrintf(" (2)");
if (!secondLocked && firstLocked && i.second.fTry)
onlyMaybeDeadlock = true;
secondLocked = true;
} }
LogPrintf(" %s\n", i.second.ToString()); LogPrintf(" %s\n", i.second.ToString());
} }
assert(onlyMaybeDeadlock); assert(false);
} }
static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
@ -134,21 +110,19 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
(*lockstack).push_back(std::make_pair(c, locklocation)); (*lockstack).push_back(std::make_pair(c, locklocation));
if (!fTry) { BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) {
BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { if (i.first == c)
if (i.first == c) break;
break;
std::pair<void*, void*> p1 = std::make_pair(i.first, c); std::pair<void*, void*> p1 = std::make_pair(i.first, c);
if (lockdata.lockorders.count(p1)) if (lockdata.lockorders.count(p1))
continue; continue;
lockdata.lockorders[p1] = (*lockstack); lockdata.lockorders[p1] = (*lockstack);
std::pair<void*, void*> p2 = std::make_pair(c, i.first); std::pair<void*, void*> p2 = std::make_pair(c, i.first);
lockdata.invlockorders.insert(p2); lockdata.invlockorders.insert(p2);
if (lockdata.lockorders.count(p2)) if (lockdata.lockorders.count(p2))
potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]);
}
} }
} }