diff --git a/core/src/tcp.rs b/core/src/tcp.rs index 63f17fe..b462350 100644 --- a/core/src/tcp.rs +++ b/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 { + 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( + addrs: impl Iterator, + budget: Duration, + mut dial: impl FnMut(&SocketAddr, Duration) -> std::io::Result, +) -> std::io::Result { + 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::(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::(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::( + 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]