From 0ea8df4d64959a7c7ec4e55b4895d6b16dad3000 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Wed, 29 Dec 2021 23:01:26 +0100 Subject: [PATCH] Socket-related cleanups Improve error handling on Windows and reduce the size of the `Address` class --- src/client/game.cpp | 1 - src/filesys.cpp | 2 + src/network/address.cpp | 119 +++++++++++++---------------------- src/network/address.h | 37 ++++++----- src/network/socket.cpp | 74 +++++++++++----------- src/network/socket.h | 14 ----- src/server.cpp | 5 +- src/unittest/test_socket.cpp | 18 +++--- 8 files changed, 114 insertions(+), 156 deletions(-) diff --git a/src/client/game.cpp b/src/client/game.cpp index 853a52ecf..f62d26e8f 100644 --- a/src/client/game.cpp +++ b/src/client/game.cpp @@ -1444,7 +1444,6 @@ bool Game::connectToServer(const GameStartData &start_data, connect_address.Resolve(start_data.address.c_str()); if (connect_address.isZero()) { // i.e. INADDR_ANY, IN6ADDR_ANY - //connect_address.Resolve("localhost"); if (connect_address.isIPv6()) { IPv6AddressBytes addr_bytes; addr_bytes.bytes[15] = 1; diff --git a/src/filesys.cpp b/src/filesys.cpp index 60090c801..ea00def6a 100644 --- a/src/filesys.cpp +++ b/src/filesys.cpp @@ -41,7 +41,9 @@ namespace fs * Windows * ***********/ +#ifndef _WIN32_WINNT #define _WIN32_WINNT 0x0501 +#endif #include #include #include diff --git a/src/network/address.cpp b/src/network/address.cpp index 05678aa62..90e561802 100644 --- a/src/network/address.cpp +++ b/src/network/address.cpp @@ -87,38 +87,31 @@ Address::Address(const IPv6AddressBytes *ipv6_bytes, u16 port) setPort(port); } -// Equality (address family, address and port must be equal) -bool Address::operator==(const Address &address) +// Equality (address family, IP and port must be equal) +bool Address::operator==(const Address &other) { - if (address.m_addr_family != m_addr_family || address.m_port != m_port) + if (other.m_addr_family != m_addr_family || other.m_port != m_port) return false; if (m_addr_family == AF_INET) { - return m_address.ipv4.sin_addr.s_addr == - address.m_address.ipv4.sin_addr.s_addr; + return m_address.ipv4.s_addr == other.m_address.ipv4.s_addr; } if (m_addr_family == AF_INET6) { - return memcmp(m_address.ipv6.sin6_addr.s6_addr, - address.m_address.ipv6.sin6_addr.s6_addr, 16) == 0; + return memcmp(m_address.ipv6.s6_addr, + other.m_address.ipv6.s6_addr, 16) == 0; } return false; } -bool Address::operator!=(const Address &address) -{ - return !(*this == address); -} - void Address::Resolve(const char *name) { if (!name || name[0] == 0) { - if (m_addr_family == AF_INET) { - setAddress((u32)0); - } else if (m_addr_family == AF_INET6) { - setAddress((IPv6AddressBytes *)0); - } + if (m_addr_family == AF_INET) + setAddress(static_cast(0)); + else if (m_addr_family == AF_INET6) + setAddress(static_cast(nullptr)); return; } @@ -126,9 +119,6 @@ void Address::Resolve(const char *name) memset(&hints, 0, sizeof(hints)); // Setup hints - hints.ai_socktype = 0; - hints.ai_protocol = 0; - hints.ai_flags = 0; if (g_settings->getBool("enable_ipv6")) { // AF_UNSPEC allows both IPv6 and IPv4 addresses to be returned hints.ai_family = AF_UNSPEC; @@ -145,14 +135,13 @@ void Address::Resolve(const char *name) if (resolved->ai_family == AF_INET) { struct sockaddr_in *t = (struct sockaddr_in *)resolved->ai_addr; m_addr_family = AF_INET; - m_address.ipv4 = *t; + m_address.ipv4 = t->sin_addr; } else if (resolved->ai_family == AF_INET6) { struct sockaddr_in6 *t = (struct sockaddr_in6 *)resolved->ai_addr; m_addr_family = AF_INET6; - m_address.ipv6 = *t; + m_address.ipv6 = t->sin6_addr; } else { - freeaddrinfo(resolved); - throw ResolveError(""); + m_addr_family = 0; } freeaddrinfo(resolved); } @@ -163,47 +152,37 @@ std::string Address::serializeString() const // windows XP doesnt have inet_ntop, maybe use better func #ifdef _WIN32 if (m_addr_family == AF_INET) { - u8 a, b, c, d; - u32 addr; - addr = ntohl(m_address.ipv4.sin_addr.s_addr); - a = (addr & 0xFF000000) >> 24; - b = (addr & 0x00FF0000) >> 16; - c = (addr & 0x0000FF00) >> 8; - d = (addr & 0x000000FF); - return itos(a) + "." + itos(b) + "." + itos(c) + "." + itos(d); + return inet_ntoa(m_address.ipv4); } else if (m_addr_family == AF_INET6) { std::ostringstream os; + os << std::hex; for (int i = 0; i < 16; i += 2) { - u16 section = (m_address.ipv6.sin6_addr.s6_addr[i] << 8) | - (m_address.ipv6.sin6_addr.s6_addr[i + 1]); - os << std::hex << section; + u16 section = (m_address.ipv6.s6_addr[i] << 8) | + (m_address.ipv6.s6_addr[i + 1]); + os << section; if (i < 14) os << ":"; } return os.str(); - } else - return std::string(""); + } else { + return ""; + } #else char str[INET6_ADDRSTRLEN]; - if (inet_ntop(m_addr_family, - (m_addr_family == AF_INET) - ? (void *)&(m_address.ipv4.sin_addr) - : (void *)&(m_address.ipv6.sin6_addr), - str, INET6_ADDRSTRLEN) == NULL) { - return std::string(""); - } - return std::string(str); + if (inet_ntop(m_addr_family, (void*) &m_address, str, sizeof(str)) == nullptr) + return ""; + return str; #endif } -struct sockaddr_in Address::getAddress() const +struct in_addr Address::getAddress() const { - return m_address.ipv4; // NOTE: NO PORT INCLUDED, use getPort() + return m_address.ipv4; } -struct sockaddr_in6 Address::getAddress6() const +struct in6_addr Address::getAddress6() const { - return m_address.ipv6; // NOTE: NO PORT INCLUDED, use getPort() + return m_address.ipv6; } u16 Address::getPort() const @@ -211,52 +190,39 @@ u16 Address::getPort() const return m_port; } -int Address::getFamily() const -{ - return m_addr_family; -} - -bool Address::isIPv6() const -{ - return m_addr_family == AF_INET6; -} - bool Address::isZero() const { if (m_addr_family == AF_INET) { - return m_address.ipv4.sin_addr.s_addr == 0; + return m_address.ipv4.s_addr == 0; } if (m_addr_family == AF_INET6) { static const char zero[16] = {0}; - return memcmp(m_address.ipv6.sin6_addr.s6_addr, zero, 16) == 0; + return memcmp(m_address.ipv6.s6_addr, zero, 16) == 0; } + return false; } void Address::setAddress(u32 address) { m_addr_family = AF_INET; - m_address.ipv4.sin_family = AF_INET; - m_address.ipv4.sin_addr.s_addr = htonl(address); + m_address.ipv4.s_addr = htonl(address); } void Address::setAddress(u8 a, u8 b, u8 c, u8 d) { - m_addr_family = AF_INET; - m_address.ipv4.sin_family = AF_INET; - u32 addr = htonl((a << 24) | (b << 16) | (c << 8) | d); - m_address.ipv4.sin_addr.s_addr = addr; + u32 addr = (a << 24) | (b << 16) | (c << 8) | d; + setAddress(addr); } void Address::setAddress(const IPv6AddressBytes *ipv6_bytes) { m_addr_family = AF_INET6; - m_address.ipv6.sin6_family = AF_INET6; if (ipv6_bytes) - memcpy(m_address.ipv6.sin6_addr.s6_addr, ipv6_bytes->bytes, 16); + memcpy(m_address.ipv6.s6_addr, ipv6_bytes->bytes, 16); else - memset(m_address.ipv6.sin6_addr.s6_addr, 0, 16); + memset(m_address.ipv6.s6_addr, 0, 16); } void Address::setPort(u16 port) @@ -268,23 +234,26 @@ void Address::print(std::ostream *s) const { if (m_addr_family == AF_INET6) *s << "[" << serializeString() << "]:" << m_port; - else + else if (m_addr_family == AF_INET) *s << serializeString() << ":" << m_port; + else + *s << "(undefined)"; } bool Address::isLocalhost() const { if (isIPv6()) { - static const unsigned char localhost_bytes[] = { + static const u8 localhost_bytes[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}; - static const unsigned char mapped_ipv4_localhost[] = { + static const u8 mapped_ipv4_localhost[] = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0x7f, 0, 0, 0}; - auto addr = m_address.ipv6.sin6_addr.s6_addr; + auto addr = m_address.ipv6.s6_addr; return memcmp(addr, localhost_bytes, 16) == 0 || memcmp(addr, mapped_ipv4_localhost, 13) == 0; } - return (m_address.ipv4.sin_addr.s_addr & 0xFF) == 0x7f; + auto addr = ntohl(m_address.ipv4.s_addr); + return (addr >> 24) == 0x7f; } diff --git a/src/network/address.h b/src/network/address.h index 4329c84a8..c2f5f2eef 100644 --- a/src/network/address.h +++ b/src/network/address.h @@ -36,9 +36,8 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "irrlichttypes.h" #include "networkexceptions.h" -class IPv6AddressBytes +struct IPv6AddressBytes { -public: u8 bytes[16]; IPv6AddressBytes() { memset(bytes, 0, 16); } }; @@ -50,30 +49,34 @@ public: Address(u32 address, u16 port); Address(u8 a, u8 b, u8 c, u8 d, u16 port); Address(const IPv6AddressBytes *ipv6_bytes, u16 port); + bool operator==(const Address &address); - bool operator!=(const Address &address); - // Resolve() may throw ResolveError (address is unchanged in this case) - void Resolve(const char *name); - struct sockaddr_in getAddress() const; - unsigned short getPort() const; - void setAddress(u32 address); - void setAddress(u8 a, u8 b, u8 c, u8 d); - void setAddress(const IPv6AddressBytes *ipv6_bytes); - struct sockaddr_in6 getAddress6() const; - int getFamily() const; - bool isIPv6() const; + bool operator!=(const Address &address) { return !(*this == address); } + + struct in_addr getAddress() const; + struct in6_addr getAddress6() const; + u16 getPort() const; + int getFamily() const { return m_addr_family; } + bool isIPv6() const { return m_addr_family == AF_INET6; } bool isZero() const; - void setPort(unsigned short port); void print(std::ostream *s) const; std::string serializeString() const; bool isLocalhost() const; + // Resolve() may throw ResolveError (address is unchanged in this case) + void Resolve(const char *name); + + void setAddress(u32 address); + void setAddress(u8 a, u8 b, u8 c, u8 d); + void setAddress(const IPv6AddressBytes *ipv6_bytes); + void setPort(u16 port); + private: - unsigned int m_addr_family = 0; + unsigned short m_addr_family = 0; union { - struct sockaddr_in ipv4; - struct sockaddr_in6 ipv6; + struct in_addr ipv4; + struct in6_addr ipv6; } m_address; u16 m_port = 0; // Port is separate from sockaddr structures }; diff --git a/src/network/socket.cpp b/src/network/socket.cpp index 94a9f4180..0bb7ea234 100644 --- a/src/network/socket.cpp +++ b/src/network/socket.cpp @@ -23,14 +23,11 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #include -#include -#include #include #include "util/string.h" #include "util/numeric.h" #include "constants.h" #include "debug.h" -#include "settings.h" #include "log.h" #ifdef _WIN32 @@ -42,9 +39,10 @@ with this program; if not, write to the Free Software Foundation, Inc., #include #include #define LAST_SOCKET_ERR() WSAGetLastError() -typedef SOCKET socket_t; +#define SOCKET_ERR_STR(e) itos(e) typedef int socklen_t; #else +#include #include #include #include @@ -53,7 +51,7 @@ typedef int socklen_t; #include #include #define LAST_SOCKET_ERR() (errno) -typedef int socket_t; +#define SOCKET_ERR_STR(e) strerror(e) #endif // Set to true to enable verbose debug output @@ -113,7 +111,7 @@ bool UDPSocket::init(bool ipv6, bool noExceptions) } throw SocketException(std::string("Failed to create socket: error ") + - itos(LAST_SOCKET_ERR())); + SOCKET_ERR_STR(LAST_SOCKET_ERR())); } setTimeoutMs(0); @@ -153,40 +151,40 @@ void UDPSocket::Bind(Address addr) } if (addr.getFamily() != m_addr_family) { - static const char *errmsg = + const char *errmsg = "Socket and bind address families do not match"; errorstream << "Bind failed: " << errmsg << std::endl; throw SocketException(errmsg); } + int ret = 0; + if (m_addr_family == AF_INET6) { struct sockaddr_in6 address; memset(&address, 0, sizeof(address)); - address = addr.getAddress6(); address.sin6_family = AF_INET6; + address.sin6_addr = addr.getAddress6(); address.sin6_port = htons(addr.getPort()); - if (bind(m_handle, (const struct sockaddr *)&address, - sizeof(struct sockaddr_in6)) < 0) { - dstream << (int)m_handle << ": Bind failed: " << strerror(errno) - << std::endl; - throw SocketException("Failed to bind socket"); - } + ret = bind(m_handle, (const struct sockaddr *) &address, + sizeof(struct sockaddr_in6)); } else { struct sockaddr_in address; memset(&address, 0, sizeof(address)); - address = addr.getAddress(); address.sin_family = AF_INET; + address.sin_addr = addr.getAddress(); address.sin_port = htons(addr.getPort()); - if (bind(m_handle, (const struct sockaddr *)&address, - sizeof(struct sockaddr_in)) < 0) { - dstream << (int)m_handle << ": Bind failed: " << strerror(errno) - << std::endl; - throw SocketException("Failed to bind socket"); - } + ret = bind(m_handle, (const struct sockaddr *) &address, + sizeof(struct sockaddr_in)); + } + + if (ret < 0) { + dstream << (int)m_handle << ": Bind failed: " + << SOCKET_ERR_STR(LAST_SOCKET_ERR()) << std::endl; + throw SocketException("Failed to bind socket"); } } @@ -233,13 +231,19 @@ void UDPSocket::Send(const Address &destination, const void *data, int size) int sent; if (m_addr_family == AF_INET6) { - struct sockaddr_in6 address = destination.getAddress6(); + struct sockaddr_in6 address = {0}; + address.sin6_family = AF_INET6; + address.sin6_addr = destination.getAddress6(); address.sin6_port = htons(destination.getPort()); + sent = sendto(m_handle, (const char *)data, size, 0, (struct sockaddr *)&address, sizeof(struct sockaddr_in6)); } else { - struct sockaddr_in address = destination.getAddress(); + struct sockaddr_in address = {0}; + address.sin_family = AF_INET; + address.sin_addr = destination.getAddress(); address.sin_port = htons(destination.getPort()); + sent = sendto(m_handle, (const char *)data, size, 0, (struct sockaddr *)&address, sizeof(struct sockaddr_in)); } @@ -267,9 +271,9 @@ int UDPSocket::Receive(Address &sender, void *data, int size) return -1; u16 address_port = ntohs(address.sin6_port); - IPv6AddressBytes bytes; - memcpy(bytes.bytes, address.sin6_addr.s6_addr, 16); - sender = Address(&bytes, address_port); + const auto *bytes = reinterpret_cast + (address.sin6_addr.s6_addr); + sender = Address(bytes, address_port); } else { struct sockaddr_in address; memset(&address, 0, sizeof(address)); @@ -341,7 +345,12 @@ bool UDPSocket::WaitData(int timeout_ms) if (result == 0) return false; - if (result < 0 && (errno == EINTR || errno == EBADF)) { + int e = LAST_SOCKET_ERR(); +#ifdef _WIN32 + if (result < 0 && (e == WSAEINTR || e == WSAEBADF)) { +#else + if (result < 0 && (e == EINTR || e == EBADF)) { +#endif // N.B. select() fails when sockets are destroyed on Connection's dtor // with EBADF. Instead of doing tricky synchronization, allow this // thread to exit but don't throw an exception. @@ -349,18 +358,9 @@ bool UDPSocket::WaitData(int timeout_ms) } if (result < 0) { - dstream << m_handle << ": Select failed: " << strerror(errno) + dstream << (int)m_handle << ": Select failed: " << SOCKET_ERR_STR(e) << std::endl; -#ifdef _WIN32 - int e = WSAGetLastError(); - dstream << (int)m_handle << ": WSAGetLastError()=" << e << std::endl; - if (e == 10004 /* WSAEINTR */ || e == 10009 /* WSAEBADF */) { - infostream << "Ignoring WSAEINTR/WSAEBADF." << std::endl; - return false; - } -#endif - throw SocketException("Select failed"); } else if (!FD_ISSET(m_handle, &readset)) { // No data diff --git a/src/network/socket.h b/src/network/socket.h index e0e76f4c2..d34186b44 100644 --- a/src/network/socket.h +++ b/src/network/socket.h @@ -19,18 +19,6 @@ with this program; if not, write to the Free Software Foundation, Inc., #pragma once -#ifdef _WIN32 -#ifndef _WIN32_WINNT -#define _WIN32_WINNT 0x0501 -#endif -#include -#include -#include -#else -#include -#include -#endif - #include #include #include "address.h" @@ -53,8 +41,6 @@ public: bool init(bool ipv6, bool noExceptions = false); - // void Close(); - // bool IsOpen(); void Send(const Address &destination, const void *data, int size); // Returns -1 if there is no data int Receive(Address &sender, void *data, int size); diff --git a/src/server.cpp b/src/server.cpp index c175cbcd2..a910185b9 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -507,8 +507,9 @@ void Server::start() << " \\/ \\/ \\/ \\/ \\/ " << std::endl; actionstream << "World at [" << m_path_world << "]" << std::endl; actionstream << "Server for gameid=\"" << m_gamespec.id - << "\" listening on " << m_bind_addr.serializeString() << ":" - << m_bind_addr.getPort() << "." << std::endl; + << "\" listening on "; + m_bind_addr.print(&actionstream); + actionstream << "." << std::endl; } void Server::stop() diff --git a/src/unittest/test_socket.cpp b/src/unittest/test_socket.cpp index 6d5cf334d..620021b59 100644 --- a/src/unittest/test_socket.cpp +++ b/src/unittest/test_socket.cpp @@ -97,11 +97,11 @@ void TestSocket::testIPv4Socket() UASSERT(strncmp(sendbuffer, rcvbuffer, sizeof(sendbuffer)) == 0); if (address != Address(0, 0, 0, 0, port)) { - UASSERT(sender.getAddress().sin_addr.s_addr == - address.getAddress().sin_addr.s_addr); + UASSERT(sender.getAddress().s_addr == + address.getAddress().s_addr); } else { - UASSERT(sender.getAddress().sin_addr.s_addr == - Address(127, 0, 0, 1, 0).getAddress().sin_addr.s_addr); + UASSERT(sender.getAddress().s_addr == + Address(127, 0, 0, 1, 0).getAddress().s_addr); } } @@ -128,7 +128,7 @@ void TestSocket::testIPv6Socket() socket6.Bind(address6); - try { + { socket6.Send(Address(&bytes, port), sendbuffer, sizeof(sendbuffer)); sleep_ms(50); @@ -142,10 +142,8 @@ void TestSocket::testIPv6Socket() } //FIXME: This fails on some systems UASSERT(strncmp(sendbuffer, rcvbuffer, sizeof(sendbuffer)) == 0); - UASSERT(memcmp(sender.getAddress6().sin6_addr.s6_addr, - Address(&bytes, 0).getAddress6().sin6_addr.s6_addr, 16) == 0); - } catch (SendFailedException &e) { - errorstream << "IPv6 support enabled but not available!" - << std::endl; + + UASSERT(memcmp(sender.getAddress6().s6_addr, + Address(&bytes, 0).getAddress6().s6_addr, 16) == 0); } }