From 5ac1a51ee5a57da59f1ff1986b7d9054484d3c80 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 25 May 2021 15:01:53 +0200 Subject: [PATCH 1/6] i2p: avoid using Sock::Get() for checking for a valid socket Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty unique_ptr to denote that there is no valid socket. --- src/i2p.cpp | 10 ++++------ src/i2p.h | 1 + 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/i2p.cpp b/src/i2p.cpp index f03e375adfa..5a3dde54ced 100644 --- a/src/i2p.cpp +++ b/src/i2p.cpp @@ -119,7 +119,6 @@ Session::Session(const fs::path& private_key_file, : m_private_key_file{private_key_file}, m_control_host{control_host}, m_interrupt{interrupt}, - m_control_sock{std::make_unique(INVALID_SOCKET)}, m_transient{false} { } @@ -127,7 +126,6 @@ Session::Session(const fs::path& private_key_file, Session::Session(const CService& control_host, CThreadInterrupt* interrupt) : m_control_host{control_host}, m_interrupt{interrupt}, - m_control_sock{std::make_unique(INVALID_SOCKET)}, m_transient{true} { } @@ -315,7 +313,7 @@ void Session::CheckControlSock() LOCK(m_mutex); std::string errmsg; - if (!m_control_sock->IsConnected(errmsg)) { + if (m_control_sock && !m_control_sock->IsConnected(errmsg)) { Log("Control socket error: %s", errmsg); Disconnect(); } @@ -364,7 +362,7 @@ Binary Session::MyDestination() const void Session::CreateIfNotCreatedAlready() { std::string errmsg; - if (m_control_sock->IsConnected(errmsg)) { + if (m_control_sock && m_control_sock->IsConnected(errmsg)) { return; } @@ -437,14 +435,14 @@ std::unique_ptr Session::StreamAccept() void Session::Disconnect() { - if (m_control_sock->Get() != INVALID_SOCKET) { + if (m_control_sock) { if (m_session_id.empty()) { Log("Destroying incomplete SAM session"); } else { Log("Destroying SAM session %s", m_session_id); } + m_control_sock.reset(); } - m_control_sock = std::make_unique(INVALID_SOCKET); m_session_id.clear(); } } // namespace sam diff --git a/src/i2p.h b/src/i2p.h index c9c99292d97..cb9da64816f 100644 --- a/src/i2p.h +++ b/src/i2p.h @@ -261,6 +261,7 @@ private: * ("SESSION CREATE"). With the established session id we later open * other connections to the SAM service to accept incoming I2P * connections and make outgoing ones. + * If not connected then this unique_ptr will be empty. * See https://geti2p.net/en/docs/api/samv3 */ std::unique_ptr m_control_sock GUARDED_BY(m_mutex); From aeac68d036e3cff57ce155f1a904d77f98b357d4 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 25 May 2021 15:05:26 +0200 Subject: [PATCH 2/6] net: don't check if the socket is valid in GetBindAddress() The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `GetBindAddress()` is called. --- src/net.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 53a2dcf1258..cf19274e349 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -427,12 +427,10 @@ static CAddress GetBindAddress(const Sock& sock) CAddress addr_bind; struct sockaddr_storage sockaddr_bind; socklen_t sockaddr_bind_len = sizeof(sockaddr_bind); - if (sock.Get() != INVALID_SOCKET) { - if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) { - addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind); - } else { - LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n"); - } + if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) { + addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind); + } else { + LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n"); } return addr_bind; } From 944b21b70ae490a5a746bcc1810a5074d74e9d34 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 25 May 2021 15:25:40 +0200 Subject: [PATCH 3/6] net: don't check if the socket is valid in ConnectSocketDirectly() The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `ConnectSocketDirectly()` is called. --- src/netbase.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/netbase.cpp b/src/netbase.cpp index a8419217f46..ca1a80d72f7 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -514,10 +514,6 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT // Create a sockaddr from the specified service. struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); - if (sock.Get() == INVALID_SOCKET) { - LogPrintf("Cannot connect to %s: invalid socket\n", addrConnect.ToStringAddrPort()); - return false; - } if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) { LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToStringAddrPort()); return false; From 7829272f7826511241defd34954e6040ea963f07 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 29 Apr 2021 18:01:03 +0200 Subject: [PATCH 4/6] net: remove now unnecessary Sock::Get() `Sock::Get()` was used only in `sock.{cpp,h}`. Remove it and access `Sock::m_socket` directly. Unit tests that used `Get()` to test for equality still verify that the behavior is correct by using the added `operator==()`. --- src/test/sock_tests.cpp | 6 +++--- src/util/sock.cpp | 7 +++++-- src/util/sock.h | 32 +++++++++++++++----------------- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp index 26ee724bf80..38a4804fcf4 100644 --- a/src/test/sock_tests.cpp +++ b/src/test/sock_tests.cpp @@ -38,7 +38,7 @@ BOOST_AUTO_TEST_CASE(constructor_and_destructor) { const SOCKET s = CreateSocket(); Sock* sock = new Sock(s); - BOOST_CHECK_EQUAL(sock->Get(), s); + BOOST_CHECK(*sock == s); BOOST_CHECK(!SocketIsClosed(s)); delete sock; BOOST_CHECK(SocketIsClosed(s)); @@ -51,7 +51,7 @@ BOOST_AUTO_TEST_CASE(move_constructor) Sock* sock2 = new Sock(std::move(*sock1)); delete sock1; BOOST_CHECK(!SocketIsClosed(s)); - BOOST_CHECK_EQUAL(sock2->Get(), s); + BOOST_CHECK(*sock2 == s); delete sock2; BOOST_CHECK(SocketIsClosed(s)); } @@ -64,7 +64,7 @@ BOOST_AUTO_TEST_CASE(move_assignment) *sock2 = std::move(*sock1); delete sock1; BOOST_CHECK(!SocketIsClosed(s)); - BOOST_CHECK_EQUAL(sock2->Get(), s); + BOOST_CHECK(*sock2 == s); delete sock2; BOOST_CHECK(SocketIsClosed(s)); } diff --git a/src/util/sock.cpp b/src/util/sock.cpp index fd64cae404b..e08edd42b74 100644 --- a/src/util/sock.cpp +++ b/src/util/sock.cpp @@ -44,8 +44,6 @@ Sock& Sock::operator=(Sock&& other) return *this; } -SOCKET Sock::Get() const { return m_socket; } - ssize_t Sock::Send(const void* data, size_t len, int flags) const { return send(m_socket, static_cast(data), len, flags); @@ -411,6 +409,11 @@ void Sock::Close() m_socket = INVALID_SOCKET; } +bool Sock::operator==(SOCKET s) const +{ + return m_socket == s; +}; + std::string NetworkErrorString(int err) { #if defined(WIN32) diff --git a/src/util/sock.h b/src/util/sock.h index 6bac2dfd346..a3d8dfe3b63 100644 --- a/src/util/sock.h +++ b/src/util/sock.h @@ -21,8 +21,7 @@ static constexpr auto MAX_WAIT_FOR_IO = 1s; /** - * RAII helper class that manages a socket. Mimics `std::unique_ptr`, but instead of a pointer it - * contains a socket and closes it automatically when it goes out of scope. + * RAII helper class that manages a socket and closes it automatically when it goes out of scope. */ class Sock { @@ -63,43 +62,37 @@ public: virtual Sock& operator=(Sock&& other); /** - * Get the value of the contained socket. - * @return socket or INVALID_SOCKET if empty - */ - [[nodiscard]] virtual SOCKET Get() const; - - /** - * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this + * send(2) wrapper. Equivalent to `send(m_socket, data, len, flags);`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual ssize_t Send(const void* data, size_t len, int flags) const; /** - * recv(2) wrapper. Equivalent to `recv(this->Get(), buf, len, flags);`. Code that uses this + * recv(2) wrapper. Equivalent to `recv(m_socket, buf, len, flags);`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual ssize_t Recv(void* buf, size_t len, int flags) const; /** - * connect(2) wrapper. Equivalent to `connect(this->Get(), addr, addrlen)`. Code that uses this + * connect(2) wrapper. Equivalent to `connect(m_socket, addr, addrlen)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int Connect(const sockaddr* addr, socklen_t addr_len) const; /** - * bind(2) wrapper. Equivalent to `bind(this->Get(), addr, addr_len)`. Code that uses this + * bind(2) wrapper. Equivalent to `bind(m_socket, addr, addr_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int Bind(const sockaddr* addr, socklen_t addr_len) const; /** - * listen(2) wrapper. Equivalent to `listen(this->Get(), backlog)`. Code that uses this + * listen(2) wrapper. Equivalent to `listen(m_socket, backlog)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int Listen(int backlog) const; /** - * accept(2) wrapper. Equivalent to `std::make_unique(accept(this->Get(), addr, addr_len))`. + * accept(2) wrapper. Equivalent to `std::make_unique(accept(m_socket, addr, addr_len))`. * Code that uses this wrapper can be unit tested if this method is overridden by a mock Sock * implementation. * The returned unique_ptr is empty if `accept()` failed in which case errno will be set. @@ -108,7 +101,7 @@ public: /** * getsockopt(2) wrapper. Equivalent to - * `getsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this + * `getsockopt(m_socket, level, opt_name, opt_val, opt_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int GetSockOpt(int level, @@ -118,7 +111,7 @@ public: /** * setsockopt(2) wrapper. Equivalent to - * `setsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this + * `setsockopt(m_socket, level, opt_name, opt_val, opt_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int SetSockOpt(int level, @@ -128,7 +121,7 @@ public: /** * getsockname(2) wrapper. Equivalent to - * `getsockname(this->Get(), name, name_len)`. Code that uses this + * `getsockname(m_socket, name, name_len)`. Code that uses this * wrapper can be unit tested if this method is overridden by a mock Sock implementation. */ [[nodiscard]] virtual int GetSockName(sockaddr* name, socklen_t* name_len) const; @@ -266,6 +259,11 @@ public: */ [[nodiscard]] virtual bool IsConnected(std::string& errmsg) const; + /** + * Check if the internal socket is equal to `s`. Use only in tests. + */ + bool operator==(SOCKET s) const; + protected: /** * Contained socket. `INVALID_SOCKET` designates the object is empty. From 5086a99b84367a45706af7197da1016dd966e6d9 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Fri, 28 May 2021 15:37:43 +0200 Subject: [PATCH 5/6] net: remove Sock default constructor, it's not necessary --- src/test/fuzz/util/net.cpp | 5 +++-- src/test/sock_tests.cpp | 4 ++-- src/test/util/net.h | 11 ++++++++--- src/util/sock.cpp | 2 -- src/util/sock.h | 5 +---- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/test/fuzz/util/net.cpp b/src/test/fuzz/util/net.cpp index 65bc336297b..72e61b7e22b 100644 --- a/src/test/fuzz/util/net.cpp +++ b/src/test/fuzz/util/net.cpp @@ -56,9 +56,10 @@ CAddress ConsumeAddress(FuzzedDataProvider& fuzzed_data_provider) noexcept } FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider) - : m_fuzzed_data_provider{fuzzed_data_provider}, m_selectable{fuzzed_data_provider.ConsumeBool()} + : Sock{fuzzed_data_provider.ConsumeIntegralInRange(INVALID_SOCKET - 1, INVALID_SOCKET)}, + m_fuzzed_data_provider{fuzzed_data_provider}, + m_selectable{fuzzed_data_provider.ConsumeBool()} { - m_socket = fuzzed_data_provider.ConsumeIntegralInRange(INVALID_SOCKET - 1, INVALID_SOCKET); } FuzzedSock::~FuzzedSock() diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp index 38a4804fcf4..9641831e196 100644 --- a/src/test/sock_tests.cpp +++ b/src/test/sock_tests.cpp @@ -60,7 +60,7 @@ BOOST_AUTO_TEST_CASE(move_assignment) { const SOCKET s = CreateSocket(); Sock* sock1 = new Sock(s); - Sock* sock2 = new Sock(); + Sock* sock2 = new Sock(INVALID_SOCKET); *sock2 = std::move(*sock1); delete sock1; BOOST_CHECK(!SocketIsClosed(s)); @@ -98,7 +98,7 @@ BOOST_AUTO_TEST_CASE(send_and_receive) SendAndRecvMessage(*sock0, *sock1); Sock* sock0moved = new Sock(std::move(*sock0)); - Sock* sock1moved = new Sock(); + Sock* sock1moved = new Sock(INVALID_SOCKET); *sock1moved = std::move(*sock1); delete sock0; diff --git a/src/test/util/net.h b/src/test/util/net.h index b2f6ebb1637..403d0ed0eae 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -106,10 +106,10 @@ constexpr auto ALL_NETWORKS = std::array{ class StaticContentsSock : public Sock { public: - explicit StaticContentsSock(const std::string& contents) : m_contents{contents} + explicit StaticContentsSock(const std::string& contents) + : Sock{INVALID_SOCKET}, + m_contents{contents} { - // Just a dummy number that is not INVALID_SOCKET. - m_socket = INVALID_SOCKET - 1; } ~StaticContentsSock() override { m_socket = INVALID_SOCKET; } @@ -192,6 +192,11 @@ public: return true; } + bool IsConnected(std::string&) const override + { + return true; + } + private: const std::string m_contents; mutable size_t m_consumed{0}; diff --git a/src/util/sock.cpp b/src/util/sock.cpp index e08edd42b74..d16dc56aa3d 100644 --- a/src/util/sock.cpp +++ b/src/util/sock.cpp @@ -24,8 +24,6 @@ static inline bool IOErrorIsPermanent(int err) return err != WSAEAGAIN && err != WSAEINTR && err != WSAEWOULDBLOCK && err != WSAEINPROGRESS; } -Sock::Sock() : m_socket(INVALID_SOCKET) {} - Sock::Sock(SOCKET s) : m_socket(s) {} Sock::Sock(Sock&& other) diff --git a/src/util/sock.h b/src/util/sock.h index a3d8dfe3b63..d78e01929bc 100644 --- a/src/util/sock.h +++ b/src/util/sock.h @@ -26,10 +26,7 @@ static constexpr auto MAX_WAIT_FOR_IO = 1s; class Sock { public: - /** - * Default constructor, creates an empty object that does nothing when destroyed. - */ - Sock(); + Sock() = delete; /** * Take ownership of an existent socket. From 7df450836969b81e98322c9a09c08b35d1095a25 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Thu, 24 Aug 2023 15:20:53 +0200 Subject: [PATCH 6/6] test: improve sock_tests/move_assignment Use also a socket for the moved-to object and check which one is closed when. --- src/test/sock_tests.cpp | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/src/test/sock_tests.cpp b/src/test/sock_tests.cpp index 9641831e196..5dd73dc1016 100644 --- a/src/test/sock_tests.cpp +++ b/src/test/sock_tests.cpp @@ -58,15 +58,27 @@ BOOST_AUTO_TEST_CASE(move_constructor) BOOST_AUTO_TEST_CASE(move_assignment) { - const SOCKET s = CreateSocket(); - Sock* sock1 = new Sock(s); - Sock* sock2 = new Sock(INVALID_SOCKET); + const SOCKET s1 = CreateSocket(); + const SOCKET s2 = CreateSocket(); + Sock* sock1 = new Sock(s1); + Sock* sock2 = new Sock(s2); + + BOOST_CHECK(!SocketIsClosed(s1)); + BOOST_CHECK(!SocketIsClosed(s2)); + *sock2 = std::move(*sock1); + BOOST_CHECK(!SocketIsClosed(s1)); + BOOST_CHECK(SocketIsClosed(s2)); + BOOST_CHECK(*sock2 == s1); + delete sock1; - BOOST_CHECK(!SocketIsClosed(s)); - BOOST_CHECK(*sock2 == s); + BOOST_CHECK(!SocketIsClosed(s1)); + BOOST_CHECK(SocketIsClosed(s2)); + BOOST_CHECK(*sock2 == s1); + delete sock2; - BOOST_CHECK(SocketIsClosed(s)); + BOOST_CHECK(SocketIsClosed(s1)); + BOOST_CHECK(SocketIsClosed(s2)); } #ifndef WIN32 // Windows does not have socketpair(2).