Repository navigation
fix(quic): let an oversized datagram step aside behind control frames - #6521
Conversation
A loss probe is clamped to min_mtu. When a queued DATAGRAM fit the path MTU but not the probe, write_frames stepped it aside only if the packet was still empty, so a PING or IMMEDIATE_ACK made it end the probe with no STREAM data, leaving a tiny first datagram that GSO then repeated. Step aside whenever the datagram cannot fit even an empty packet of this size. Also simplify the GSO minimum-segment comment and pin that guard with a test, since the committed fuzz input no longer reaches it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Merging. The maintainer chose /quest-merge. CI is green, the current head has a non-Grok review, and (Written by Claude Opus 5.5) |
Problem
A loss probe is clamped to
min_mtu. When the head of the send schedule is a DATAGRAM that fits the path MTU but not the probe,write_framesstepped it aside only if the packet was still empty. A PING or IMMEDIATE_ACK already in the probe counted as "not empty", so the probe ended with no STREAM data even though a stream was queued behind the datagram.That tiny probe (36 bytes in the fuzz case) became the GSO segment size. moq-dev/moq#6279 ended the batch there to stop the resulting
MIN_PACKET_SPACEassertion, but the short probe itself was the bug.Change
write_frames: a datagram that does not fit the room left ends the packet only when it would fit an empty one. If it can't fit even an empty packet of this size, it steps aside for what fits behind it, whatever control frames are already in the packet.a_clamped_packet_sends_what_fitsnow puts a PING ahead of the clamped packet. It fails without the fix.a_datagram_behind_control_frames_ends_the_packet), so a short first packet with streams queued is still reachable. The newtail_loss_probe_ended_short_ends_the_gso_batchtest reproduces that throughpoll_transmitand hits thedebug_assertwithout the guard. The committed fuzz input stopped reaching this path after moq-dev/moq#5878.Dropped: making the GSO re-check size-aware in place of the guard. A 40 to 86 byte first packet leaves room for a STREAM frame, so a size-aware re-check would pass it, but the allocation still needs
MIN_PACKET_SPACE(87 bytes). The guard already guarantees room for a frame.Tests
just check: lint, compile, and 1521 Rust tests pass.Impact
No public API or wire change. Consumer: every moq-quic connection that sends DATAGRAMs larger than
min_mtualongside streams.The quinn-proto counterpart is quinn-rs/quinn#2949. quinn writes datagrams before streams, so it never hits the step-aside case; only the guard applies there.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code