From fd13eca147be80f3ebd1a5a1db1ce75bbeeb1953 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 2 Feb 2017 20:03:46 -0500 Subject: [PATCH 1/4] Lock cs_vSend and cs_inventory in a consistent order even in TRY --- src/net.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index df88b12c7..5cddc6f44 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1070,12 +1070,13 @@ void CConnman::ThreadSocketHandler() { bool fDelete = false; { - TRY_LOCK(pnode->cs_vSend, lockSend); - if (lockSend) + TRY_LOCK(pnode->cs_inventory, lockInv); + if (lockInv) { - TRY_LOCK(pnode->cs_inventory, lockInv); - if (lockInv) - fDelete = true; + TRY_LOCK(pnode->cs_vSend, lockSend); + if (lockSend) { + fDelete = true; + } } } if (fDelete) From 8465631845eac3db834942a4feb50f65c3401c68 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 2 Feb 2017 22:22:01 -0500 Subject: [PATCH 2/4] Always enforce lock strict lock ordering (try or not) --- src/sync.cpp | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index a18d0f148..25773f08e 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -77,52 +77,28 @@ boost::thread_specific_ptr lockstack; static void potential_deadlock_detected(const std::pair& 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("Previous lock order was:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) { if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (!firstLocked && secondLocked && i.second.fTry) - onlyMaybeDeadlock = true; - firstLocked = true; } if (i.first == mismatch.second) { LogPrintf(" (2)"); - if (!secondLocked && firstLocked && i.second.fTry) - onlyMaybeDeadlock = true; - secondLocked = true; } LogPrintf(" %s\n", i.second.ToString()); } - firstLocked = false; - secondLocked = false; LogPrintf("Current lock order is:\n"); BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) { if (i.first == mismatch.first) { LogPrintf(" (1)"); - if (!firstLocked && secondLocked && i.second.fTry) - onlyMaybeDeadlock = true; - firstLocked = true; } if (i.first == mismatch.second) { LogPrintf(" (2)"); - if (!secondLocked && firstLocked && i.second.fTry) - onlyMaybeDeadlock = true; - secondLocked = true; } LogPrintf(" %s\n", i.second.ToString()); } - assert(onlyMaybeDeadlock); + assert(false); } static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) From 2a962d4540a253f63803d1f145fa26b938e69633 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 4 Feb 2017 16:44:05 -0500 Subject: [PATCH 3/4] Fixup style a bit by moving { to the same line as if statements --- src/net.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 5cddc6f44..704d3b8d3 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1066,21 +1066,18 @@ void CConnman::ThreadSocketHandler() BOOST_FOREACH(CNode* pnode, vNodesDisconnectedCopy) { // wait until threads are done using it - if (pnode->GetRefCount() <= 0) - { + if (pnode->GetRefCount() <= 0) { bool fDelete = false; { TRY_LOCK(pnode->cs_inventory, lockInv); - if (lockInv) - { + if (lockInv) { TRY_LOCK(pnode->cs_vSend, lockSend); if (lockSend) { fDelete = true; } } } - if (fDelete) - { + if (fDelete) { vNodesDisconnected.remove(pnode); DeleteNode(pnode); } From 618ee9249b178d94911ea66cb4b5291f000ef1fb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 7 Feb 2017 14:15:28 -0500 Subject: [PATCH 4/4] Further-enforce lockordering by enforcing directly after TRY_LOCKs --- src/sync.cpp | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/sync.cpp b/src/sync.cpp index 25773f08e..fce57f1df 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -110,21 +110,19 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry) (*lockstack).push_back(std::make_pair(c, locklocation)); - if (!fTry) { - BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { - if (i.first == c) - break; - - std::pair p1 = std::make_pair(i.first, c); - if (lockdata.lockorders.count(p1)) - continue; - lockdata.lockorders[p1] = (*lockstack); - - std::pair p2 = std::make_pair(c, i.first); - lockdata.invlockorders.insert(p2); - if (lockdata.lockorders.count(p2)) - potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); - } + BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, (*lockstack)) { + if (i.first == c) + break; + + std::pair p1 = std::make_pair(i.first, c); + if (lockdata.lockorders.count(p1)) + continue; + lockdata.lockorders[p1] = (*lockstack); + + std::pair p2 = std::make_pair(c, i.first); + lockdata.invlockorders.insert(p2); + if (lockdata.lockorders.count(p2)) + potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]); } }