From e6c0b03e94474919952dce05ec81ac5721252e77 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 8 Mar 2024 20:36:07 +0000 Subject: [PATCH 1/4] RIPD-1848 Enforce no duplicate slots from incoming connections --- src/ripple/peerfinder/impl/Logic.h | 11 ++++++++++- 1 file changed, 10 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, From 25cdc6c041c975ea34f7138eb2663d3604c983a2 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Mon, 11 Mar 2024 13:22:01 +0000 Subject: [PATCH 2/4] RIPD-1848 Add unit test --- src/test/peerfinder/PeerFinder_test.cpp | 105 ++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/src/test/peerfinder/PeerFinder_test.cpp b/src/test/peerfinder/PeerFinder_test.cpp index b0750e689f3..5d376e834e6 100644 --- a/src/test/peerfinder/PeerFinder_test.cpp +++ b/src/test/peerfinder/PeerFinder_test.cpp @@ -24,6 +24,7 @@ #include #include #include +#include "ripple/beast/unit_test/suite.hpp" #include namespace ripple { @@ -157,6 +158,108 @@ class PeerFinder_test : public beast::unit_test::suite BEAST_EXPECT(n <= (seconds + 59) / 60); } + void + test_duplicateOutIn() + { + auto const seconds = 10000; + 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); + } + + PublicKey const pk(randomKeyPair(KeyType::secp256k1).first); + std::size_t n = 0; + + for (std::size_t i = 0; i < seconds; ++i) + { + auto const list = logic.autoconnect(); + if (!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); + } + ++n; + } + clock.advance(std::chrono::seconds(1)); + logic.once_per_second(); + } + BEAST_EXPECT(n <= (seconds + 59) / 60); + } + + void + test_duplicateInOut() + { + auto const seconds = 10000; + 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); + } + + PublicKey const pk(randomKeyPair(KeyType::secp256k1).first); + std::size_t n = 0; + + for (std::size_t i = 0; i < seconds; ++i) + { + auto const list = logic.autoconnect(); + if (!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); + } + ++n; + } + clock.advance(std::chrono::seconds(1)); + logic.once_per_second(); + } + BEAST_EXPECT(n <= (seconds + 59) / 60); + } + void test_config() { @@ -279,6 +382,8 @@ class PeerFinder_test : public beast::unit_test::suite { test_backoff1(); test_backoff2(); + test_duplicateOutIn(); + test_duplicateInOut(); test_config(); test_invalid_config(); } From 051408fab727bfac2016d301f5be271a6ec80e23 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 12 Mar 2024 19:13:27 +0000 Subject: [PATCH 3/4] Minor cleanup --- src/test/peerfinder/PeerFinder_test.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/test/peerfinder/PeerFinder_test.cpp b/src/test/peerfinder/PeerFinder_test.cpp index 5d376e834e6..0a4e39090fd 100644 --- a/src/test/peerfinder/PeerFinder_test.cpp +++ b/src/test/peerfinder/PeerFinder_test.cpp @@ -20,11 +20,11 @@ #include #include #include +#include #include #include #include #include -#include "ripple/beast/unit_test/suite.hpp" #include namespace ripple { @@ -177,9 +177,7 @@ class PeerFinder_test : public beast::unit_test::suite logic.config(c); } - PublicKey const pk(randomKeyPair(KeyType::secp256k1).first); std::size_t n = 0; - for (std::size_t i = 0; i < seconds; ++i) { auto const list = logic.autoconnect(); @@ -228,9 +226,7 @@ class PeerFinder_test : public beast::unit_test::suite logic.config(c); } - PublicKey const pk(randomKeyPair(KeyType::secp256k1).first); std::size_t n = 0; - for (std::size_t i = 0; i < seconds; ++i) { auto const list = logic.autoconnect(); From 5a7f9e4c80036f05ce91f87862f27da18bfa3618 Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Tue, 12 Mar 2024 19:45:50 +0000 Subject: [PATCH 4/4] Remove unnecessary loop from test --- src/test/peerfinder/PeerFinder_test.cpp | 82 ++++++++++--------------- 1 file changed, 32 insertions(+), 50 deletions(-) diff --git a/src/test/peerfinder/PeerFinder_test.cpp b/src/test/peerfinder/PeerFinder_test.cpp index 0a4e39090fd..daa316e566c 100644 --- a/src/test/peerfinder/PeerFinder_test.cpp +++ b/src/test/peerfinder/PeerFinder_test.cpp @@ -161,7 +161,6 @@ class PeerFinder_test : public beast::unit_test::suite void test_duplicateOutIn() { - auto const seconds = 10000; testcase("duplicate out/in"); TestStore store; TestChecker checker; @@ -177,40 +176,31 @@ class PeerFinder_test : public beast::unit_test::suite logic.config(c); } - std::size_t n = 0; - for (std::size_t i = 0; i < seconds; ++i) + auto const list = logic.autoconnect(); + if (BEAST_EXPECT(!list.empty())) { - auto const list = logic.autoconnect(); - if (!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(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); - } - ++n; + 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); } - clock.advance(std::chrono::seconds(1)); - logic.once_per_second(); } - BEAST_EXPECT(n <= (seconds + 59) / 60); } void test_duplicateInOut() { - auto const seconds = 10000; testcase("duplicate in/out"); TestStore store; TestChecker checker; @@ -226,34 +216,26 @@ class PeerFinder_test : public beast::unit_test::suite logic.config(c); } - std::size_t n = 0; - for (std::size_t i = 0; i < seconds; ++i) + auto const list = logic.autoconnect(); + if (BEAST_EXPECT(!list.empty())) { - auto const list = logic.autoconnect(); - if (!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(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); - } - ++n; + 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); } - clock.advance(std::chrono::seconds(1)); - logic.once_per_second(); } - BEAST_EXPECT(n <= (seconds + 59) / 60); } void