From 184e56d6683d05fc84f5153cfff83a2e32883556 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 13 Apr 2021 12:31:49 +0200 Subject: net: add new method Sock::SetSockOpt() that wraps setsockopt() This will help to increase `Sock` usage and make more code mockable. --- src/test/fuzz/util.cpp | 13 +++++++++++++ src/test/fuzz/util.h | 2 ++ src/test/util/net.h | 2 ++ src/util/sock.cpp | 5 +++++ src/util/sock.h | 10 ++++++++++ 5 files changed, 32 insertions(+) (limited to 'src') diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index 6766fbf2d9..033c6e18d5 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -193,6 +193,19 @@ int FuzzedSock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* op return 0; } +int FuzzedSock::SetSockOpt(int, int, const void*, socklen_t) const +{ + constexpr std::array setsockopt_errnos{ + ENOMEM, + ENOBUFS, + }; + if (m_fuzzed_data_provider.ConsumeBool()) { + SetFuzzedErrNo(m_fuzzed_data_provider, setsockopt_errnos); + return -1; + } + return 0; +} + bool FuzzedSock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const { constexpr std::array wait_errnos{ diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 6c91844633..580105e442 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -68,6 +68,8 @@ public: int GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) const override; + int SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const override; + bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override; bool IsConnected(std::string& errmsg) const override; diff --git a/src/test/util/net.h b/src/test/util/net.h index 20c45058a1..e980fe4967 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -150,6 +150,8 @@ public: return 0; } + int SetSockOpt(int, int, const void*, socklen_t) const override { return 0; } + bool Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred = nullptr) const override diff --git a/src/util/sock.cpp b/src/util/sock.cpp index 2029d70a37..b5c1e28294 100644 --- a/src/util/sock.cpp +++ b/src/util/sock.cpp @@ -105,6 +105,11 @@ int Sock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len) return getsockopt(m_socket, level, opt_name, static_cast(opt_val), opt_len); } +int Sock::SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const +{ + return setsockopt(m_socket, level, opt_name, static_cast(opt_val), opt_len); +} + bool Sock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const { #ifdef USE_POLL diff --git a/src/util/sock.h b/src/util/sock.h index 7510482857..dd2913a66c 100644 --- a/src/util/sock.h +++ b/src/util/sock.h @@ -115,6 +115,16 @@ public: void* opt_val, socklen_t* opt_len) const; + /** + * setsockopt(2) wrapper. Equivalent to + * `setsockopt(this->Get(), 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, + int opt_name, + const void* opt_val, + socklen_t opt_len) const; + using Event = uint8_t; /** -- cgit v1.2.3 From d65b6c3fb9cdd41fa53bc76a7b8f49aaa089b0bc Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 13 Apr 2021 13:28:10 +0200 Subject: net: use Sock::SetSockOpt() instead of setsockopt() --- src/net.cpp | 15 ++++++++++++--- src/netbase.cpp | 17 ++++++++++------- 2 files changed, 22 insertions(+), 10 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index 602d56ab98..9a2c565b7d 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2395,17 +2395,26 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError, // Allow binding if the port is still in TIME_WAIT state after // the program was closed and restarted. - setsockopt(sock->Get(), SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int)); + if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) { + strError = strprintf(Untranslated("Error setting SO_REUSEADDR on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError())); + LogPrintf("%s\n", strError.original); + } // some systems don't have IPV6_V6ONLY but are always v6only; others do have the option // and enable it by default or not. Try to enable it, if possible. if (addrBind.IsIPv6()) { #ifdef IPV6_V6ONLY - setsockopt(sock->Get(), IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int)); + if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) { + strError = strprintf(Untranslated("Error setting IPV6_V6ONLY on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError())); + LogPrintf("%s\n", strError.original); + } #endif #ifdef WIN32 int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED; - setsockopt(sock->Get(), IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int)); + if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int)) == SOCKET_ERROR) { + strError = strprintf(Untranslated("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway"), NetworkErrorString(WSAGetLastError())); + LogPrintf("%s\n", strError.original); + } #endif } diff --git a/src/netbase.cpp b/src/netbase.cpp index e6d4f16ba0..0860851678 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -499,10 +499,11 @@ std::unique_ptr CreateSockTCP(const CService& address_family) return nullptr; } + auto sock = std::make_unique(hSocket); + // Ensure that waiting for I/O on this socket won't result in undefined // behavior. - if (!IsSelectableSocket(hSocket)) { - CloseSocket(hSocket); + if (!IsSelectableSocket(sock->Get())) { LogPrintf("Cannot create connection: non-selectable socket created (fd >= FD_SETSIZE ?)\n"); return nullptr; } @@ -511,19 +512,21 @@ std::unique_ptr CreateSockTCP(const CService& address_family) int set = 1; // Set the no-sigpipe option on the socket for BSD systems, other UNIXes // should use the MSG_NOSIGNAL flag for every send. - setsockopt(hSocket, SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int)); + if (sock->SetSockOpt(SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int)) == SOCKET_ERROR) { + LogPrintf("Error setting SO_NOSIGPIPE on socket: %s, continuing anyway\n", + NetworkErrorString(WSAGetLastError())); + } #endif // Set the no-delay option (disable Nagle's algorithm) on the TCP socket. - SetSocketNoDelay(hSocket); + SetSocketNoDelay(sock->Get()); // Set the non-blocking option on the socket. - if (!SetSocketNonBlocking(hSocket)) { - CloseSocket(hSocket); + if (!SetSocketNonBlocking(sock->Get())) { LogPrintf("Error setting socket to non-blocking: %s\n", NetworkErrorString(WSAGetLastError())); return nullptr; } - return std::make_unique(hSocket); + return sock; } std::function(const CService&)> CreateSock = CreateSockTCP; -- cgit v1.2.3 From a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Tue, 13 Apr 2021 14:01:44 +0200 Subject: net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay() Since the former is mockable, this makes it easier to test higher level code that sets the TCP_NODELAY flag. --- src/net.cpp | 6 +++++- src/netbase.cpp | 12 ++++-------- src/netbase.h | 2 -- 3 files changed, 9 insertions(+), 11 deletions(-) (limited to 'src') diff --git a/src/net.cpp b/src/net.cpp index 9a2c565b7d..5a0c057c3e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1190,7 +1190,11 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr&& sock, // According to the internet TCP_NODELAY is not carried into accepted sockets // on all platforms. Set it again here just to be sure. - SetSocketNoDelay(sock->Get()); + const int on{1}; + if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) { + LogPrint(BCLog::NET, "connection from %s: unable to set TCP_NODELAY, continuing anyway\n", + addr.ToString()); + } // Don't accept connections from banned peers. bool banned = m_banman && m_banman->IsBanned(addr); diff --git a/src/netbase.cpp b/src/netbase.cpp index 0860851678..9557297df1 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -519,7 +519,10 @@ std::unique_ptr CreateSockTCP(const CService& address_family) #endif // Set the no-delay option (disable Nagle's algorithm) on the TCP socket. - SetSocketNoDelay(sock->Get()); + const int on{1}; + if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) == SOCKET_ERROR) { + LogPrint(BCLog::NET, "Unable to set TCP_NODELAY on a newly created socket, continuing anyway\n"); + } // Set the non-blocking option on the socket. if (!SetSocketNonBlocking(sock->Get())) { @@ -729,13 +732,6 @@ bool SetSocketNonBlocking(const SOCKET& hSocket) return true; } -bool SetSocketNoDelay(const SOCKET& hSocket) -{ - int set = 1; - int rc = setsockopt(hSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&set, sizeof(int)); - return rc == 0; -} - void InterruptSocks5(bool interrupt) { interruptSocks5Recv = interrupt; diff --git a/src/netbase.h b/src/netbase.h index c226e6b73d..bf7522210d 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -223,8 +223,6 @@ bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_ /** Enable non-blocking mode for a socket */ bool SetSocketNonBlocking(const SOCKET& hSocket); -/** Set the TCP_NODELAY flag on a socket */ -bool SetSocketNoDelay(const SOCKET& hSocket); void InterruptSocks5(bool interrupt); /** -- cgit v1.2.3