fix: various DHT bugs (#6569)

* fix: unconditionally send DHT Port msg if supported

* chore: add "maybe" to dht add_node functions

* feat: change `bind-address-ipv*` defaults to empty string

* chore: housekeeping

* fix: initialise DHT node id with random bytes

* chore: housekeeping

docs/Editing-Configuration-Files.md

@@ -93,8 +93,8 @@ Here is a sample of the three basic types: respectively Boolean, Number and Stri
  * **preferred-transport:** String ("utp" = Prefer µTP, "tcp" = Prefer TCP; default = "utp") Choose your preferred transport protocol (has no effect if one of them is disabled).
 
 #### Peers
- * **bind-address-ipv4:** String (default = "0.0.0.0") Where to listen for peer connections. When no valid IPv4 address is provided, Transmission will bind to "0.0.0.0".
- * **bind-address-ipv6:** String (default = "::") Where to listen for peer connections. When no valid IPv6 address is provided, Transmission will try to bind to your default global IPv6 address. If that didn't work, then Transmission will bind to "::".
+ * **bind-address-ipv4:** String (default = "") Where to listen for peer connections. When no valid IPv4 address is provided, Transmission will bind to "0.0.0.0".
+ * **bind-address-ipv6:** String (default = "") Where to listen for peer connections. When no valid IPv6 address is provided, Transmission will try to bind to your default global IPv6 address. If that didn't work, then Transmission will bind to "::".
  * **peer-congestion-algorithm:** String. This is documented on https://www.pps.jussieu.fr/~jch/software/bittorrent/tcp-congestion-control.html.
  * **peer-limit-global:** Number (default = 240)
  * **peer-limit-per-torrent:** Number (default =  60)

libtransmission/peer-msgs.cc

@@ -351,13 +351,9 @@ public:
         tellPeerWhatWeHave(this);
 
         if (session->allowsDHT() && io->supports_dht())
-        {
-            // only send PORT over IPv6 iff IPv6 DHT is running (BEP-32).
-            if (auto const addr = session->bind_address(TR_AF_INET6); !addr.is_any())
         {
             protocolSendPort(this, session->udpPort());
         }
-        }
 
         io->set_callbacks(canRead, didWrite, gotError, this);
         updateDesiredRequestCount(this);
@@ -1485,7 +1481,7 @@ ReadResult process_peer_message(tr_peerMsgsImpl* msgs, uint8_t id, MessageReader
             if (auto const dht_port = tr_port::from_host(hport); !std::empty(dht_port))
             {
                 msgs->dht_port = dht_port;
-                msgs->session->addDhtNode(msgs->io->address(), msgs->dht_port);
+                msgs->session->maybe_add_dht_node(msgs->io->address(), msgs->dht_port);
             }
         }
         break;

libtransmission/session-settings.h

@@ -21,8 +21,8 @@ struct tr_variant;
 #define SESSION_SETTINGS_FIELDS(V) \
     V(TR_KEY_announce_ip, announce_ip, std::string, "", "") \
     V(TR_KEY_announce_ip_enabled, announce_ip_enabled, bool, false, "") \
-    V(TR_KEY_bind_address_ipv4, bind_address_ipv4, std::string, "0.0.0.0", "") \
-    V(TR_KEY_bind_address_ipv6, bind_address_ipv6, std::string, "::", "") \
+    V(TR_KEY_bind_address_ipv4, bind_address_ipv4, std::string, "", "") \
+    V(TR_KEY_bind_address_ipv6, bind_address_ipv6, std::string, "", "") \
     V(TR_KEY_blocklist_enabled, blocklist_enabled, bool, false, "") \
     V(TR_KEY_blocklist_url, blocklist_url, std::string, "http://www.example.com/blocklist", "") \
     V(TR_KEY_cache_size_mb, cache_size_mbytes, size_t, 4U, "") \

libtransmission/session.h

@@ -901,11 +901,11 @@ public:
 
     void addTorrent(tr_torrent* tor);
 
-    void addDhtNode(tr_address const& addr, tr_port port)
+    void maybe_add_dht_node(tr_address const& addr, tr_port port)
     {
         if (dht_)
         {
-            dht_->add_node(addr, port);
+            dht_->maybe_add_node(addr, port);
         }
     }
 

libtransmission/tr-dht.cc

@@ -139,11 +139,9 @@ public:
     {
         tr_logAddDebug(fmt::format("Starting DHT on port {port}", fmt::arg("port", peer_port.host())));
 
-        // load up the bootstrap nodes
-        if (tr_sys_path_exists(state_filename_.c_str()))
-        {
-            std::tie(id_, bootstrap_queue_) = load_state(state_filename_);
-        }
+        // init state from scratch, or load from state file if it exists
+        std::tie(id_, bootstrap_queue_) = init_state(state_filename_);
+
         get_nodes_from_bootstrap_file(tr_pathbuf{ mediator_.config_dir(), "/dht.bootstrap"sv }, bootstrap_queue_);
         get_nodes_from_name("dht.transmissionbt.com", tr_port::from_host(6881), bootstrap_queue_);
         bootstrap_timer_->start_single_shot(100ms);
@@ -176,7 +174,7 @@ public:
         tr_logAddTrace("Done uninitializing DHT");
     }
 
-    void add_node(tr_address const& addr, tr_port port) override
+    void maybe_add_node(tr_address const& addr, tr_port port) override
     {
         if (addr.is_ipv4())
         {
@@ -207,7 +205,7 @@ public:
     }
 
 private:
-    [[nodiscard]] constexpr tr_socket_t udpSocket(int af) const noexcept
+    [[nodiscard]] constexpr tr_socket_t udp_socket(int af) const noexcept
     {
         switch (af)
         {
@@ -224,7 +222,7 @@ private:
 
     [[nodiscard]] SwarmStatus swarm_status(int family, int* const setme_node_count = nullptr) const
     {
-        if (udpSocket(family) == TR_BAD_SOCKET)
+        if (udp_socket(family) == TR_BAD_SOCKET)
         {
             if (setme_node_count != nullptr)
             {
@@ -309,7 +307,7 @@ private:
 
         auto [address, port] = bootstrap_queue_.front();
         bootstrap_queue_.pop_front();
-        add_node(address, port);
+        maybe_add_node(address, port);
         ++n_bootstrapped_;
 
         bootstrap_timer_->start_single_shot(bootstrap_interval(n_bootstrapped_));
@@ -413,12 +411,12 @@ private:
 
     void save_state() const
     {
-        auto constexpr MaxNodes = int{ 300 };
-        auto constexpr PortLen = size_t{ 2 };
-        auto constexpr CompactAddrLen = size_t{ 4 };
-        auto constexpr CompactLen = size_t{ CompactAddrLen + PortLen };
-        auto constexpr Compact6AddrLen = size_t{ 16 };
-        auto constexpr Compact6Len = size_t{ Compact6AddrLen + PortLen };
+        static auto constexpr MaxNodes = int{ 300 };
+        static auto constexpr PortLen = tr_port::CompactPortBytes;
+        static auto constexpr CompactAddrLen = tr_address::CompactAddrBytes[TR_AF_INET];
+        static auto constexpr CompactLen = tr_socket_address::CompactSockAddrBytes[TR_AF_INET];
+        static auto constexpr Compact6AddrLen = tr_address::CompactAddrBytes[TR_AF_INET6];
+        static auto constexpr Compact6Len = tr_socket_address::CompactSockAddrBytes[TR_AF_INET6];
 
         auto sins4 = std::array<struct sockaddr_in, MaxNodes>{};
         auto sins6 = std::array<struct sockaddr_in6, MaxNodes>{};
@@ -464,17 +462,28 @@ private:
         tr_variant_serde::benc().to_file(benc, state_filename_);
     }
 
-    [[nodiscard]] static std::pair<Id, Nodes> load_state(std::string_view filename)
+    [[nodiscard]] static std::pair<Id, Nodes> init_state(std::string_view filename)
     {
         // Note that DHT ids need to be distributed uniformly,
         // so it should be something truly random
         auto id = tr_rand_obj<Id>();
 
-        auto nodes = Nodes{};
-
-        if (auto otop = tr_variant_serde::benc().parse_file(filename); otop)
+        if (!tr_sys_path_exists(std::data(filename)))
         {
+            return { id, {} };
+        }
+
+        auto otop = tr_variant_serde::benc().parse_file(filename);
+        if (!otop)
+        {
+            return { id, {} };
+        }
+
+        static auto constexpr CompactLen = tr_socket_address::CompactSockAddrBytes[TR_AF_INET];
+        static auto constexpr Compact6Len = tr_socket_address::CompactSockAddrBytes[TR_AF_INET6];
+
         auto& top = *otop;
+        auto nodes = Nodes{};
 
         if (auto sv = std::string_view{}; tr_variantDictFindStrView(&top, TR_KEY_id, &sv) && std::size(sv) == std::size(id))
         {
@@ -483,7 +492,7 @@ private:
 
         size_t raw_len = 0U;
         std::byte const* raw = nullptr;
-            if (tr_variantDictFindRaw(&top, TR_KEY_nodes, &raw, &raw_len) && raw_len % 6 == 0)
+        if (tr_variantDictFindRaw(&top, TR_KEY_nodes, &raw, &raw_len) && raw_len % CompactLen == 0)
         {
             auto* walk = raw;
             auto const* const end = raw + raw_len;
@@ -497,7 +506,7 @@ private:
             }
         }
 
-            if (tr_variantDictFindRaw(&top, TR_KEY_nodes6, &raw, &raw_len) && raw_len % 18 == 0)
+        if (tr_variantDictFindRaw(&top, TR_KEY_nodes6, &raw, &raw_len) && raw_len % Compact6Len == 0)
         {
             auto* walk = raw;
             auto const* const end = raw + raw_len;
@@ -510,9 +519,8 @@ private:
                 nodes.emplace_back(addr, port);
             }
         }
-        }
 
-        return std::make_pair(id, nodes);
+        return { id, std::move(nodes) };
     }
 
     ///
@@ -556,11 +564,9 @@ private:
         hints.ai_protocol = 0;
         hints.ai_flags = 0;
 
-        auto port_str = std::array<char, 16>{};
-        *fmt::format_to(std::data(port_str), "{:d}", port_in.host()) = '\0';
-
+        auto const port_str = fmt::format("{:d}", port_in.host());
         addrinfo* info = nullptr;
-        if (int const rc = getaddrinfo(name, std::data(port_str), &hints, &info); rc != 0)
+        if (int const rc = getaddrinfo(name, port_str.c_str(), &hints, &info); rc != 0)
         {
             tr_logAddWarn(fmt::format(
                 _("Couldn't look up '{address}:{port}': {error} ({error_code})"),

libtransmission/tr-dht.h

@@ -114,6 +114,6 @@ public:
         tr_socket_t udp6_socket);
     virtual ~tr_dht() = default;
 
-    virtual void add_node(tr_address const& address, tr_port port) = 0;
+    virtual void maybe_add_node(tr_address const& address, tr_port port) = 0;
     virtual void handle_message(unsigned char const* msg, size_t msglen, struct sockaddr* from, socklen_t fromlen) = 0;
 };

tests/libtransmission/dht-test.cc

@@ -619,7 +619,7 @@ TEST_F(DhtTest, pingsAddedNodes)
     EXPECT_TRUE(addr.has_value());
     assert(addr.has_value());
     auto constexpr Port = tr_port::from_host(128);
-    dht->add_node(*addr, Port);
+    dht->maybe_add_node(*addr, Port);
 
     ASSERT_EQ(1U, std::size(mediator.mock_dht_.pinged_));
     EXPECT_EQ(addr, mediator.mock_dht_.pinged_.front().addrport.address());