Enforce no duplicate slots from incoming connections: (#4944)

We do not currently enforce that incoming peer connection does not have
remote_endpoint which is already used (either by incoming or outgoing
connection), hence already stored in slots_. If we happen to receive a
connection from such a duplicate remote_endpoint, it will eventually result in a
crash (when disconnecting) or weird behavior (when updating slot state), as a
result of an apparently matching remote_endpoint in slots_ being used by a
different connection.

src/ripple/peerfinder/impl/Logic.h

@@ -273,7 +273,7 @@ class Logic
 
         std::lock_guard _(lock_);
 
-        // Check for duplicate connection
+        // Check for connection limit per address
         if (is_public(remote_endpoint))
         {
             auto const count =
@@ -287,6 +287,15 @@ class Logic
             }
         }
 
+        // Check for duplicate connection
+        if (slots_.find(remote_endpoint) != slots_.end())
+        {
+            JLOG(m_journal.debug())
+                << beast::leftw(18) << "Logic dropping " << remote_endpoint
+                << " as duplicate incoming";
+            return SlotImp::ptr();
+        }
+
         // Create the slot
         SlotImp::ptr const slot(std::make_shared<SlotImp>(
             local_endpoint,

src/test/peerfinder/PeerFinder_test.cpp

@@ -20,6 +20,7 @@
 #include <ripple/basics/Slice.h>
 #include <ripple/basics/chrono.h>
 #include <ripple/beast/unit_test.h>
+#include <ripple/beast/unit_test/suite.hpp>
 #include <ripple/core/Config.h>
 #include <ripple/peerfinder/impl/Logic.h>
 #include <ripple/protocol/PublicKey.h>
@@ -157,6 +158,86 @@ class PeerFinder_test : public beast::unit_test::suite
         BEAST_EXPECT(n <= (seconds + 59) / 60);
     }
 
+    void
+    test_duplicateOutIn()
+    {
+        testcase("duplicate out/in");
+        TestStore store;
+        TestChecker checker;
+        TestStopwatch clock;
+        Logic<TestChecker> logic(clock, store, checker, journal_);
+        logic.addFixedPeer(
+            "test", beast::IP::Endpoint::from_string("65.0.0.1:5"));
+        {
+            Config c;
+            c.autoConnect = false;
+            c.listeningPort = 1024;
+            c.ipLimit = 2;
+            logic.config(c);
+        }
+
+        auto const list = logic.autoconnect();
+        if (BEAST_EXPECT(!list.empty()))
+        {
+            BEAST_EXPECT(list.size() == 1);
+            auto const remote = list.front();
+            auto const slot1 = logic.new_outbound_slot(remote);
+            if (BEAST_EXPECT(slot1 != nullptr))
+            {
+                BEAST_EXPECT(
+                    logic.connectedAddresses_.count(remote.address()) == 1);
+                auto const local =
+                    beast::IP::Endpoint::from_string("65.0.0.2:1024");
+                auto const slot2 = logic.new_inbound_slot(local, remote);
+                BEAST_EXPECT(
+                    logic.connectedAddresses_.count(remote.address()) == 1);
+                if (!BEAST_EXPECT(slot2 == nullptr))
+                    logic.on_closed(slot2);
+                logic.on_closed(slot1);
+            }
+        }
+    }
+
+    void
+    test_duplicateInOut()
+    {
+        testcase("duplicate in/out");
+        TestStore store;
+        TestChecker checker;
+        TestStopwatch clock;
+        Logic<TestChecker> logic(clock, store, checker, journal_);
+        logic.addFixedPeer(
+            "test", beast::IP::Endpoint::from_string("65.0.0.1:5"));
+        {
+            Config c;
+            c.autoConnect = false;
+            c.listeningPort = 1024;
+            c.ipLimit = 2;
+            logic.config(c);
+        }
+
+        auto const list = logic.autoconnect();
+        if (BEAST_EXPECT(!list.empty()))
+        {
+            BEAST_EXPECT(list.size() == 1);
+            auto const remote = list.front();
+            auto const local =
+                beast::IP::Endpoint::from_string("65.0.0.2:1024");
+            auto const slot1 = logic.new_inbound_slot(local, remote);
+            if (BEAST_EXPECT(slot1 != nullptr))
+            {
+                BEAST_EXPECT(
+                    logic.connectedAddresses_.count(remote.address()) == 1);
+                auto const slot2 = logic.new_outbound_slot(remote);
+                BEAST_EXPECT(
+                    logic.connectedAddresses_.count(remote.address()) == 1);
+                if (!BEAST_EXPECT(slot2 == nullptr))
+                    logic.on_closed(slot2);
+                logic.on_closed(slot1);
+            }
+        }
+    }
+
     void
     test_config()
     {
@@ -279,6 +360,8 @@ class PeerFinder_test : public beast::unit_test::suite
     {
         test_backoff1();
         test_backoff2();
+        test_duplicateOutIn();
+        test_duplicateInOut();
         test_config();
         test_invalid_config();
     }