From ca6c16b54ea658db0276b4924cf8a2423a1c5649 Mon Sep 17 00:00:00 2001 From: Heiko Bornholdt Date: Thu, 20 Aug 2026 23:17:47 +0200 Subject: [PATCH 1/5] fix(proto): do not coalesce into a too small datagram tail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description Fixes #791. The CONNECTION_CLOSE branch passes its datagram to the next packet number space without checking that there is room for another packet. With `initial_mtu` above 1200 the Initial CONNECTION_CLOSE packet, padded to 1200, leaves a tail behind, and when that tail is under `MIN_PACKET_SPACE` the Handshake space writes into it anyway. In a release build, where the assert is gone, it goes ahead and does that. At `min_mtu` and `initial_mtu` of 1250, `poll_transmit` hands back `size = 1282, segment_size = 1250`. The Handshake packet starts at byte 1200 and ends at 1282, so it runs 32 bytes past the end of the datagram it went into. GSO then sends two datagrams, 1250 and 32 bytes, and the Handshake CONNECTION_CLOSE packet is cut in half between them. Neither half can be decrypted, so the other side drops both, and those last 32 bytes go out as their own useless datagram. My connection still closed, but only because the Initial copy of the CONNECTION_CLOSE is intact in the first datagram. For the fix I did not add a second size check to the CONNECTION_CLOSE branch. Instead I made the loop keep the promise the assert makes: if what is left of the datagram cannot hold a packet, finish the datagram, so the code below starts a new one for the next packet. `TransmitBuf::finish_datagram` does that. For the first datagram of a batch it is `clip_segment_size`; a later one ends up shorter than the segment size, so the batch has to end there and `max_datagrams` is capped. It only does anything in the cases the assert rejects today, so anything that passes the assert works exactly as before. ## Breaking Changes None 🤞. ## Change checklist - [x] Self-review. - [x] Tests if relevant. - [x] This PR was created by a human that thought critically about the proposed change and wrote an as clear and concise description as they could. - [x] This PR isn't slop, and is carefully crafted to do have the intented effect. - [x] `cargo make` passes locally. --- noq-proto/src/connection/mod.rs | 17 ++++++ noq-proto/src/connection/transmit_buf.rs | 18 ++++++ noq-proto/src/tests/mod.rs | 74 ++++++++++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 7971c74d11..bedcbac01e 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -1536,6 +1536,23 @@ impl Connection { }; } + // An earlier packet space hands us its datagram to coalesce into, but what is + // left of it may be too small for a whole packet: a CONNECTION_CLOSE packet + // padded to MIN_INITIAL_SIZE inside a larger datagram leaves such a tail. + // Finish the datagram off, so a new one is started below rather than a packet + // being written past the end of this one. + let datagram_remaining = transmit.datagram_remaining_mut(); + if transmit.datagram_start_offset() < transmit.len() + && datagram_remaining > 0 + && datagram_remaining < MIN_PACKET_SPACE + { + trace!( + datagram_remaining, + "datagram too small for another packet, finishing it" + ); + transmit.finish_datagram(); + } + // We want to send on this space, check congestion control if we can. But only // if we will need to start a new datagram. If we are coalescing into an already // started datagram we do not need to check congestion control again. diff --git a/noq-proto/src/connection/transmit_buf.rs b/noq-proto/src/connection/transmit_buf.rs index 0e63cfdbcd..e9d6168bae 100644 --- a/noq-proto/src/connection/transmit_buf.rs +++ b/noq-proto/src/connection/transmit_buf.rs @@ -153,6 +153,24 @@ impl<'a> TransmitBuf<'a> { self.buf_capacity = self.buf.len(); } + /// Finishes the current datagram, so the next packet starts a new datagram + /// + /// Used when what is left of the current datagram is too small to hold another packet. + /// The datagram then ends up shorter than the segment size: if it is the first + /// datagram of the batch the segment size is clipped to it, otherwise the batch has to + /// end with it, because only the first and the last datagram of a GSO batch may be + /// smaller than the segment size. + pub(super) fn finish_datagram(&mut self) { + debug_assert!(self.num_datagrams > 0); + if self.num_datagrams == 1 { + self.clip_segment_size(); + } else { + // This datagram is shorter than the segment size, so the batch has to end here. + self.buf_capacity = self.buf.len(); + self.max_datagrams = NonZeroUsize::MIN.saturating_add(self.num_datagrams - 1); + } + } + /// Returns the GSO segment size /// /// This is also the maximum size datagrams are allowed to be. The first and last diff --git a/noq-proto/src/tests/mod.rs b/noq-proto/src/tests/mod.rs index 9176505e28..d544a62d91 100644 --- a/noq-proto/src/tests/mod.rs +++ b/noq-proto/src/tests/mod.rs @@ -4937,3 +4937,77 @@ fn regression_initial_coalescing_large_cid() { pair.time += Duration::from_secs(5); pair.drive_client(); // this used to try to build a packet without enough datagram space } + +/// A CONNECTION_CLOSE written while more than one packet number space still has keys hands +/// its datagram to the next space to coalesce into, without checking that what is left of +/// the datagram can hold another packet. +/// +/// A client pads its Initial CONNECTION_CLOSE packet to `MIN_INITIAL_SIZE`. With an +/// `initial_mtu` above `MIN_INITIAL_SIZE` that leaves a tail smaller than a packet, and the +/// Handshake space used to write into that tail: a failed `MIN_PACKET_SPACE` debug assertion +/// in the debug profile, a packet written past the end of the datagram in the release +/// profile. +#[test] +fn close_during_handshake_does_not_coalesce_into_a_too_small_datagram_tail() { + let _guard = subscribe(); + + const MTU: u16 = 1250; + + let mut transport = TransportConfig::default(); + transport.min_mtu(MTU); + transport.initial_mtu(MTU); + transport.mtu_discovery_config(None); + let transport = Arc::new(transport); + + let mut server_cfg = server_config(); + server_cfg.transport = transport.clone(); + let mut pair = Pair::new(Default::default(), server_cfg); + + let mut client_cfg = client_config(); + client_cfg.transport = transport; + + // The client's Initial goes out and the server's flight comes back, but the client only + // *receives* it: it now has Handshake keys while still holding its Initial keys, because + // a client discards those when it sends its first Handshake packet, which it has not + // done yet. + let client_ch = pair.begin_connect(client_cfg); + pair.drive_client(); + pair.drive_server(); + + let now = pair.time; + pair.client.drive_incoming(now); + let events: Vec<_> = pair + .client + .conn_events + .remove(&client_ch) + .unwrap_or_default() + .into_iter() + .collect(); + let conn = pair.client.connections.get_mut(&client_ch).unwrap(); + for event in events { + conn.handle_event(event); + } + conn.close(now, VarInt::from_u32(42), Bytes::from_static(b"bye")); + + let mut buf = Vec::new(); + let transmit = conn + .poll_transmit(now, NonZeroUsize::new(10).expect("known"), &mut buf) + .expect("a CONNECTION_CLOSE is sent"); + + // The Initial CONNECTION_CLOSE packet fills its datagram to MIN_INITIAL_SIZE, and the + // 50 byte tail that leaves in a MTU sized datagram is not handed on: the Handshake and + // Data CONNECTION_CLOSE packets go into a datagram of their own. + let segment_size = transmit + .segment_size + .expect("more than one datagram was written"); + assert_eq!( + segment_size, + usize::from(MIN_INITIAL_SIZE), + "the datagram carrying the Initial CONNECTION_CLOSE ends at MIN_INITIAL_SIZE" + ); + assert!( + transmit.size > segment_size, + "expected a CONNECTION_CLOSE in a further space, got {} bytes", + transmit.size + ); +} From 7f8af87fc66dcc5828505eaf4110d6f132b31dda Mon Sep 17 00:00:00 2001 From: Heiko <789788+HeikoBornholdt@users.noreply.github.com> Date: Sun, 30 Aug 2026 19:03:53 +0200 Subject: [PATCH 2/5] test(proto): pin min_mtu in the close coalescing test The test raised `min_mtu` together with `initial_mtu`. Those are the only sizes where the tail after the Initial CONNECTION_CLOSE depends on which of the two the padding follows, so a fix that pads to `min_mtu` rather than `MIN_INITIAL_SIZE` would fill the datagram and the test would pass without the coalescing check. Pin `min_mtu` to the QUIC minimum instead. The tail is then there in either case: it is the difference between the datagram size and whatever the packet was padded to, and the datagram size is the path MTU. It is set explicitly rather than left at the default so that a change to that default cannot silently change what the test covers. --- noq-proto/src/tests/mod.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/noq-proto/src/tests/mod.rs b/noq-proto/src/tests/mod.rs index d544a62d91..c9aa88ca05 100644 --- a/noq-proto/src/tests/mod.rs +++ b/noq-proto/src/tests/mod.rs @@ -4947,14 +4947,19 @@ fn regression_initial_coalescing_large_cid() { /// Handshake space used to write into that tail: a failed `MIN_PACKET_SPACE` debug assertion /// in the debug profile, a packet written past the end of the datagram in the release /// profile. +/// +/// The tail has to stay below `MIN_PACKET_SPACE`, above that it holds a whole packet and +/// coalescing into it is fine. `min_mtu` at `MIN_INITIAL_SIZE` and an `initial_mtu` of 1250 +/// leave 50 bytes. #[test] fn close_during_handshake_does_not_coalesce_into_a_too_small_datagram_tail() { let _guard = subscribe(); + const MIN_MTU: u16 = MIN_INITIAL_SIZE; const MTU: u16 = 1250; let mut transport = TransportConfig::default(); - transport.min_mtu(MTU); + transport.min_mtu(MIN_MTU); transport.initial_mtu(MTU); transport.mtu_discovery_config(None); let transport = Arc::new(transport); From ab77e438674bad5afb4a48417618676095d6e0c7 Mon Sep 17 00:00:00 2001 From: Heiko <789788+HeikoBornholdt@users.noreply.github.com> Date: Sun, 30 Aug 2026 19:20:22 +0200 Subject: [PATCH 3/5] docs(proto): say where the coalescing space is checked The comment above the assertion and the TODO above the CONNECTION_CLOSE branch both still describe the state before the check existed. Point them at the loop, and leave the parts of the TODO that are still open. --- noq-proto/src/connection/mod.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index bedcbac01e..52f57d2604 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -1612,8 +1612,8 @@ impl Connection { pad_datagram = PadDatagram::No; } - // If coalescing another packet into the existing datagram, there should - // still be enough space for a whole packet. + // A datagram with a tail too small for a packet was finished above, so + // anything we coalesce into still has room for a whole packet. if transmit.datagram_start_offset() < transmit.len() { debug_assert!(transmit.datagram_remaining_mut() >= MIN_PACKET_SPACE); } @@ -1727,15 +1727,13 @@ impl Connection { // Send a close frame in every possible space for robustness, per // RFC9000 "Immediate Close during the Handshake". Don't bother trying // to send anything else. - // TODO(flub): This breaks during the handshake if we can not coalesce - // packets due to space reasons: the next space would either fail a - // debug_assert checking for enough packet space or produce an invalid - // packet. We need to keep track of per-space pending CONNECTION_CLOSE to - // be able to send these across multiple calls to poll_transmit. Then - // check for coalescing space here because initial packets need to be in - // padded datagrams. And also add space checks for CONNECTION_CLOSE in - // space_can_send so it would stop a GSO batch if the datagram is too - // small for another CONNECTION_CLOSE packet. + // The space left in the datagram is checked at the top of the loop, + // which finishes it if the next packet would not fit. + // TODO(flub): We need to keep track of per-space pending CONNECTION_CLOSE to + // be able to send these across multiple calls to poll_transmit. And also + // add space checks for CONNECTION_CLOSE in space_can_send so it would + // stop a GSO batch if the datagram is too small for another + // CONNECTION_CLOSE packet. return PollPathSpaceStatus::WrotePacket { last_packet_number: last_pn, pad_datagram, From ce8a9de630f48131df8f0d87e39d8c8289157f6b Mon Sep 17 00:00:00 2001 From: Heiko <789788+HeikoBornholdt@users.noreply.github.com> Date: Thu, 3 Sep 2026 17:52:40 +0200 Subject: [PATCH 4/5] refactor(proto): compute finish_datagram's max_datagrams without a subtraction --- noq-proto/src/connection/transmit_buf.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noq-proto/src/connection/transmit_buf.rs b/noq-proto/src/connection/transmit_buf.rs index e9d6168bae..f7937754ec 100644 --- a/noq-proto/src/connection/transmit_buf.rs +++ b/noq-proto/src/connection/transmit_buf.rs @@ -167,7 +167,7 @@ impl<'a> TransmitBuf<'a> { } else { // This datagram is shorter than the segment size, so the batch has to end here. self.buf_capacity = self.buf.len(); - self.max_datagrams = NonZeroUsize::MIN.saturating_add(self.num_datagrams - 1); + self.max_datagrams = NonZeroUsize::new(self.num_datagrams).unwrap_or(NonZeroUsize::MIN); } } From 630df68b10764ff22ff217da06141e1bc8998a39 Mon Sep 17 00:00:00 2001 From: Heiko <789788+HeikoBornholdt@users.noreply.github.com> Date: Sat, 5 Sep 2026 14:27:33 +0200 Subject: [PATCH 5/5] refactor(proto): move the too-small-tail check into the CONNECTION_CLOSE branch --- noq-proto/src/connection/mod.rs | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/noq-proto/src/connection/mod.rs b/noq-proto/src/connection/mod.rs index 52f57d2604..7fe47231da 100644 --- a/noq-proto/src/connection/mod.rs +++ b/noq-proto/src/connection/mod.rs @@ -1536,23 +1536,6 @@ impl Connection { }; } - // An earlier packet space hands us its datagram to coalesce into, but what is - // left of it may be too small for a whole packet: a CONNECTION_CLOSE packet - // padded to MIN_INITIAL_SIZE inside a larger datagram leaves such a tail. - // Finish the datagram off, so a new one is started below rather than a packet - // being written past the end of this one. - let datagram_remaining = transmit.datagram_remaining_mut(); - if transmit.datagram_start_offset() < transmit.len() - && datagram_remaining > 0 - && datagram_remaining < MIN_PACKET_SPACE - { - trace!( - datagram_remaining, - "datagram too small for another packet, finishing it" - ); - transmit.finish_datagram(); - } - // We want to send on this space, check congestion control if we can. But only // if we will need to start a new datagram. If we are coalescing into an already // started datagram we do not need to check congestion control again. @@ -1612,8 +1595,9 @@ impl Connection { pad_datagram = PadDatagram::No; } - // A datagram with a tail too small for a packet was finished above, so - // anything we coalesce into still has room for a whole packet. + // If a previous space left a datagram whose tail is too small for another + // packet, its CONNECTION_CLOSE branch finished that datagram, so anything we + // coalesce into still has room for a whole packet. if transmit.datagram_start_offset() < transmit.len() { debug_assert!(transmit.datagram_remaining_mut() >= MIN_PACKET_SPACE); } @@ -1727,13 +1711,17 @@ impl Connection { // Send a close frame in every possible space for robustness, per // RFC9000 "Immediate Close during the Handshake". Don't bother trying // to send anything else. - // The space left in the datagram is checked at the top of the loop, - // which finishes it if the next packet would not fit. // TODO(flub): We need to keep track of per-space pending CONNECTION_CLOSE to // be able to send these across multiple calls to poll_transmit. And also // add space checks for CONNECTION_CLOSE in space_can_send so it would // stop a GSO batch if the datagram is too small for another // CONNECTION_CLOSE packet. + // If what is left of this datagram is too small for another packet, finish + // it so the next space starts a fresh datagram rather than a packet being + // coalesced past its end. + if transmit.datagram_remaining_mut() < MIN_PACKET_SPACE { + transmit.finish_datagram(); + } return PollPathSpaceStatus::WrotePacket { last_packet_number: last_pn, pad_datagram,