From 68ed5c6e2e7fcad8354cb1a398f8a06d7baa61ab Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 17:16:03 +0700 Subject: [PATCH 1/4] 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: &[], + } ); } -- 2.54.0 From ca2c3dcbd80694663299c9b6c84f0b161aa5b657 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 17:21:23 +0700 Subject: [PATCH 2/4] fix(server): reject unusable timestamp sessions --- crates/irtt-server/AGENTS.md | 24 +++++++++ crates/irtt-server/src/core.rs | 51 +++++++++++++++++-- crates/irtt-server/src/negotiate.rs | 6 +++ crates/irtt-server/src/tests/no_test.rs | 26 +++++++++- crates/irtt-server/src/tests/open.rs | 68 ++++++++++++++++++++++++- 5 files changed, 169 insertions(+), 6 deletions(-) diff --git a/crates/irtt-server/AGENTS.md b/crates/irtt-server/AGENTS.md index 3069944..e4d0dc0 100644 --- a/crates/irtt-server/AGENTS.md +++ b/crates/irtt-server/AGENTS.md @@ -60,6 +60,30 @@ Do not let reply behavior reveal which stage rejected a datagram — in particular, an authentication failure must be indistinguishable from an unknown token. +## An acknowledged open must be executable + +Validate the **final, restricted** parameters before acknowledging an open, not +just the requested ones. Once an open passes admission and negotiation, its +effective parameters must be safe for the server to run. + +- Effective non-none `StampAt` with `Clock::Unspecified` is silently rejected. + Timestamps from no clock lay out no timestamp field, so the session could not + be run. Do **not** synthesize a clock and do **not** rewrite `StampAt` to + none: either answers with a session the client did not ask for. This is + `irtt-rs` policy for nonconforming input, chosen because the clean evidence + records the reference server accepting this combination and then failing on + the first echo. A conforming client always sends a clock when it selects + timestamps, so conforming traffic is unaffected. +- `Clock::Unspecified` alone is valid — it is just an absent tag. Only the + combination is refused, and only an absent tag reaches it, since an explicit + zero Clock is already out of range for the decoder. +- Run the check after negotiation, so that a later policy restricting `StampAt` + to none makes an omitted clock safe without this rule changing. +- Negative `Length` stays an accepted negotiated value, returned unchanged in + the open reply and stored verbatim in the session. It needs no clamping to be + executable: `irtt-proto` floors actual echo packet sizing at the required + protocol field block, so a negative length behaves as zero does. + ## Resource policy Total session and resource state must stay bounded. A single unauthenticated diff --git a/crates/irtt-server/src/core.rs b/crates/irtt-server/src/core.rs index 7320c0e..0e28c6b 100644 --- a/crates/irtt-server/src/core.rs +++ b/crates/irtt-server/src/core.rs @@ -1,8 +1,8 @@ use std::{collections::HashMap, net::SocketAddr}; use irtt_proto::{ - decode_request, encode_open_reply, verify_packet_hmac, DecodedParams, DecodedRequestKind, - OpenReply, Params, FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY, + decode_request, encode_open_reply, verify_packet_hmac, Clock, DecodedParams, + DecodedRequestKind, OpenReply, Params, StampAt, FLAG_CLOSE, FLAG_OPEN, FLAG_REPLY, }; use crate::{ @@ -95,8 +95,9 @@ impl ServerCore { /// datagram is discarded when it is structurally invalid, carries the reply /// flag, disagrees with this server's authentication configuration, fails /// authentication, carries a malformed or out-of-range open parameter - /// payload, would exceed the session bound, or is a request kind this slice - /// does not implement. None of that mutates any live session. + /// payload, negotiates to parameters the server could not execute, would + /// exceed the session bound, or is a request kind this slice does not + /// implement. None of that mutates any live session. /// /// `Ok(None)` is not only a rejection, though. A close request that does /// release its session is answered with nothing as well, because protocol @@ -161,6 +162,11 @@ impl ServerCore { return Ok(None); } let params = negotiate_params(decoded.params, &self.config); + // Deliberately after negotiation, and before either open path: what has + // to be executable is the effective session, not the request. + if !negotiated_params_are_admissible(¶ms) { + return Ok(None); + } if no_test { // No-test validates parameters without running a test: same @@ -284,6 +290,11 @@ fn same_endpoint(session: SocketAddr, peer: SocketAddr) -> bool { /// Applies the open parameter rules the decoder cannot express. /// +/// This judges the *request*, before negotiation, which is why it takes the +/// decoded form with its presence flags. Whether the resulting session is +/// coherent is a separate question, asked after negotiation by +/// [`negotiated_params_are_admissible`]. +/// /// Duration and Interval are accepted when absent, taking the wire default /// zero, and rejected when explicitly present and not positive. The presence /// distinction is why the decoder reports it: testing the value alone would @@ -301,3 +312,35 @@ fn open_params_are_admissible(decoded: &DecodedParams) -> bool { admissible(decoded.presence.duration_ns, decoded.params.duration_ns) && admissible(decoded.presence.interval_ns, decoded.params.interval_ns) } + +/// Whether the *effective* parameters describe a session this server could +/// actually run. +/// +/// This is the last gate before an open is acknowledged, and it asks a +/// different question from [`open_params_are_admissible`]: not whether the +/// request was well formed, but whether the parameters negotiation settled on +/// are coherent. It therefore runs on the negotiated params, after +/// [`negotiate_params`], and covers the no-test path too — a no-test open +/// exists to tell a client what the session would be, so it must not report a +/// session that could not exist. +/// +/// One rule so far: **selecting timestamps requires a specified clock.** A +/// non-none `stamp_at` with [`Clock::Unspecified`] asks for timestamp fields +/// from no clock at all, which lays out no timestamp field and leaves the +/// session with a request the server cannot honor. +/// +/// [`Clock::Unspecified`] means the Clock tag was absent — an explicit zero is +/// already out of range for the decoder — so this cannot be reached by a +/// conforming client, which sends a clock whenever it selects timestamps. The +/// combination is silently rejected rather than repaired: synthesizing a clock +/// would answer with a session the client never asked for, and rewriting +/// `stamp_at` to none would silently drop the measurement it did ask for. +/// Neither is the server's to decide. +/// +/// The check is deliberately *after* negotiation so that a later policy slice +/// restricting timestamps can restrict `stamp_at` to [`StampAt::None`], at +/// which point an absent clock is safe and this predicate holds without +/// changing. +fn negotiated_params_are_admissible(params: &Params) -> bool { + params.stamp_at == StampAt::None || params.clock != Clock::Unspecified +} diff --git a/crates/irtt-server/src/negotiate.rs b/crates/irtt-server/src/negotiate.rs index 6485153..486dfe6 100644 --- a/crates/irtt-server/src/negotiate.rs +++ b/crates/irtt-server/src/negotiate.rs @@ -18,6 +18,12 @@ use crate::config::ServerConfig; /// timestamp allowance, DSCP policy and server-fill allow-list. This function /// is where they belong, which is why the configuration is threaded through /// even though no knob reads it yet. +/// +/// Negotiation rewriting nothing but the version does not mean every open is +/// answered: the core validates what this function returns and silently +/// discards an open whose effective parameters it could not execute. That is a +/// separate step on purpose — this function decides what the session *would* +/// be, and the core decides whether that session can exist. pub(crate) fn negotiate_params(mut requested: Params, _config: &ServerConfig) -> Params { requested.protocol_version = PROTOCOL_VERSION; requested diff --git a/crates/irtt-server/src/tests/no_test.rs b/crates/irtt-server/src/tests/no_test.rs index ea4602d..ea89ca2 100644 --- a/crates/irtt-server/src/tests/no_test.rs +++ b/crates/irtt-server/src/tests/no_test.rs @@ -1,4 +1,4 @@ -use irtt_proto::{verify_packet_hmac, Params}; +use irtt_proto::{verify_packet_hmac, Clock, Params, StampAt}; use super::support::{ client_params, core_with_tokens, expect_no_test_reply, no_test_request, open_request, @@ -74,6 +74,30 @@ fn a_no_test_open_is_rejected_on_the_same_parameters_a_normal_open_is() { assert_eq!(core.session_count(), 0); } +#[test] +fn a_no_test_open_selecting_timestamps_without_a_clock_is_silently_refused() { + // No-test exists to tell a client what the session would be, so it must not + // report a session that could not exist. The effective-parameter check runs + // before the no-test path branches off, so both open forms refuse the same + // incoherent timestamp request. + let tokens = ScriptedTokens::new([TOKEN_A]); + let mut core = core_with_tokens(ServerConfig::default(), tokens.clone()); + + let requested = Params { + clock: Clock::Unspecified, + stamp_at: StampAt::Both, + ..client_params() + }; + + assert_eq!( + core.handle_datagram(peer(), &no_test_request(&requested, None)) + .expect("an unusable timestamp request is not a server error"), + None + ); + assert_eq!(core.session_count(), 0); + assert_eq!(tokens.remaining(), 1, "no token may be drawn"); +} + #[test] fn a_no_test_open_is_still_answered_when_no_session_could_be_created() { // No-test consumes no capacity, so it must keep working exactly where a diff --git a/crates/irtt-server/src/tests/open.rs b/crates/irtt-server/src/tests/open.rs index c2876d7..9a13509 100644 --- a/crates/irtt-server/src/tests/open.rs +++ b/crates/irtt-server/src/tests/open.rs @@ -104,6 +104,10 @@ fn an_empty_parameter_payload_is_answered_with_only_a_version() { ..Params::default() } ); + // An unspecified clock is not itself a defect. Only pairing it with a + // selected `stamp_at` is, so a session requesting no timestamps at all is + // fully valid with no Clock tag. + assert_eq!(reply.params.stamp_at, StampAt::None); assert_eq!(reply.params.clock, Clock::Unspecified); assert_eq!(core.session_count(), 1); } @@ -136,10 +140,61 @@ fn unknown_parameter_tags_are_ignored_and_not_reflected_in_the_reply() { } #[test] -fn this_slice_restricts_nothing_but_the_protocol_version() { +fn selecting_timestamps_without_a_clock_is_silently_refused() { + // Timestamps from no clock at all describe a session the server could not + // run: the negotiated layout carries no timestamp field to fill. The open + // is discarded rather than repaired — synthesizing a clock would answer + // with a session the client never asked for, and dropping `stamp_at` would + // silently discard the measurement it did ask for. + // + // Only an *absent* Clock tag reaches this: an explicit zero is already out + // of range for the decoder, and a conforming client always sends a clock + // when it selects timestamps. + for stamp_at in [ + StampAt::Send, + StampAt::Receive, + StampAt::Both, + StampAt::Midpoint, + ] { + let requested = Params { + clock: Clock::Unspecified, + stamp_at, + ..client_params() + }; + let tokens = ScriptedTokens::new([TOKEN_A]); + let mut core = core_with_tokens(ServerConfig::default(), tokens.clone()); + + let reply = core + .handle_datagram(peer(), &open_request(&requested, None)) + .expect("an unusable timestamp request is not a server error"); + + assert!( + reply.is_none(), + "{stamp_at:?} without a clock must be silent" + ); + assert_eq!( + core.session_count(), + 0, + "{stamp_at:?} without a clock must create no session" + ); + assert_eq!( + tokens.remaining(), + 1, + "{stamp_at:?} without a clock must not consume a token" + ); + } +} + +#[test] +fn negotiation_restricts_nothing_but_the_protocol_version() { // These are the values a later restriction-policy slice will deliberately // start changing. Pinning them now makes that a visible decision rather // than an accident. + // + // This is about what *negotiation* rewrites, which is a separate question + // from whether the core will acknowledge the result: an open whose + // effective parameters are incoherent is still discarded, as + // `selecting_timestamps_without_a_clock_is_silently_refused` shows. let unrestricted = Params { protocol_version: 2, duration_ns: 3_600_000_000_000, @@ -175,6 +230,11 @@ fn negative_and_out_of_range_length_and_dscp_survive_negotiation() { // Length and DSCP are accepted as decoded in this slice, including values a // socket could never carry. Restricting a DSCP the server cannot apply is a // later policy decision, not an admission rule. + // + // A negative length in particular stays negative in both the reply and the + // stored session. It does not need clamping here to be executable: echo + // packet sizing in `irtt-proto` floors it at the required field block, so + // the session this creates can be run as negotiated. for (length, dscp) in [(-1, -1), (0, 0), (1472, 184), (-4096, 512)] { let requested = Params { protocol_version: 1, @@ -193,5 +253,11 @@ fn negative_and_out_of_range_length_and_dscp_survive_negotiation() { assert_eq!(reply.params.length, length); assert_eq!(reply.params.dscp, dscp); assert_eq!(core.session_count(), 1); + let session = core.session(TOKEN_A).expect("the session must be live"); + assert_eq!( + session.params(), + &reply.params, + "the session must hold the negotiated values verbatim" + ); } } -- 2.54.0 From c833b0663d579df44e2ff0e795ecebe426b78b05 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 19:40:17 +0700 Subject: [PATCH 3/4] docs(proto): correct the echo length saturation note --- crates/irtt-proto/src/layout.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/irtt-proto/src/layout.rs b/crates/irtt-proto/src/layout.rs index 5ad0141..919a11b 100644 --- a/crates/irtt-proto/src/layout.rs +++ b/crates/irtt-proto/src/layout.rs @@ -122,10 +122,17 @@ pub fn echo_header_len(hmac: bool, params: &Params) -> usize { /// 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. +/// +/// 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 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. + // 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) } -- 2.54.0 From 3845142d306503ed5bd3726495d8c42b123d075a Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Wed, 12 Aug 2026 20:05:01 +0700 Subject: [PATCH 4/4] 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, -- 2.54.0