From 64e46878e096e9f3da67e963c2fe4a0c0f4613d7 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Sat, 23 Mar 2024 03:08:16 +0900 Subject: [PATCH] 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 | 11 +++- src/test/peerfinder/PeerFinder_test.cpp | 83 +++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/src/ripple/peerfinder/impl/Logic.h b/src/ripple/peerfinder/impl/Logic.h index dc325cc480d..7a2b6a7543a 100644 --- a/src/ripple/peerfinder/impl/Logic.h +++ b/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( local_endpoint, diff --git a/src/test/peerfinder/PeerFinder_test.cpp b/src/test/peerfinder/PeerFinder_test.cpp index b0750e689f3..daa316e566c 100644 --- a/src/test/peerfinder/PeerFinder_test.cpp +++ b/src/test/peerfinder/PeerFinder_test.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -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 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 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(); }