Fix crash inside OverladImpl loops

XRPLF · Jul 24, 2024 · f273f4d · f273f4d
1 parent ad14d09
commit f273f4d
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 4 deletions.
diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp
@@ -1157,13 +1157,13 @@ OverlayImpl::getActivePeers(
     std::size_t& enabledInSkip) const
 {
     Overlay::PeerSequence ret;
-    std::lock_guard lock(mutex_);
+    std::unique_lock lock(mutex_);
 
     active = ids_.size();
     disabled = enabledInSkip = 0;
     ret.reserve(ids_.size());
 
-    for (auto& [id, w] : ids_)
+    for (auto& [id, w] : copyIds(lock))
     {
         if (auto p = w.lock())
         {
@@ -1204,8 +1204,8 @@ OverlayImpl::findPeerByShortID(Peer::id_t const& id) const
 std::shared_ptr<Peer>
 OverlayImpl::findPeerByPublicKey(PublicKey const& pubKey)
 {
-    std::lock_guard lock(mutex_);
-    for (auto const& e : ids_)
+    std::unique_lock lock(mutex_);
+    for (auto const& e : copyIds(lock))
     {
         if (auto peer = e.second.lock())
         {

diff --git a/src/xrpld/overlay/detail/OverlayImpl.h b/src/xrpld/overlay/detail/OverlayImpl.h
@@ -627,6 +627,23 @@ class OverlayImpl : public Overlay, public reduce_relay::SquelchHandler
         }
         m_stats.peerDisconnects = getPeerDisconnect();
     }
+
+    // Use this function to replace ids_ in a buggy loop similar to this:
+    //
+    // std::unique_lock lock(mutex_);
+    // for (auto p : ids_)
+    // {
+    //     if (auto p = w.lock())
+    //     {
+    //         // . . .
+    //     } // PeerImp owned by p may be destroyed, invalidating the iterator
+    // }
+    auto
+    copyIds(std::unique_lock<std::recursive_mutex>&) const
+    {
+        return std::vector<decltype(ids_)::value_type>(
+            ids_.begin(), ids_.end());
+    }
 };
 
 }  // namespace ripple