fix: bound the TCP connect with the client timeout

core/src/tcp.rs

@@ -2,8 +2,8 @@
 //! layered on top (SPEC.md sec 4), and the framed transport over it.
 
 use std::io::{Read, Write};
-use std::net::TcpStream;
-use std::time::Duration;
+use std::net::{SocketAddr, TcpStream, ToSocketAddrs};
+use std::time::{Duration, Instant};
 
 use snow::{Builder, TransportState};
 
@@ -120,6 +120,54 @@ fn dropped_mid_handshake(e: &std::io::Error) -> bool {
     )
 }
 
+/// `TcpStream::connect` has no timeout of its own: on a path that silently
+/// drops packets instead of refusing them (a common firewall/NAT
+/// behaviour), it can block far past the `timeout` this client otherwise
+/// promises, for every caller waiting on the connection -- register,
+/// fetch, send alike. DNS is resolved eagerly and each candidate address
+/// is dialled with an explicit bound instead.
+fn connect_timeout(host: &str, port: u16, timeout: u64) -> std::io::Result<TcpStream> {
+    dial_bounded(
+        (host, port).to_socket_addrs()?,
+        Duration::from_secs(timeout),
+        TcpStream::connect_timeout,
+    )
+}
+
+/// Dials each candidate in turn under one shared deadline, rather than granting
+/// every candidate the full budget: a dual-stack host resolves to several
+/// addresses, and dialling each with the whole timeout multiplies the wait the
+/// caller was promised. `dial` is a parameter so the bound is testable without
+/// a route that blackholes packets.
+fn dial_bounded<S>(
+    addrs: impl Iterator<Item = SocketAddr>,
+    budget: Duration,
+    mut dial: impl FnMut(&SocketAddr, Duration) -> std::io::Result<S>,
+) -> std::io::Result<S> {
+    let deadline = Instant::now() + budget;
+    let mut last_err = None;
+    for addr in addrs {
+        let remaining = deadline.saturating_duration_since(Instant::now());
+        if remaining.is_zero() {
+            last_err = Some(std::io::Error::new(
+                std::io::ErrorKind::TimedOut,
+                "connection timed out",
+            ));
+            break;
+        }
+        match dial(&addr, remaining) {
+            Ok(stream) => return Ok(stream),
+            Err(e) => last_err = Some(e),
+        }
+    }
+    Err(last_err.unwrap_or_else(|| {
+        std::io::Error::new(
+            std::io::ErrorKind::InvalidInput,
+            "no addresses to connect to",
+        )
+    }))
+}
+
 /// One Noise_NX handshake attempt: dial, exchange the two handshake
 /// messages, extract the server's static key and the bind values. The NX
 /// pattern has the server transmit its static key during the handshake, so
@@ -130,7 +178,7 @@ fn handshake(
     port: u16,
     timeout: u64,
 ) -> Result<(TcpStream, TransportState, [u8; KEY_LEN], TransportBindValues), Error> {
-    let mut stream = TcpStream::connect((host, port)).map_err(|source| Error::Unreachable {
+    let mut stream = connect_timeout(host, port, timeout).map_err(|source| Error::Unreachable {
         host: host.to_string(),
         port,
         source,
@@ -253,6 +301,97 @@ mod tests {
         dropper.join().unwrap();
     }
 
+    /// The budget is spent across candidates, not renewed for each: a
+    /// dual-stack host resolves to several addresses, and a dial that consumes
+    /// most of its bound must leave the next one less, never the whole budget
+    /// again. The fake dialler stands in for a blackholing path -- packets
+    /// dropped rather than refused, which is what `TcpStream::connect` blocks
+    /// on indefinitely -- since no test can rely on a real route to blackhole.
+    #[test]
+    fn the_dial_budget_is_spent_across_candidates_not_renewed() {
+        let budget = Duration::from_millis(400);
+        let addrs = vec![blackhole(1), blackhole(2), blackhole(3)];
+        let mut bounds = Vec::new();
+        let started = Instant::now();
+        dial_bounded::<TcpStream>(addrs.into_iter(), budget, |_addr, remaining| {
+            bounds.push(remaining);
+            // A blackholed dial returns nothing until its own bound runs low.
+            std::thread::sleep(remaining.mul_f32(0.75));
+            Err(std::io::Error::from(std::io::ErrorKind::TimedOut))
+        })
+        .expect_err("a blackholed dial cannot connect");
+        let elapsed = started.elapsed();
+
+        assert_eq!(bounds.len(), 3, "a candidate went undialled: {bounds:?}");
+        assert!(
+            bounds.windows(2).all(|w| w[1] < w[0]),
+            "a candidate was dialled with a renewed budget: {bounds:?}"
+        );
+        assert!(
+            bounds[0] <= budget,
+            "first bound {:?} exceeds the budget",
+            bounds[0]
+        );
+        // Coarse backstop; the assertions above are what pin the behaviour.
+        assert!(
+            elapsed < budget * 2,
+            "dial took {elapsed:?}, past the {budget:?} budget"
+        );
+    }
+
+    /// A single candidate that consumes the whole budget ends the dial: the
+    /// remaining candidates are not dialled with nothing left, which would hand
+    /// `TcpStream::connect_timeout` a zero duration it rejects outright.
+    #[test]
+    fn an_exhausted_budget_ends_the_dial_as_a_timeout() {
+        let budget = Duration::from_millis(200);
+        let addrs = vec![blackhole(1), blackhole(2)];
+        let mut dialled = 0;
+        let error = dial_bounded::<TcpStream>(addrs.into_iter(), budget, |_addr, remaining| {
+            dialled += 1;
+            std::thread::sleep(remaining);
+            Err(std::io::Error::from(std::io::ErrorKind::TimedOut))
+        })
+        .expect_err("a blackholed dial cannot connect");
+
+        assert_eq!(dialled, 1, "a candidate was dialled past the budget");
+        // TimedOut is not a mid-handshake drop, so the dial is not retried.
+        assert_eq!(error.kind(), std::io::ErrorKind::TimedOut);
+    }
+
+    /// A host that resolves to nothing is reported, not silently treated as a
+    /// success or a timeout.
+    #[test]
+    fn a_host_with_no_addresses_is_an_error() {
+        let error = dial_bounded::<TcpStream>(
+            std::iter::empty(),
+            Duration::from_secs(1),
+            |_addr, _remaining| unreachable!("no candidate to dial"),
+        )
+        .expect_err("no address can connect");
+        assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput);
+    }
+
+    /// TEST-NET-3 (RFC 5737), never routed, so a dial either blackholes or
+    /// fails outright -- never connects. Ignored by default: the blackhole is a
+    /// property of whatever network the suite runs on, and a transparent proxy
+    /// could answer instead. Run with `cargo test -- --ignored` to exercise the
+    /// real `connect_timeout` against an unresponsive peer.
+    #[test]
+    #[ignore = "dials an off-host address"]
+    fn a_dial_to_an_unrouted_address_is_bounded() {
+        let started = Instant::now();
+        connect_timeout("203.0.113.1", 1961, 1).expect_err("TEST-NET-3 cannot connect");
+        let elapsed = started.elapsed();
+        assert!(elapsed < Duration::from_secs(3), "dial took {elapsed:?}");
+    }
+
+    /// A distinct unroutable address per candidate; the port is the Smol Mail
+    /// default and is never reached.
+    fn blackhole(host: u8) -> SocketAddr {
+        SocketAddr::from(([203, 0, 113, host], 1961))
+    }
+
     /// A TCP-level refusal is not a mid-handshake drop: it surfaces
     /// immediately as unreachable, without burning retries.
     #[test]