From 9e48fc0c834e8a6e340c521e9ec58b97b944c1fd Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 6 Nov 2024 22:22:42 +0000 Subject: [PATCH] Fix potential deadlock (#5124) * 2.2.2 changed functions acquireAsync and NetworkOPsImp::recvValidation to add an item to a collection under lock, unlock, do some work, then lock again to do remove the item. It will deadlock if an exception is thrown while adding the item - before unlocking. * Replace ScopedUnlock with scope_unlock. --- include/xrpl/basics/scope.h | 65 ++++++++++++++ .../app/ledger/detail/InboundLedgers.cpp | 5 +- src/xrpld/app/ledger/detail/LedgerMaster.cpp | 90 ++----------------- src/xrpld/app/misc/NetworkOPs.cpp | 6 +- 4 files changed, 77 insertions(+), 89 deletions(-) diff --git a/include/xrpl/basics/scope.h b/include/xrpl/basics/scope.h index 54c05998fa4..b1d13eae8ce 100644 --- a/include/xrpl/basics/scope.h +++ b/include/xrpl/basics/scope.h @@ -21,6 +21,7 @@ #define RIPPLE_BASICS_SCOPE_H_INCLUDED #include +#include #include #include @@ -186,6 +187,70 @@ class scope_success template scope_success(EF) -> scope_success; +/** + Automatically unlocks and re-locks a unique_lock object. + + This is the reverse of a std::unique_lock object - instead of locking the + mutex for the lifetime of this object, it unlocks it. + + Make sure you don't try to unlock mutexes that aren't actually locked! + + This is essentially a less-versatile boost::reverse_lock. + + e.g. @code + + std::mutex mut; + + for (;;) + { + std::unique_lock myScopedLock{mut}; + // mut is now locked + + ... do some stuff with it locked .. + + while (xyz) + { + ... do some stuff with it locked .. + + scope_unlock unlocker{myScopedLock}; + + // mut is now unlocked for the remainder of this block, + // and re-locked at the end. + + ...do some stuff with it unlocked ... + } // mut gets locked here. + + } // mut gets unlocked here + @endcode +*/ + +template +class scope_unlock +{ + std::unique_lock* plock; + +public: + explicit scope_unlock(std::unique_lock& lock) noexcept(true) + : plock(&lock) + { + assert(plock->owns_lock()); + plock->unlock(); + } + + // Immovable type + scope_unlock(scope_unlock const&) = delete; + scope_unlock& + operator=(scope_unlock const&) = delete; + + ~scope_unlock() noexcept(true) + { + plock->lock(); + } +}; + +template +scope_unlock(std::unique_lock&) -> scope_unlock; + } // namespace ripple #endif diff --git a/src/xrpld/app/ledger/detail/InboundLedgers.cpp b/src/xrpld/app/ledger/detail/InboundLedgers.cpp index 72eb9e27189..f6d86a4d737 100644 --- a/src/xrpld/app/ledger/detail/InboundLedgers.cpp +++ b/src/xrpld/app/ledger/detail/InboundLedgers.cpp @@ -25,9 +25,11 @@ #include #include #include +#include #include #include #include + #include #include #include @@ -139,7 +141,7 @@ class InboundLedgersImp : public InboundLedgers if (pendingAcquires_.contains(hash)) return; pendingAcquires_.insert(hash); - lock.unlock(); + scope_unlock unlock(lock); acquire(hash, seq, reason); } catch (std::exception const& e) @@ -154,7 +156,6 @@ class InboundLedgersImp : public InboundLedgers << "Unknown exception thrown for acquiring new inbound ledger " << hash; } - lock.lock(); pendingAcquires_.erase(hash); } diff --git a/src/xrpld/app/ledger/detail/LedgerMaster.cpp b/src/xrpld/app/ledger/detail/LedgerMaster.cpp index d1eeabeb619..53edef17d33 100644 --- a/src/xrpld/app/ledger/detail/LedgerMaster.cpp +++ b/src/xrpld/app/ledger/detail/LedgerMaster.cpp @@ -46,10 +46,12 @@ #include #include #include +#include #include #include #include #include + #include #include #include @@ -60,86 +62,6 @@ namespace ripple { -namespace { - -//============================================================================== -/** - Automatically unlocks and re-locks a unique_lock object. - - This is the reverse of a std::unique_lock object - instead of locking the - mutex for the lifetime of this object, it unlocks it. - - Make sure you don't try to unlock mutexes that aren't actually locked! - - This is essentially a less-versatile boost::reverse_lock. - - e.g. @code - - std::mutex mut; - - for (;;) - { - std::unique_lock myScopedLock{mut}; - // mut is now locked - - ... do some stuff with it locked .. - - while (xyz) - { - ... do some stuff with it locked .. - - ScopedUnlock unlocker{myScopedLock}; - - // mut is now unlocked for the remainder of this block, - // and re-locked at the end. - - ...do some stuff with it unlocked ... - } // mut gets locked here. - - } // mut gets unlocked here - @endcode -*/ -template -class ScopedUnlock -{ - std::unique_lock& lock_; - -public: - /** Creates a ScopedUnlock. - - As soon as it is created, this will unlock the unique_lock, and - when the ScopedLock object is deleted, the unique_lock will - be re-locked. - - Make sure this object is created and deleted by the same thread, - otherwise there are no guarantees what will happen! Best just to use it - as a local stack object, rather than creating on the heap. - */ - explicit ScopedUnlock(std::unique_lock& lock) : lock_(lock) - { - assert(lock_.owns_lock()); - lock_.unlock(); - } - - ScopedUnlock(ScopedUnlock const&) = delete; - ScopedUnlock& - operator=(ScopedUnlock const&) = delete; - - /** Destructor. - - The unique_lock will be locked after the destructor is called. - - Make sure this object is created and deleted by the same thread, - otherwise there are no guarantees what will happen! - */ - ~ScopedUnlock() noexcept(false) - { - lock_.lock(); - } -}; - -} // namespace - // Don't catch up more than 100 ledgers (cannot exceed 256) static constexpr int MAX_LEDGER_GAP{100}; @@ -1336,7 +1258,7 @@ LedgerMaster::findNewLedgersToPublish( auto valLedger = mValidLedger.get(); std::uint32_t valSeq = valLedger->info().seq; - ScopedUnlock sul{sl}; + scope_unlock sul{sl}; try { for (std::uint32_t seq = pubSeq; seq <= valSeq; ++seq) @@ -1882,7 +1804,7 @@ LedgerMaster::fetchForHistory( InboundLedger::Reason reason, std::unique_lock& sl) { - ScopedUnlock sul{sl}; + scope_unlock sul{sl}; if (auto hash = getLedgerHashForHistory(missing, reason)) { assert(hash->isNonZero()); @@ -2052,7 +1974,7 @@ LedgerMaster::doAdvance(std::unique_lock& sl) for (auto const& ledger : pubLedgers) { { - ScopedUnlock sul{sl}; + scope_unlock sul{sl}; JLOG(m_journal.debug()) << "tryAdvance publishing seq " << ledger->info().seq; setFullLedger(ledger, true, true); @@ -2061,7 +1983,7 @@ LedgerMaster::doAdvance(std::unique_lock& sl) setPubLedger(ledger); { - ScopedUnlock sul{sl}; + scope_unlock sul{sl}; app_.getOPs().pubLedger(ledger); } } diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 46a7dfcaacd..d647df91f1e 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -53,6 +53,7 @@ #include #include #include +#include #include #include #include @@ -2310,7 +2311,7 @@ NetworkOPsImp::recvValidation( bypassAccept = BypassAccept::yes; else pendingValidations_.insert(val->getLedgerHash()); - lock.unlock(); + scope_unlock unlock(lock); handleNewValidation(app_, val, source, bypassAccept, m_journal); } catch (std::exception const& e) @@ -2327,10 +2328,9 @@ NetworkOPsImp::recvValidation( } if (bypassAccept == BypassAccept::no) { - lock.lock(); pendingValidations_.erase(val->getLedgerHash()); - lock.unlock(); } + lock.unlock(); pubValidation(val);