From 3845142d306503ed5bd3726495d8c42b123d075a Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 20:05:01 +0700 Subject: [PATCH] fix(proto): reject packet lengths usize cannot represent --- crates/irtt-cli/tests/client_multi_target.rs | 4 +- crates/irtt-client/src/async_client.rs | 9 ++- crates/irtt-client/src/client.rs | 7 ++- .../irtt-client/src/client/tests/support.rs | 12 +++- crates/irtt-client/src/session/machine.rs | 18 ++++-- crates/irtt-client/tests/support/mod.rs | 9 ++- crates/irtt-proto/src/echo.rs | 6 +- crates/irtt-proto/src/error.rs | 2 + crates/irtt-proto/src/layout.rs | 59 ++++++++++++++----- crates/irtt-proto/src/request.rs | 8 ++- crates/irtt-proto/tests/directional_codecs.rs | 10 ++-- 11 files changed, 102 insertions(+), 42 deletions(-) diff --git a/crates/irtt-cli/tests/client_multi_target.rs b/crates/irtt-cli/tests/client_multi_target.rs index 2b98d0a..c8ab20d 100644 --- a/crates/irtt-cli/tests/client_multi_target.rs +++ b/crates/irtt-cli/tests/client_multi_target.rs @@ -778,7 +778,7 @@ fn start_synchronized_peer_close_server( ); assert_eq!( packet.len(), - echo_packet_len(false, ¶ms), + echo_packet_len(false, ¶ms).unwrap(), "expected a complete echo probe before synchronized peer-close reply" ); let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap()); @@ -829,7 +829,7 @@ fn start_gated_peer_close_server( } assert_eq!( packet.len(), - echo_packet_len(false, ¶ms), + echo_packet_len(false, ¶ms).unwrap(), "expected a complete echo probe before peer-close reply" ); let seq = u32::from_le_bytes(packet[12..16].try_into().unwrap()); diff --git a/crates/irtt-client/src/async_client.rs b/crates/irtt-client/src/async_client.rs index 7280897..22220e6 100644 --- a/crates/irtt-client/src/async_client.rs +++ b/crates/irtt-client/src/async_client.rs @@ -246,7 +246,7 @@ impl AsyncClient { machine, schedule: None, remote, - recv_buffer: vec![0_u8; recv_buffer_size(false, None)], + recv_buffer: vec![0_u8; recv_buffer_size(false, None)?], applied_traffic_class: None, prepared_open: Some(prepared_open), prepared_probe: None, @@ -820,7 +820,12 @@ impl AsyncClient { return Err(Box::new(PreparedAsyncOpenFailure { primary, machine })); } }; - let recv_buffer_len = recv_buffer_size(self.machine.has_hmac(), Some(negotiated)); + let recv_buffer_len = match recv_buffer_size(self.machine.has_hmac(), Some(negotiated)) { + Ok(size) => size, + Err(primary) => { + return Err(Box::new(PreparedAsyncOpenFailure { primary, machine })); + } + }; let negotiated_traffic_class = match u8::try_from(negotiated.params.dscp) { Ok(traffic_class) => traffic_class, Err(_) => { diff --git a/crates/irtt-client/src/client.rs b/crates/irtt-client/src/client.rs index 9d39d94..064f271 100644 --- a/crates/irtt-client/src/client.rs +++ b/crates/irtt-client/src/client.rs @@ -131,7 +131,7 @@ impl Client { schedule: None, socket, remote, - recv_buffer: vec![0_u8; recv_buffer_size(false, None)], + recv_buffer: vec![0_u8; recv_buffer_size(false, None)?], applied_traffic_class: None, #[cfg(test)] test_hooks: ClientTestHooks::default(), @@ -530,7 +530,10 @@ impl Client { Ok(schedule) => schedule, Err(primary) => return Err(Box::new(PreparedClientOpenFailure { primary, machine })), }; - let recv_buffer_len = recv_buffer_size(self.runtime.has_hmac(), Some(negotiated)); + let recv_buffer_len = match recv_buffer_size(self.runtime.has_hmac(), Some(negotiated)) { + Ok(size) => size, + Err(primary) => return Err(Box::new(PreparedClientOpenFailure { primary, machine })), + }; #[cfg(test)] let recv_buffer_len = self .test_hooks diff --git a/crates/irtt-client/src/client/tests/support.rs b/crates/irtt-client/src/client/tests/support.rs index a3e6be8..0826134 100644 --- a/crates/irtt-client/src/client/tests/support.rs +++ b/crates/irtt-client/src/client/tests/support.rs @@ -1,8 +1,8 @@ use super::*; -pub(crate) use irtt_proto::echo_packet_len; use irtt_proto::{ - encode_echo_reply, encode_open_reply, flags::FLAG_OPEN, flags::FLAG_REPLY, - layout::PacketLayout, Clock, EchoReply, OpenReply, ReceivedStats, StampAt, + echo_packet_len as checked_echo_packet_len, encode_echo_reply, encode_open_reply, + flags::FLAG_OPEN, flags::FLAG_REPLY, layout::PacketLayout, Clock, EchoReply, OpenReply, + ReceivedStats, StampAt, }; use std::{ net::UdpSocket, @@ -72,6 +72,12 @@ pub(super) fn open_reply( .unwrap() } +/// Test params always name a representable length, so the platform +/// representability check cannot fire here. +pub(crate) fn echo_packet_len(hmac: bool, params: &Params) -> usize { + checked_echo_packet_len(hmac, params).expect("test params must have a representable length") +} + pub(super) fn echo_reply_packet( token: u64, seq: u32, diff --git a/crates/irtt-client/src/session/machine.rs b/crates/irtt-client/src/session/machine.rs index 9f09069..cbfe6c7 100644 --- a/crates/irtt-client/src/session/machine.rs +++ b/crates/irtt-client/src/session/machine.rs @@ -823,13 +823,23 @@ impl SessionMachine { } } -pub(crate) fn recv_buffer_size(has_hmac: bool, negotiated: Option<&NegotiatedParams>) -> usize { - match negotiated { - Some(negotiated) => echo_packet_len(has_hmac, &negotiated.params) +/// Sizes the receive buffer for the negotiated layout. +/// +/// Fallible only because a negotiated length wider than `usize` cannot name a +/// buffer at all. Negotiation already rejects a returned length that is +/// negative or larger than this client requested, and the requested length is a +/// `u32`, so the error is unreachable here in practice — it is propagated +/// rather than asserted away so that no 64-bit assumption is baked in. +pub(crate) fn recv_buffer_size( + has_hmac: bool, + negotiated: Option<&NegotiatedParams>, +) -> Result { + Ok(match negotiated { + Some(negotiated) => echo_packet_len(has_hmac, &negotiated.params)? .saturating_add(1) .max(MIN_RECV_BUFFER_SIZE), None => MIN_RECV_BUFFER_SIZE, - } + }) } pub(crate) fn params_from_config(config: &ClientConfig) -> Result { diff --git a/crates/irtt-client/tests/support/mod.rs b/crates/irtt-client/tests/support/mod.rs index d35dfc1..d88e26b 100644 --- a/crates/irtt-client/tests/support/mod.rs +++ b/crates/irtt-client/tests/support/mod.rs @@ -12,13 +12,12 @@ use std::{ use irtt_client::{Client, ClientConfig, ClientEvent, NegotiatedParams, OpenOutcome, SocketConfig}; use irtt_proto::{ - compute_hmac_in_place, decode_request, encode_echo_reply, encode_open_reply, flags, - verify_packet_hmac, Clock, DecodedRequest, DecodedRequestKind, EchoReply, OpenReply, + compute_hmac_in_place, decode_request, echo_packet_len, encode_echo_reply, encode_open_reply, + flags, verify_packet_hmac, Clock, DecodedRequest, DecodedRequestKind, EchoReply, OpenReply, PacketLayout, Params, ProtoError, ReceivedStats, ServerFill, StampAt, TimestampFields, HMAC_SIZE, MAGIC, PROTOCOL_VERSION, }; -pub(super) use irtt_proto::echo_packet_len as test_echo_packet_len; pub use real_irtt::RealIrtServer; const HMAC_OFFSET: usize = 4; @@ -534,6 +533,10 @@ fn open_reply(flags: u8, token: u64, params: &Params, hmac_key: Option<&[u8]>) - .unwrap() } +pub(super) fn test_echo_packet_len(hmac: bool, params: &Params) -> usize { + echo_packet_len(hmac, params).expect("test params must have a representable length") +} + fn echo_reply_packet( token: u64, seq: u32, diff --git a/crates/irtt-proto/src/echo.rs b/crates/irtt-proto/src/echo.rs index 116e8ba..a17731d 100644 --- a/crates/irtt-proto/src/echo.rs +++ b/crates/irtt-proto/src/echo.rs @@ -66,7 +66,7 @@ pub fn encode_echo_reply( hmac_key: Option<&[u8]>, ) -> Result> { let layout = PacketLayout::echo(hmac_key.is_some(), params); - let len = echo_packet_len(hmac_key.is_some(), params); + let len = echo_packet_len(hmac_key.is_some(), params)?; let payload_offset = layout.header_len(); let available_payload_len = len.saturating_sub(payload_offset); if reply.payload.len() > available_payload_len { @@ -186,7 +186,7 @@ fn validate_echo_length(packet: &[u8], params: &Params, layout: PacketLayout) -> actual: packet.len(), }); } - let expected_len = echo_packet_len(layout.hmac, params); + let expected_len = echo_packet_len(layout.hmac, params)?; if packet.len() == expected_len { return Ok(false); } @@ -396,7 +396,7 @@ mod tests { }; params.received_stats = ReceivedStats::Both; let layout = PacketLayout::echo(true, ¶ms); - let mut packet = Vec::with_capacity(echo_packet_len(true, ¶ms)); + let mut packet = Vec::with_capacity(echo_packet_len(true, ¶ms).unwrap()); write_header(&mut packet, FLAG_REPLY | FLAG_HMAC); packet.extend_from_slice(&[0; HMAC_SIZE]); packet.extend_from_slice(&0x7896_b6ab_8771_5213u64.to_le_bytes()); diff --git a/crates/irtt-proto/src/error.rs b/crates/irtt-proto/src/error.rs index a13147b..8a876c5 100644 --- a/crates/irtt-proto/src/error.rs +++ b/crates/irtt-proto/src/error.rs @@ -30,6 +30,8 @@ pub enum ProtoError { InvalidUtf8, #[error("invalid enum value {value} for {name}")] InvalidEnum { name: &'static str, value: i64 }, + #[error("packet length {length} cannot be represented as a buffer length")] + PacketLengthUnrepresentable { length: i64 }, #[error("parameter {tag} length is too large: {length} bytes")] ParameterLengthTooLarge { tag: u64, length: u64 }, #[error("open reply has zero token without close flag")] diff --git a/crates/irtt-proto/src/layout.rs b/crates/irtt-proto/src/layout.rs index 919a11b..7c486a0 100644 --- a/crates/irtt-proto/src/layout.rs +++ b/crates/irtt-proto/src/layout.rs @@ -1,7 +1,7 @@ use crate::{ params::{Params, StampAt}, - HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, TIMESTAMP_SIZE, - TOKEN_SIZE, + ProtoError, Result, HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, + TIMESTAMP_SIZE, TOKEN_SIZE, }; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -123,18 +123,30 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize { /// therefore requests no space beyond the mandatory field block, exactly as /// zero does. /// -/// Only the negative end is bounded. An absurdly large positive length is -/// returned as asked, and encoding a packet that size fails in the allocator -/// rather than here — on a 64-bit target every positive `i64` converts, so that -/// has always been true of a length beyond addressable memory. Capping a -/// negotiated length to something a UDP datagram can actually be belongs with -/// the deferred maximum-packet-length policy, which would apply long before -/// this conversion. -pub fn echo_packet_len(hmac: bool, params: &Params) -> usize { - // Saturating keeps the same answer on a target whose `usize` is narrower - // than the length: still unallocatable, just unrepresentable sooner. - let requested = usize::try_from(params.length.max(0)).unwrap_or(usize::MAX); - echo_header_len(hmac, params).max(requested) +/// # Errors +/// +/// Returns [`ProtoError::PacketLengthUnrepresentable`] for a positive length +/// this platform's `usize` cannot hold. That is a representability check, not a +/// size policy: a wire value that cannot even name a local buffer must not be +/// converted into one that can, because the result is handed to an allocator. +/// It is unreachable on a 64-bit target, where every positive `i64` converts, +/// and is the reason this returns a [`Result`] at all. +/// +/// A ceiling on what a negotiated length may legitimately *be* — an MTU, a +/// resource bound, a maximum packet size — is deliberately not here. That is +/// server and runtime policy, and it belongs where the negotiation happens. +pub fn echo_packet_len(hmac: bool, params: &Params) -> Result { + let header_len = echo_header_len(hmac, params); + let requested = if params.length <= 0 { + // No datagram is shorter than none, so a negative length asks for + // nothing beyond the mandatory field block, exactly as zero does. + 0 + } else { + usize::try_from(params.length).map_err(|_| ProtoError::PacketLengthUnrepresentable { + length: params.length, + })? + }; + Ok(header_len.max(requested)) } #[cfg(test)] @@ -304,9 +316,26 @@ mod tests { p.length = length; assert_eq!( echo_packet_len(false, &p), - expected, + Ok(expected), "unexpected packet length for negotiated length {length}" ); } } + + /// A positive length wider than `usize` must stay an error rather than + /// becoming a saturated buffer size. Only reachable where `usize` is + /// narrower than `i64`; on a 64-bit target every positive `i64` converts. + #[cfg(target_pointer_width = "32")] + #[test] + fn a_length_wider_than_usize_is_rejected_rather_than_saturated() { + let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both); + p.length = 5_000_000_000; + + assert_eq!( + echo_packet_len(false, &p), + Err(ProtoError::PacketLengthUnrepresentable { + length: 5_000_000_000 + }) + ); + } } diff --git a/crates/irtt-proto/src/request.rs b/crates/irtt-proto/src/request.rs index 15085f7..834e37c 100644 --- a/crates/irtt-proto/src/request.rs +++ b/crates/irtt-proto/src/request.rs @@ -84,8 +84,10 @@ pub enum DecodedRequestKind<'a> { /// # Errors /// /// Returns [`ProtoError::PayloadTooLarge`] when an ECHO payload does not fit -/// the negotiated length. A negative negotiated length is not an error: see -/// [`echo_packet_len`], which floors it at the mandatory field block. +/// the negotiated length, and [`ProtoError::PacketLengthUnrepresentable`] when +/// the negotiated length is positive and wider than `usize`. A *negative* +/// negotiated length is not an error: see [`echo_packet_len`], which floors it +/// at the mandatory field block. pub fn encode_request(request: RequestToEncode<'_>, hmac_key: Option<&[u8]>) -> Result> { match request { RequestToEncode::Open { params, no_test } => encode_open(params, no_test, hmac_key), @@ -205,7 +207,7 @@ fn encode_echo( hmac_key: Option<&[u8]>, ) -> Result> { let layout = PacketLayout::echo(hmac_key.is_some(), params); - let len = echo_packet_len(hmac_key.is_some(), params); + let len = echo_packet_len(hmac_key.is_some(), params)?; let payload_offset = layout.header_len(); let available_payload_len = len.saturating_sub(payload_offset); if payload.len() > available_payload_len { diff --git a/crates/irtt-proto/tests/directional_codecs.rs b/crates/irtt-proto/tests/directional_codecs.rs index f450801..25b9a13 100644 --- a/crates/irtt-proto/tests/directional_codecs.rs +++ b/crates/irtt-proto/tests/directional_codecs.rs @@ -316,7 +316,7 @@ fn a_negative_negotiated_length_still_encodes_in_both_directions() { length: -4096, ..Params::default() }; - assert_eq!(echo_packet_len(false, ¶ms), 16); + assert_eq!(echo_packet_len(false, ¶ms), Ok(16)); let request = encode_request(echo_request(¶ms, &[]), None) .expect("a negative negotiated length must still encode a request"); @@ -372,7 +372,7 @@ fn midpoint_params(clock: Clock, payload_space: usize) -> Params { fn midpoint_compat_len(params: &Params, hmac: bool) -> usize { let compat_header = PacketLayout::echo(hmac, params).header_len() + 8; - compat_header.max(echo_packet_len(hmac, params)) + compat_header.max(echo_packet_len(hmac, params).unwrap()) } /// Hand-builds upstream's dual-midpoint ECHO reply: both midpoint fields on @@ -425,7 +425,7 @@ fn midpoint_dual_field_reply_is_accepted_wherever_length_identifies_it() { for (payload_space, expected_extra) in [(0, 8), (1, 7), (4, 4), (7, 1)] { for clock in [Clock::Wall, Clock::Monotonic] { let params = midpoint_params(clock, payload_space); - let normal_len = echo_packet_len(false, ¶ms); + let normal_len = echo_packet_len(false, ¶ms).unwrap(); let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 3, (111, 222), None); assert_eq!( packet.len(), @@ -464,7 +464,7 @@ fn midpoint_dual_field_reply_of_equal_length_is_accepted_but_not_correctable() { for payload_space in [8, 64] { for clock in [Clock::Wall, Clock::Monotonic] { let params = midpoint_params(clock, payload_space); - let normal_len = echo_packet_len(false, ¶ms); + let normal_len = echo_packet_len(false, ¶ms).unwrap(); let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 4, (111, 222), None); assert_eq!( packet.len(), @@ -517,7 +517,7 @@ fn midpoint_dual_field_reply_authenticates_over_full_packet() { fn only_the_exact_compat_length_is_accepted_as_an_extension() { let params = midpoint_params(Clock::Wall, 0); let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 1, (10, 20), None); - let expected_len = echo_packet_len(false, ¶ms); + let expected_len = echo_packet_len(false, ¶ms).unwrap(); assert_eq!(packet.len(), expected_len + 8); // Every other length around the compatibility size stays rejected,