refactor: add tr_address::fromSockaddr() (#3964)

* refactor: remove TR_DISCARD_ALIGN

* refactor: add tr_address::fromSockaddr()

libtransmission/announcer-udp.cc

@@ -48,11 +48,11 @@ static void tau_sockaddr_setport(struct sockaddr* sa, tr_port port)
 {
     if (sa->sa_family == AF_INET)
     {
-        TR_DISCARD_ALIGN(sa, struct sockaddr_in*)->sin_port = port.network();
+        reinterpret_cast<sockaddr_in*>(sa)->sin_port = port.network();
     }
     else if (sa->sa_family == AF_INET6)
     {
-        TR_DISCARD_ALIGN(sa, struct sockaddr_in6*)->sin6_port = port.network();
+        reinterpret_cast<sockaddr_in6*>(sa)->sin6_port = port.network();
     }
 }
 

libtransmission/net.cc

@@ -186,27 +186,32 @@ void tr_netSetCongestionControl([[maybe_unused]] tr_socket_t s, [[maybe_unused]]
 #endif
 }
 
-bool tr_address_from_sockaddr_storage(tr_address* setme_addr, tr_port* setme_port, struct sockaddr_storage const* from)
+std::optional<std::pair<tr_address, tr_port>> tr_address::fromSockaddr(struct sockaddr const* from)
 {
-    if (from->ss_family == AF_INET)
+    if (from == nullptr)
     {
-        auto const* const sin = (struct sockaddr_in const*)from;
-        setme_addr->type = TR_AF_INET;
-        setme_addr->addr.addr4.s_addr = sin->sin_addr.s_addr;
-        *setme_port = tr_port::fromNetwork(sin->sin_port);
-        return true;
+        return {};
     }
 
-    if (from->ss_family == AF_INET6)
+    if (from->sa_family == AF_INET)
     {
-        auto const* const sin6 = (struct sockaddr_in6 const*)from;
-        setme_addr->type = TR_AF_INET6;
-        setme_addr->addr.addr6 = sin6->sin6_addr;
-        *setme_port = tr_port::fromNetwork(sin6->sin6_port);
-        return true;
+        auto const* const sin = reinterpret_cast<struct sockaddr_in const*>(from);
+        auto addr = tr_address{};
+        addr.type = TR_AF_INET;
+        addr.addr.addr4 = sin->sin_addr;
+        return std::make_pair(addr, tr_port::fromNetwork(sin->sin_port));
+    }
+
+    if (from->sa_family == AF_INET6)
+    {
+        auto const* const sin6 = reinterpret_cast<struct sockaddr_in6 const*>(from);
+        auto addr = tr_address{};
+        addr.type = TR_AF_INET6;
+        addr.addr.addr6 = sin6->sin6_addr;
+        return std::make_pair(addr, tr_port::fromNetwork(sin6->sin6_port));
     }
 
-    return false;
+    return {};
 }
 
 std::pair<sockaddr_storage, socklen_t> tr_address::toSockaddr(tr_port port) const noexcept
@@ -216,7 +221,7 @@ std::pair<sockaddr_storage, socklen_t> tr_address::toSockaddr(tr_port port) cons
     if (isIPv4())
     {
         auto* const ss4 = reinterpret_cast<sockaddr_in*>(&ss);
-        ss4->sin_addr.s_addr = addr.addr4.s_addr;
+        ss4->sin_addr = addr.addr4;
         ss4->sin_family = AF_INET;
         ss4->sin_port = port.network();
         return { ss, sizeof(sockaddr_in) };
@@ -533,32 +538,30 @@ bool tr_net_hasIPv6(tr_port port)
     return result;
 }
 
-tr_socket_t tr_netAccept(tr_session* session, tr_socket_t listening_sockfd, tr_address* addr, tr_port* port)
+std::optional<std::tuple<tr_address, tr_port, tr_socket_t>> tr_netAccept(tr_session* session, tr_socket_t listening_sockfd)
 {
     TR_ASSERT(session != nullptr);
-    TR_ASSERT(addr != nullptr);
-    TR_ASSERT(port != nullptr);
 
     // accept the incoming connection
     auto sock = sockaddr_storage{};
     socklen_t len = sizeof(struct sockaddr_storage);
     auto const sockfd = accept(listening_sockfd, (struct sockaddr*)&sock, &len);
     if (sockfd == TR_BAD_SOCKET)
     {
-        return TR_BAD_SOCKET;
+        return {};
     }
 
     // get the address and port,
     // make the socket unblocking,
     // and confirm we don't have too many peers
-    if (!tr_address_from_sockaddr_storage(addr, port, &sock) || evutil_make_socket_nonblocking(sockfd) == -1 ||
-        !session->incPeerCount())
+    auto const addrport = tr_address::fromSockaddr(reinterpret_cast<struct sockaddr*>(&sock));
+    if (!addrport || evutil_make_socket_nonblocking(sockfd) == -1 || !session->incPeerCount())
     {
         tr_netCloseSocket(sockfd);
-        return TR_BAD_SOCKET;
+        return {};
     }
 
-    return sockfd;
+    return std::make_tuple(addrport->first, addrport->second, sockfd);
 }
 
 void tr_netCloseSocket(tr_socket_t sockfd)

libtransmission/net.h

@@ -153,6 +153,7 @@ class tr_port
 struct tr_address
 {
     [[nodiscard]] static std::optional<tr_address> fromString(std::string_view address_sv);
+    [[nodiscard]] static std::optional<std::pair<tr_address, tr_port>> fromSockaddr(struct sockaddr const*);
     [[nodiscard]] static std::pair<tr_address, uint8_t const*> fromCompact4(uint8_t const* compact) noexcept;
     [[nodiscard]] static std::pair<tr_address, uint8_t const*> fromCompact6(uint8_t const* compact) noexcept;
 
@@ -206,8 +207,6 @@ struct tr_address
 extern tr_address const tr_inaddr_any;
 extern tr_address const tr_in6addr_any;
 
-bool tr_address_from_sockaddr_storage(tr_address* setme, tr_port* port, struct sockaddr_storage const* from);
-
 bool tr_address_is_valid_for_peers(tr_address const* addr, tr_port port);
 
 constexpr bool tr_address_is_valid(tr_address const* a)
@@ -223,7 +222,9 @@ struct tr_session;
 
 tr_socket_t tr_netBindTCP(tr_address const* addr, tr_port port, bool suppress_msgs);
 
-tr_socket_t tr_netAccept(tr_session* session, tr_socket_t listening_sockfd, tr_address* setme_addr, tr_port* setme_port);
+[[nodiscard]] std::optional<std::tuple<tr_address, tr_port, tr_socket_t>> tr_netAccept(
+    tr_session* session,
+    tr_socket_t listening_sockfd);
 
 void tr_netSetCongestionControl(tr_socket_t s, char const* algorithm);
 

libtransmission/session.cc

@@ -287,15 +287,11 @@ static void acceptIncomingPeer(evutil_socket_t fd, short /*what*/, void* vsessio
 {
     auto* session = static_cast<tr_session*>(vsession);
 
-    auto client_addr = tr_address{};
-    auto client_port = tr_port{};
-    auto const client_socket = tr_netAccept(session, fd, &client_addr, &client_port);
-
-    if (client_socket != TR_BAD_SOCKET)
+    if (auto const incoming_info = tr_netAccept(session, fd); incoming_info)
     {
-        tr_logAddTrace(fmt::format("new incoming connection {} ({})", client_socket, client_addr.readable(client_port)));
-
-        session->addIncoming(client_addr, client_port, tr_peer_socket_tcp_create(client_socket));
+        auto const [addr, port, sock] = *incoming_info;
+        tr_logAddTrace(fmt::format("new incoming connection {} ({})", sock, addr.readable(port)));
+        session->addIncoming(addr, port, tr_peer_socket_tcp_create(sock));
     }
 }
 

libtransmission/tr-macros.h

@@ -60,9 +60,6 @@
 
 #define TR_PATH_DELIMITER '/'
 
-/* Only use this macro to suppress false-positive alignment warnings */
-#define TR_DISCARD_ALIGN(ptr, type) ((type)(void*)(ptr))
-
 #define TR_INET6_ADDRSTRLEN 46
 
 #define TR_ADDRSTRLEN 64

libtransmission/tr-utp.cc

@@ -76,30 +76,30 @@ void tr_utpClose(tr_session* /*session*/)
 /* Greg says 50ms works for them. */
 static auto constexpr UtpInterval = 50ms;
 
-static void utp_on_accept(tr_session* const session, UTPSocket* const s)
+static void utp_on_accept(tr_session* const session, UTPSocket* const utp_sock)
 {
     auto from_storage = sockaddr_storage{};
     auto* const from = (struct sockaddr*)&from_storage;
     socklen_t fromlen = sizeof(from_storage);
-    auto addr = tr_address{};
-    auto port = tr_port{};
 
     if (!session->allowsUTP())
     {
-        utp_close(s);
+        utp_close(utp_sock);
         return;
     }
 
-    utp_getpeername(s, from, &fromlen);
+    utp_getpeername(utp_sock, from, &fromlen);
 
-    if (!tr_address_from_sockaddr_storage(&addr, &port, &from_storage))
+    if (auto addrport = tr_address::fromSockaddr(reinterpret_cast<struct sockaddr*>(&from_storage)); addrport)
+    {
+        auto const [addr, port] = *addrport;
+        session->addIncoming(addr, port, tr_peer_socket_utp_create(utp_sock));
+    }
+    else
     {
         tr_logAddWarn(_("Unknown socket family"));
-        utp_close(s);
-        return;
+        utp_close(utp_sock);
     }
-
-    session->addIncoming(addr, port, tr_peer_socket_utp_create(s));
 }
 
 static void utp_send_to(

tests/libtransmission/CMakeLists.txt

@@ -21,6 +21,7 @@ add_executable(libtransmission-test
     magnet-metainfo-test.cc
     makemeta-test.cc
     move-test.cc
+    net-test.cc
     open-files-test.cc
     peer-mgr-active-requests-test.cc
     peer-mgr-wishlist-test.cc

tests/libtransmission/net-test.cc

@@ -0,0 +1,32 @@
+// This file Copyright (C) 2013-2022 Mnemosyne LLC.
+// It may be used under GPLv2 (SPDX: GPL-2.0-only), GPLv3 (SPDX: GPL-3.0-only),
+// or any future license endorsed by Mnemosyne LLC.
+// License text can be found in the licenses/ folder.
+
+#include "transmission.h"
+
+#include "net.h"
+
+#include "test-fixtures.h"
+
+using NetTest = ::testing::Test;
+using namespace std::literals;
+
+TEST_F(NetTest, conversionsIPv4)
+{
+    auto constexpr port = tr_port::fromHost(80);
+    auto constexpr addrstr = "127.0.0.1"sv;
+
+    auto addr = tr_address::fromString(addrstr);
+    EXPECT_TRUE(addr);
+    EXPECT_EQ(addrstr, addr->readable());
+
+    auto [ss, sslen] = addr->toSockaddr(port);
+    EXPECT_EQ(AF_INET, ss.ss_family);
+    EXPECT_EQ(port.network(), reinterpret_cast<sockaddr_in const*>(&ss)->sin_port);
+
+    auto addrport = tr_address::fromSockaddr(reinterpret_cast<sockaddr const*>(&ss));
+    EXPECT_TRUE(addrport);
+    EXPECT_EQ(addr, addrport->first);
+    EXPECT_EQ(port, addrport->second);
+}