From 68ed5c6e2e7fcad8354cb1a398f8a06d7baa61ab Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 17:16:03 +0700 Subject: [PATCH] fix(proto): floor negative echo lengths --- 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 | 11 +--- crates/irtt-client/src/session/machine.rs | 12 ++-- crates/irtt-client/tests/support/mod.rs | 10 +-- crates/irtt-proto/src/echo.rs | 6 +- crates/irtt-proto/src/error.rs | 2 - crates/irtt-proto/src/layout.rs | 66 +++++++++---------- crates/irtt-proto/src/request.rs | 6 +- crates/irtt-proto/tests/directional_codecs.rs | 34 ++++++++-- crates/irtt-proto/tests/request_codec.rs | 35 ++++++---- 12 files changed, 107 insertions(+), 95 deletions(-) diff --git a/crates/irtt-cli/tests/client_multi_target.rs b/crates/irtt-cli/tests/client_multi_target.rs index c8ab20d..2b98d0a 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).unwrap(), + echo_packet_len(false, ¶ms), "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).unwrap(), + echo_packet_len(false, ¶ms), "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 22220e6..7280897 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,12 +820,7 @@ impl AsyncClient { return Err(Box::new(PreparedAsyncOpenFailure { primary, machine })); } }; - 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 recv_buffer_len = recv_buffer_size(self.machine.has_hmac(), Some(negotiated)); 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 064f271..9d39d94 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,10 +530,7 @@ impl Client { Ok(schedule) => schedule, Err(primary) => return Err(Box::new(PreparedClientOpenFailure { primary, machine })), }; - 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 })), - }; + let recv_buffer_len = recv_buffer_size(self.runtime.has_hmac(), Some(negotiated)); #[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 d612696..a3e6be8 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::{ - 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, + encode_echo_reply, encode_open_reply, flags::FLAG_OPEN, flags::FLAG_REPLY, + layout::PacketLayout, Clock, EchoReply, OpenReply, ReceivedStats, StampAt, }; use std::{ net::UdpSocket, @@ -72,11 +72,6 @@ pub(super) fn open_reply( .unwrap() } -pub(crate) fn echo_packet_len(hmac: bool, params: &Params) -> usize { - checked_echo_packet_len(hmac, params) - .expect("test params must have a non-negative packet 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 a7a7775..9f09069 100644 --- a/crates/irtt-client/src/session/machine.rs +++ b/crates/irtt-client/src/session/machine.rs @@ -235,7 +235,6 @@ impl SessionMachine { | irtt_proto::ProtoError::VarintOverflow | irtt_proto::ProtoError::InvalidUtf8 | irtt_proto::ProtoError::InvalidEnum { .. } - | irtt_proto::ProtoError::NegativePacketLength { .. } | irtt_proto::ProtoError::ParameterLengthTooLarge { .. } | irtt_proto::ProtoError::MalformedParams), ) => Err(ClientError::Protocol(error)), @@ -824,16 +823,13 @@ impl SessionMachine { } } -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)? +pub(crate) fn recv_buffer_size(has_hmac: bool, negotiated: Option<&NegotiatedParams>) -> usize { + 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 1d1d592..d35dfc1 100644 --- a/crates/irtt-client/tests/support/mod.rs +++ b/crates/irtt-client/tests/support/mod.rs @@ -12,12 +12,13 @@ use std::{ use irtt_client::{Client, ClientConfig, ClientEvent, NegotiatedParams, OpenOutcome, SocketConfig}; use irtt_proto::{ - compute_hmac_in_place, decode_request, echo_packet_len, encode_echo_reply, encode_open_reply, - flags, verify_packet_hmac, Clock, DecodedRequest, DecodedRequestKind, EchoReply, OpenReply, + compute_hmac_in_place, decode_request, 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; @@ -533,11 +534,6 @@ 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("managed runner test params must have a non-negative packet 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 a17731d..116e8ba 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).unwrap()); + let mut packet = Vec::with_capacity(echo_packet_len(true, ¶ms)); 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 1e69546..a13147b 100644 --- a/crates/irtt-proto/src/error.rs +++ b/crates/irtt-proto/src/error.rs @@ -30,8 +30,6 @@ pub enum ProtoError { InvalidUtf8, #[error("invalid enum value {value} for {name}")] InvalidEnum { name: &'static str, value: i64 }, - #[error("packet length must be non-negative, got {length}")] - NegativePacketLength { 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 e712f18..5ad0141 100644 --- a/crates/irtt-proto/src/layout.rs +++ b/crates/irtt-proto/src/layout.rs @@ -1,7 +1,7 @@ use crate::{ params::{Params, StampAt}, - ProtoError, Result, HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, - TIMESTAMP_SIZE, TOKEN_SIZE, + HEADER_SIZE, HMAC_SIZE, RECV_COUNT_SIZE, RECV_WINDOW_SIZE, SEQ_SIZE, TIMESTAMP_SIZE, + TOKEN_SIZE, }; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -111,13 +111,23 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize { PacketLayout::echo(hmac, params).header_len() } -pub fn echo_packet_len(hmac: bool, params: &Params) -> Result { - let header_len = echo_header_len(hmac, params); - let requested = - usize::try_from(params.length).map_err(|_| ProtoError::NegativePacketLength { - length: params.length, - })?; - Ok(header_len.max(requested)) +/// The datagram length of an ECHO request or reply under `params`. +/// +/// The negotiated length is a floor a peer asks for, never a ceiling: a packet +/// can never be shorter than the field block its negotiated layout requires, so +/// the result is `max(header, requested)`. +/// +/// A negative negotiated length is a legitimate input rather than an encoder +/// error. It is accepted during open and echoed back unchanged, so a session +/// can genuinely carry one, and there is no datagram shorter than none — it +/// therefore requests no space beyond the mandatory field block, exactly as +/// zero does. +pub fn echo_packet_len(hmac: bool, params: &Params) -> usize { + // Saturating rather than wrapping: on a target whose `usize` is narrower + // than the requested length, the largest representable packet is the + // closest honest answer, and the encoder's own limits reject it from there. + let requested = usize::try_from(params.length.max(0)).unwrap_or(usize::MAX); + echo_header_len(hmac, params).max(requested) } #[cfg(test)] @@ -278,32 +288,18 @@ mod tests { } #[test] - fn negotiated_length_never_smaller_than_header() { + fn negotiated_length_is_floored_at_the_required_field_block() { + // The 60-byte header of this layout is the floor. A negative negotiated + // length is not an error: it asks for nothing beyond the mandatory + // fields, exactly as zero does. let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both); - p.length = 20; - assert_eq!(echo_packet_len(false, &p).unwrap(), 60); - p.length = 92; - assert_eq!(echo_packet_len(false, &p).unwrap(), 92); - } - - #[test] - fn checked_packet_len_rejects_negative_requested_length() { - let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both); - p.length = -1; - - assert_eq!( - echo_packet_len(false, &p), - Err(ProtoError::NegativePacketLength { length: -1 }) - ); - } - - #[test] - fn checked_packet_len_preserves_non_negative_header_floor() { - let mut p = params(ReceivedStats::Both, StampAt::Both, Clock::Both); - p.length = 0; - assert_eq!(echo_packet_len(false, &p), Ok(60)); - - p.length = 92; - assert_eq!(echo_packet_len(false, &p), Ok(92)); + for (length, expected) in [(-4096, 60), (-1, 60), (0, 60), (20, 60), (92, 92)] { + p.length = length; + assert_eq!( + echo_packet_len(false, &p), + expected, + "unexpected packet length for negotiated length {length}" + ); + } } } diff --git a/crates/irtt-proto/src/request.rs b/crates/irtt-proto/src/request.rs index 0e2c670..15085f7 100644 --- a/crates/irtt-proto/src/request.rs +++ b/crates/irtt-proto/src/request.rs @@ -84,8 +84,8 @@ pub enum DecodedRequestKind<'a> { /// # Errors /// /// Returns [`ProtoError::PayloadTooLarge`] when an ECHO payload does not fit -/// the negotiated length, and [`ProtoError::NegativePacketLength`] when the -/// negotiated length is negative. +/// the negotiated length. 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 +205,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 cfdd467..f450801 100644 --- a/crates/irtt-proto/tests/directional_codecs.rs +++ b/crates/irtt-proto/tests/directional_codecs.rs @@ -305,6 +305,32 @@ fn echo_payload_is_copied_zero_filled_and_bounded_in_both_directions() { ); } +#[test] +fn a_negative_negotiated_length_still_encodes_in_both_directions() { + // A server accepts a negative negotiated length during open and returns it + // unchanged, so a live session can hold one and must stay executable. There + // is no datagram shorter than none, so both directions fall back to the + // mandatory field block — header (4) + token (8) + sequence (4) — exactly + // as a zero length would. + let params = Params { + length: -4096, + ..Params::default() + }; + assert_eq!(echo_packet_len(false, ¶ms), 16); + + let request = encode_request(echo_request(¶ms, &[]), None) + .expect("a negative negotiated length must still encode a request"); + assert_eq!(request.len(), 16); + + let reply = encode_echo_reply(&echo_reply(¶ms, FLAG_REPLY), ¶ms, None) + .expect("a negative negotiated length must still encode a reply"); + assert_eq!(reply.len(), 16); + assert_eq!( + decode_echo_reply(&reply, ¶ms, None).unwrap().token, + TOKEN + ); +} + // ---------- Upstream midpoint ECHO-reply compatibility ---------- // // Verified upstream irtt 0.9.1 behavior: for single-clock @@ -346,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).unwrap()) + compat_header.max(echo_packet_len(hmac, params)) } /// Hand-builds upstream's dual-midpoint ECHO reply: both midpoint fields on @@ -399,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).unwrap(); + let normal_len = echo_packet_len(false, ¶ms); let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 3, (111, 222), None); assert_eq!( packet.len(), @@ -438,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).unwrap(); + let normal_len = echo_packet_len(false, ¶ms); let packet = build_midpoint_dual_field_reply(¶ms, FLAG_REPLY, 4, (111, 222), None); assert_eq!( packet.len(), @@ -491,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).unwrap(); + let expected_len = echo_packet_len(false, ¶ms); assert_eq!(packet.len(), expected_len + 8); // Every other length around the compatibility size stays rejected, diff --git a/crates/irtt-proto/tests/request_codec.rs b/crates/irtt-proto/tests/request_codec.rs index daa353d..2652732 100644 --- a/crates/irtt-proto/tests/request_codec.rs +++ b/crates/irtt-proto/tests/request_codec.rs @@ -450,21 +450,34 @@ fn encoded_requests_decode_back_to_their_sender_side_identity() { } #[test] -fn echo_encoding_rejects_a_negative_negotiated_length() { +fn echo_encoding_floors_a_negative_negotiated_length_at_the_field_block() { + // A negative negotiated length is accepted during open and echoed back + // unchanged, so a real session can carry one. It must therefore stay + // encodable: it asks for no space beyond the mandatory fields. let params = Params { length: -1, ..Params::default() }; + let packet = encode_request( + RequestToEncode::Echo { + token: TOKEN, + sequence: SEQUENCE, + params: ¶ms, + payload: &[], + }, + None, + ) + .expect("a negative negotiated length must still encode"); + + // header (4) + token (8) + sequence (4). + assert_eq!(packet.len(), 16); + let request = decode_request(&packet).unwrap(); assert_eq!( - encode_request( - RequestToEncode::Echo { - token: TOKEN, - sequence: SEQUENCE, - params: ¶ms, - payload: &[], - }, - None, - ), - Err(ProtoError::NegativePacketLength { length: -1 }) + request.kind, + DecodedRequestKind::Echo { + token: TOKEN, + sequence: SEQUENCE, + tail: &[], + } ); }