From d1b6b8d950f17f1a078db7f945a0dd8df5d402ec Mon Sep 17 00:00:00 2001 From: disarray09 Date: Fri, 31 Jul 2026 10:41:31 -0400 Subject: [PATCH] Bound buffered frames by their declared length, before reading the payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FrameDecoder accumulates into a BufList until a frame's whole declared payload has arrived. Consuming each chunk is what releases the flow-control credit for the next one, so credit is returned as fast as the decoder accumulates and no transport window bounds the buffer. A peer declaring a large HEADERS is held in memory in full before max_field_section_size can reject it, and a peer declaring a large unknown frame is held in full before it can be ignored. The bound is the sender's rate times how long the connection is allowed to live. Frame::decode now decides from the type and the declared length alone: * DATA is untouched — it carries a length and streams its body. * A reserved HTTP/2 type is a connection error decided by the type, so its payload is no longer waited for. * An unknown type returns SkipUnknown with its length, and FrameDecoder discards the payload as it arrives instead of storing it to drop it. RFC 9114 asks that unknown frames be ignored; ignoring one used to cost its declared length. Refusing it instead would bound memory just as well and would not be conformant. * A frame that must be buffered to be parsed is refused with TooLarge if its declared length exceeds the decoder's ceiling, before any of it is read, and the accumulated state is released rather than left for the caller. The ceiling defaults to 1 MiB — generous beside anything legitimate, since field sections run to kilobytes and the control frames to tens of bytes, and finite, which is the property that was missing. An endpoint that advertises a larger max_field_section_size raises it to match: the ceiling is a memory bound, and must never be tighter than what the endpoint has told peers it accepts, or a field section would be refused from its encoded length before QPACK can report the uncompressed size that RFC 9114 4.2.2 is written in terms of. What the refusal means depends on where the frame arrived, so FrameStreamError reports it rather than deciding: an oversized HEADERS on a request stream is HeaderTooBig, a message-level rejection that leaves the connection alone, while on the control stream — where every legitimate frame is tens of bytes and the stream itself is critical — it is H3_EXCESSIVE_LOAD. Two existing tests changed behaviour rather than shape: unknown_frame_type and reserved_frame now observe SkipUnknown from the header instead of UnknownFrame after the payload. --- examples/server.rs | 4 +- h3/src/client/connection.rs | 6 +- h3/src/client/stream.rs | 14 +- h3/src/connection.rs | 49 ++++- h3/src/error/connection_error_creators.rs | 16 ++ h3/src/frame.rs | 225 +++++++++++++++++++++- h3/src/proto/frame.rs | 178 ++++++++++++++++- h3/src/server/connection.rs | 6 +- h3/src/server/request.rs | 10 + 9 files changed, 486 insertions(+), 22 deletions(-) diff --git a/examples/server.rs b/examples/server.rs index a38c873a..ee5ec568 100644 --- a/examples/server.rs +++ b/examples/server.rs @@ -112,7 +112,9 @@ async fn main() -> Result<(), Box> { Ok(conn) => { info!("new connection established"); - let mut h3_conn = h3::server::Connection::new(h3_quinn::Connection::new(conn)) + let mut h3_conn = h3::server::builder() + .max_field_section_size(1024 * 16) + .build(h3_quinn::Connection::new(conn)) .await .unwrap(); diff --git a/h3/src/client/connection.rs b/h3/src/client/connection.rs index 82b800e3..a5a60445 100644 --- a/h3/src/client/connection.rs +++ b/h3/src/client/connection.rs @@ -216,7 +216,11 @@ where let request_stream = RequestStream { inner: connection::RequestStream::new( - FrameStream::new(BufRecvStream::new(stream)), + FrameStream::new_with_max_buffered_frame( + BufRecvStream::new(stream), + self.max_field_section_size + .max(crate::frame::DEFAULT_MAX_BUFFERED_FRAME), + ), self.max_field_section_size, self.conn_state.clone(), self.send_grease_frame, diff --git a/h3/src/client/stream.rs b/h3/src/client/stream.rs index b1a9641f..90f41907 100644 --- a/h3/src/client/stream.rs +++ b/h3/src/client/stream.rs @@ -12,6 +12,7 @@ use crate::{ internal_error::InternalConnectionError, Code, StreamError, }, + frame::FrameStreamError, proto::{frame::Frame, headers::Header}, qpack, quic::{self}, @@ -97,8 +98,17 @@ where /// [`recv_data()`]: #method.recv_data #[cfg_attr(feature = "tracing", instrument(skip_all, level = "trace"))] pub async fn recv_response(&mut self) -> Result, StreamError> { - let mut frame = future::poll_fn(|cx| self.inner.stream.poll_next(cx)) - .await + let frame = future::poll_fn(|cx| self.inner.stream.poll_next(cx)).await; + if matches!( + frame, + Err(FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + }) + ) { + self.inner.stop_sending(Code::H3_REQUEST_CANCELLED); + } + let mut frame = frame .map_err(|e| self.handle_frame_stream_error_on_request_stream(e))? .ok_or_else(|| { //= https://www.rfc-editor.org/rfc/rfc9114#section-4.1 diff --git a/h3/src/connection.rs b/h3/src/connection.rs index 44302f1a..b19ff254 100644 --- a/h3/src/connection.rs +++ b/h3/src/connection.rs @@ -565,6 +565,22 @@ where ), ))); } + Err(FrameStreamError::PayloadTooLarge { + frame_type: _ty, + actual_size: _actual, + max_size: _max, + }) => { + // Unlike a request stream, there is no message to reject here: + // every frame the control stream may legitimately carry is tens + // of bytes, and the stream itself is critical. + return Poll::Ready(Err(self.handle_connection_error( + InternalConnectionError::new( + Code::H3_EXCESSIVE_LOAD, + "control frame declared a payload larger than this endpoint will buffer" + .to_string(), + ), + ))); + } Err(FrameStreamError::Proto(frame_error)) => { return Poll::Ready(Err(self.handle_connection_error( InternalConnectionError::got_frame_error(frame_error), @@ -858,9 +874,18 @@ where if !self.stream.has_data() { match ready!(self.stream.poll_next(cx)) { Err(frame_stream_error) => { + if matches!( + frame_stream_error, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); + } return Poll::Ready(Err( self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )) + )); } Ok(None) => return Poll::Ready(Ok(None)), Ok(Some(Frame::Headers(encoded))) => { @@ -919,9 +944,18 @@ where } else { match ready!(self.stream.poll_next(cx)) { Err(frame_stream_error) => { + if matches!( + frame_stream_error, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); + } return Poll::Ready(Err( self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )) + )); } Ok(None) => return Poll::Ready(Ok(None)), Ok(Some(Frame::Headers(encoded))) => encoded, @@ -967,9 +1001,18 @@ where match self.stream.poll_next(cx) { Poll::Ready(Err(frame_stream_error)) => { + if matches!( + frame_stream_error, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.stream.stop_sending(Code::H3_REQUEST_CANCELLED); + } return Poll::Ready(Err( self.handle_frame_stream_error_on_request_stream(frame_stream_error) - )) + )); } Poll::Ready(Ok(Some(trailing_frame))) => { // Received a known frame after trailers -> fail. diff --git a/h3/src/error/connection_error_creators.rs b/h3/src/error/connection_error_creators.rs index 93193e20..c99e95fc 100644 --- a/h3/src/error/connection_error_creators.rs +++ b/h3/src/error/connection_error_creators.rs @@ -197,6 +197,22 @@ where FrameStreamError::Proto(frame_error) => self.handle_connection_error_on_stream( InternalConnectionError::got_frame_error(frame_error), ), + // A HEADERS limit is per message: rejecting one request must not + // close unrelated streams on the same connection. + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + actual_size, + max_size, + } => StreamError::HeaderTooBig { + actual_size, + max_size, + }, + FrameStreamError::PayloadTooLarge { .. } => { + self.handle_connection_error_on_stream(InternalConnectionError::new( + Code::H3_EXCESSIVE_LOAD, + "frame declared a payload larger than this endpoint will buffer".to_string(), + )) + } FrameStreamError::UnexpectedEnd => { self.handle_connection_error_on_stream(InternalConnectionError::new( Code::H3_FRAME_ERROR, diff --git a/h3/src/frame.rs b/h3/src/frame.rs index f9efc57b..61fd5fdc 100644 --- a/h3/src/frame.rs +++ b/h3/src/frame.rs @@ -29,9 +29,23 @@ pub struct FrameStream { impl FrameStream { pub fn new(stream: BufRecvStream) -> Self { + Self::new_with_max_buffered_frame(stream, DEFAULT_MAX_BUFFERED_FRAME) + } + + /// `max_buffered_frame` is a memory bound, not a policy limit: it must not + /// be tighter than what this endpoint has advertised it will accept, or a + /// field section would be refused from its encoded length before QPACK can + /// report the uncompressed size that [`RFC 9114 §4.2.2`] is written in + /// terms of, and that a peer needs in order to retry. + /// + /// [`RFC 9114 §4.2.2`]: https://www.rfc-editor.org/rfc/rfc9114#section-4.2.2 + pub(crate) fn new_with_max_buffered_frame( + stream: BufRecvStream, + max_buffered_frame: u64, + ) -> Self { Self { stream, - decoder: FrameDecoder::default(), + decoder: FrameDecoder::new(max_buffered_frame), remaining_data: 0, } } @@ -88,7 +102,7 @@ where Poll::Ready(false) => continue, Poll::Pending => return Poll::Pending, Poll::Ready(true) => { - if self.stream.buf_mut().has_remaining() { + if self.stream.buf_mut().has_remaining() || self.decoder.is_incomplete() { // Reached the end of receive stream, but there is still some data: // The frame is incomplete. return Poll::Ready(Err(FrameStreamError::UnexpectedEnd)); @@ -212,12 +226,51 @@ where } } -#[derive(Default)] +/// Default ceiling on the declared length of a frame that must be buffered in +/// full before it can be parsed. +/// +/// Generous next to anything legitimate — field sections in the wild are a few +/// kilobytes, and the control frames are tens of bytes — and finite, which is +/// the property that was missing. Nothing that works today against a +/// well-behaved peer stops working. +pub const DEFAULT_MAX_BUFFERED_FRAME: u64 = 1024 * 1024; + pub struct FrameDecoder { expected: Option, + /// Payload bytes of an unknown frame still to be discarded. + /// + /// Ignoring an unknown frame used to mean buffering it in full and then + /// dropping it, because the type was only recognised once `Frame::decode` + /// had the whole payload. It is recognised from the header now, and this is + /// what is left to throw away as it arrives. + skipping: u64, + /// See [`DEFAULT_MAX_BUFFERED_FRAME`]. + max_buffered: u64, +} + +impl Default for FrameDecoder { + fn default() -> Self { + Self { + expected: None, + skipping: 0, + max_buffered: DEFAULT_MAX_BUFFERED_FRAME, + } + } } impl FrameDecoder { + fn new(max_buffered: u64) -> Self { + Self { + expected: None, + skipping: 0, + max_buffered, + } + } + + fn is_incomplete(&self) -> bool { + self.expected.is_some() || self.skipping > 0 + } + fn decode( &mut self, src: &mut BufList, @@ -229,6 +282,18 @@ impl FrameDecoder { return Ok(None); } + // An unknown frame's payload, discarded as it arrives rather than + // after all of it has been stored. + if self.skipping > 0 { + let discard = self.skipping.min(src.remaining() as u64); + src.advance(discard as usize); + self.skipping -= discard; + if self.skipping > 0 { + return Ok(None); + } + continue; + } + if let Some(min) = self.expected { if src.remaining() < min { return Ok(None); @@ -237,12 +302,12 @@ impl FrameDecoder { let (pos, decoded) = { let mut cur = src.cursor(); - let decoded = Frame::decode(&mut cur); + let decoded = Frame::decode(&mut cur, self.max_buffered); (cur.position(), decoded) }; match decoded { - Err(frame::FrameError::UnknownFrame(_ty)) => { + Err(frame::FrameError::SkipUnknown { ty: _ty, len }) => { //= https://www.rfc-editor.org/rfc/rfc9114#section-4.1 //# Frames of unknown types (Section 9), including reserved frames //# (Section 7.2.8) MAY be sent on a request or push stream before, @@ -250,6 +315,42 @@ impl FrameDecoder { //= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.8 //# Endpoints MUST //# NOT consider these frames to have any meaning upon receipt. + #[cfg(feature = "tracing")] + trace!("ignore unknown frame type {:#x}, {} bytes", _ty, len); + + // Only the header is consumed here. The payload goes to the + // branch above as it arrives, so ignoring an enormous + // unknown frame costs nothing to hold. + src.advance(pos); + self.expected = None; + self.skipping = len; + continue; + } + Err(frame::FrameError::TooLarge { + ty: _ty, + len: _len, + limit: _limit, + }) => { + #[cfg(feature = "tracing")] + trace!( + "frame type {:#x} declared {} bytes, over the {} ceiling", + _ty, + _len, + _limit + ); + + // Nothing of this frame was accumulated, and what came + // before it is released rather than left for a caller that + // is about to tear the stream down anyway. + src.advance(src.remaining()); + self.expected = None; + return Err(FrameStreamError::PayloadTooLarge { + frame_type: _ty, + actual_size: _len, + max_size: _limit, + }); + } + Err(frame::FrameError::UnknownFrame(_ty)) => { #[cfg(feature = "tracing")] trace!("ignore unknown frame type {:#x}", _ty); @@ -304,6 +405,14 @@ pub enum FrameStreamError { Proto(FrameProtocolError), Quic(StreamErrorIncoming), UnexpectedEnd, + /// A frame that must be buffered to be parsed declared a payload past the + /// decoder's ceiling. The owner decides from the frame context whether + /// this is a message-level rejection or a connection error. + PayloadTooLarge { + frame_type: u64, + actual_size: u64, + max_size: u64, + }, } #[derive(Debug, PartialEq)] @@ -399,6 +508,81 @@ mod tests { assert_matches!(decoder.decode(&mut buf), Ok(None)); } + #[test] + fn skips_unknown_payload_incrementally_then_decodes_next_frame() { + // Unknown type 22, declared payload length 63. Only ten payload bytes + // arrive with the header; they must be discarded, not retained. + let mut first = vec![22, 63]; + first.extend([0_u8; 10]); + let mut buf = BufList::from(Bytes::from(first)); + let mut decoder = FrameDecoder::default(); + + assert_matches!(decoder.decode(&mut buf), Ok(None)); + assert_eq!(buf.remaining(), 0); + assert_eq!(decoder.skipping, 53); + + // The rest of the unknown payload and a valid DATA header arrive + // together. The decoder must discard exactly the former and preserve + // framing for the latter. + let mut rest = vec![0_u8; 53]; + rest.extend([0, 4]); + buf.push(Bytes::from(rest)); + + assert_matches!( + decoder.decode(&mut buf), + Ok(Some(Frame::Data(PayloadLen(4)))) + ); + assert_eq!(decoder.skipping, 0); + assert_eq!(buf.remaining(), 0); + } + + #[test] + fn an_enormous_unknown_frame_costs_nothing_to_ignore() { + // 0x21 is the first of the reserved GREASE frame types: an unknown + // frame is not by itself an attack, it is something h3 sends on + // purpose, and it must be ignored whatever length it declares. + // Ignoring it is what must not cost the declared length. + const DECLARED: u64 = 4 * 1024 * 1024 * 1024; + const CHUNK: usize = 1024 * 1024; + + let mut header = BytesMut::new(); + VarInt::from_u64(0x21).unwrap().encode(&mut header); + VarInt::from_u64(DECLARED).unwrap().encode(&mut header); + + let mut buf = BufList::from(header.freeze()); + let mut decoder = FrameDecoder::default(); + + // The header on its own settles what happens to the payload. Note that + // the declared length exceeds u32 and, on a 32-bit target, `usize`: + // what is left to discard is counted, never sized into an allocation. + assert_matches!(decoder.decode(&mut buf), Ok(None)); + assert_eq!(decoder.skipping, DECLARED); + assert_eq!(buf.remaining(), 0); + + // Stream the payload past the decoder. `BufList::advance` drops the + // chunks it consumes, so "nothing remaining" is "nothing retained": + // the peak cost here is one chunk, not the four gigabytes declared. + let chunk = Bytes::from(vec![0_u8; CHUNK]); + for _ in 0..(DECLARED / CHUNK as u64) - 1 { + buf.push(chunk.clone()); + assert_matches!(decoder.decode(&mut buf), Ok(None)); + assert_eq!(buf.remaining(), 0, "a payload being ignored was retained"); + } + assert_eq!(decoder.skipping, CHUNK as u64); + + // The last of the ignored payload arrives in the same chunk as a real + // frame. The decoder has to discard exactly the former — one byte + // either way desynchronises the stream — and then parse the latter. + let mut tail = BytesMut::with_capacity(CHUNK + 16); + tail.put_bytes(0, CHUNK); + Frame::headers(&b"salut"[..]).encode_with_payload(&mut tail); + buf.push(tail.freeze()); + + assert_matches!(decoder.decode(&mut buf), Ok(Some(Frame::Headers(_)))); + assert_eq!(decoder.skipping, 0); + assert_eq!(buf.remaining(), 0); + } + // FrameStream macro_rules! assert_poll_matches { @@ -496,6 +680,37 @@ mod tests { ); } + #[tokio::test] + async fn truncated_unknown_frame_is_an_unexpected_end() { + let mut recv = FakeRecv::default(); + // Unknown type 22 declares four payload bytes but sends only two. + recv.chunk(Bytes::from_static(&[22, 4, b'a', b'b'])); + let mut stream: FrameStream<_, ()> = FrameStream::new(BufRecvStream::new(recv)); + + assert_poll_matches!( + |cx| stream.poll_next(cx), + Err(FrameStreamError::UnexpectedEnd) + ); + } + + #[tokio::test] + async fn oversized_buffered_frame_fails_from_header_alone() { + let mut recv = FakeRecv::default(); + // HEADERS declares 63 bytes and sends no payload. + recv.chunk(Bytes::from_static(&[1, 63])); + let mut stream: FrameStream<_, ()> = + FrameStream::new_with_max_buffered_frame(BufRecvStream::new(recv), 62); + + assert_poll_matches!( + |cx| stream.poll_next(cx), + Err(FrameStreamError::PayloadTooLarge { + frame_type: 1, + actual_size: 63, + max_size: 62 + }) + ); + } + #[tokio::test] #[should_panic( expected = "There is still data to read, please call poll_data() until it returns None" diff --git a/h3/src/proto/frame.rs b/h3/src/proto/frame.rs index a09520bb..5e51b373 100644 --- a/h3/src/proto/frame.rs +++ b/h3/src/proto/frame.rs @@ -25,6 +25,27 @@ pub enum FrameError { Settings(SettingsError), InvalidStreamId(InvalidStreamId), InvalidPushId(InvalidPushId), + /// A buffered frame declared a payload larger than the configured ceiling. + /// + /// Returned from the declared length alone, before any of the payload is + /// accumulated — which is the whole point. Every other error here can be + /// reached only once the frame is in memory. + TooLarge { + ty: u64, + len: u64, + limit: u64, + }, + /// An unknown frame type, reported *before* its payload has been read so + /// the caller can discard it as it arrives. + /// + /// Distinct from [`FrameError::UnknownFrame`], which is returned once the + /// payload is already buffered. RFC 9114 requires unknown frames to be + /// ignored; accumulating one in full in order to ignore it satisfies the + /// letter and not the substance. + SkipUnknown { + ty: u64, + len: u64, + }, } impl std::error::Error for FrameError {} @@ -40,6 +61,14 @@ impl fmt::Display for FrameError { FrameError::Settings(x) => write!(f, "invalid settings: {}", x), FrameError::InvalidStreamId(x) => write!(f, "{}", x), FrameError::InvalidPushId(x) => write!(f, "{}", x), + FrameError::TooLarge { ty, len, limit } => write!( + f, + "frame 0x{:x} declared {} bytes, over the {} byte ceiling", + ty, len, limit + ), + FrameError::SkipUnknown { ty, len } => { + write!(f, "frame 0x{:x} ignored, {} bytes skipped", ty, len) + } } } } @@ -79,7 +108,22 @@ impl Frame { pub const MAX_ENCODED_SIZE: usize = VarInt::MAX_SIZE * 7; /// Decodes a Frame from the stream according to - pub fn decode(buf: &mut T) -> Result { + /// + /// `max_buffered` bounds the declared payload length of a frame this + /// function has to hold in memory to parse. It is checked against the + /// length field, before the caller is ever told to accumulate — which is + /// the difference that matters. Previously the completeness check came + /// first, so a peer declaring a large `HEADERS` had its payload buffered in + /// full before `max_field_section_size` could reject it, and a peer + /// declaring a large *unknown* frame had it buffered in full before it + /// could be ignored. Neither was bounded by flow control: the caller + /// consumes each chunk as it arrives, which is what releases the credit for + /// the next one. + /// + /// `DATA` is unaffected. Its payload is never held here — the frame carries + /// only a length and the body is streamed — so the ceiling does not apply + /// to it and request bodies keep whatever bound their reader already has. + pub fn decode(buf: &mut T, max_buffered: u64) -> Result { let remaining = buf.remaining(); let ty = FrameType::decode(buf).map_err(|_| FrameError::Incomplete(remaining + 1))?; @@ -101,6 +145,28 @@ impl Frame { return Ok(Frame::Data((len as usize).into())); } + // Everything below is decided from the type and the declared length + // alone, before anything asks the caller to hold a payload. + + // A reserved HTTP/2 type is a connection error whatever it carries, so + // reading what it carries is pure cost. + if ty.is_reserved() { + return Err(FrameError::UnsupportedFrame(ty.0)); + } + + // Unknown: to be ignored, and ignoring it must not mean storing it. + if !ty.is_buffered() { + return Err(FrameError::SkipUnknown { ty: ty.0, len }); + } + + if len > max_buffered { + return Err(FrameError::TooLarge { + ty: ty.0, + len, + limit: max_buffered, + }); + } + if buf.remaining() < len as usize { return Err(FrameError::Incomplete(2 + len as usize)); } @@ -317,6 +383,43 @@ impl FrameType { pub fn grease() -> Self { FrameType(fastrand::u64(0..0x210842108421083) * 0x1f + 0x21) } + + /// Whether parsing this type requires its whole payload in memory. + /// + /// The list is exactly the arms of the match in [`Frame::decode`] that read + /// from `payload`. `DATA` is absent because its body is streamed rather + /// than held, and `WEBTRANSPORT_BI_STREAM` because it carries no length at + /// all — both are handled before this is consulted. + /// + /// Kept beside the type list rather than derived from the match so that + /// adding a frame type to one and not the other is a compile-time + /// omission with a visible home, instead of a type that silently becomes + /// unbounded. + fn is_buffered(self) -> bool { + matches!( + self, + FrameType::HEADERS + | FrameType::SETTINGS + | FrameType::CANCEL_PUSH + | FrameType::PUSH_PROMISE + | FrameType::GOAWAY + | FrameType::MAX_PUSH_ID + ) + } + + /// The HTTP/2 types RFC 9114 §7.2.8 requires be treated as a connection + /// error of type `H3_FRAME_UNEXPECTED`. + /// + /// Decided by the type, so the payload is never worth reading. + fn is_reserved(self) -> bool { + matches!( + self, + FrameType::H2_PRIORITY + | FrameType::H2_PING + | FrameType::H2_WINDOW_UPDATE + | FrameType::H2_CONTINUATION + ) + } } #[derive(Copy, Clone, Eq, PartialEq, Debug)] @@ -619,31 +722,85 @@ mod tests { use assert_matches::assert_matches; use std::io::Cursor; + /// An unknown type is now reported from its header, with the payload left + /// for the caller to discard as it arrives. + /// + /// It used to be reported as `UnknownFrame` *after* the whole payload had + /// been buffered, which is how ignoring an unknown frame came to cost its + /// full declared length in memory. `FrameDecoder` does the skipping, and + /// `frame::tests` covers reading the frame that follows. #[test] fn unknown_frame_type() { let mut buf = Cursor::new(&[22, 4, 0, 255, 128, 0, 3, 1, 2]); - assert_matches!(Frame::decode(&mut buf), Err(FrameError::UnknownFrame(22))); - assert_matches!(Frame::decode(&mut buf), Ok(Frame::CancelPush(PushId(2)))); + assert_matches!( + Frame::decode(&mut buf, u64::MAX), + Err(FrameError::SkipUnknown { ty: 22, len: 4 }) + ); + } + + /// The ceiling is applied to the declared length, with no payload present. + /// + /// The buffer holds a two-byte header and nothing else, so a decoder that + /// checked completeness first would answer `Incomplete` and wait — which is + /// precisely the accumulation this exists to prevent. + #[test] + fn oversized_buffered_frame_is_refused_from_its_length_alone() { + // HEADERS, declared length 63, no payload. 63 is the largest value a + // single-byte QUIC varint holds, which keeps the fixture two bytes. + let mut buf = Cursor::new(&[1, 63]); + assert_matches!( + Frame::decode(&mut buf, 62), + Err(FrameError::TooLarge { + ty: 1, + len: 63, + limit: 62 + }) + ); + + // Exactly at the ceiling is not over it: this must reach the ordinary + // completeness path instead. + let mut buf = Cursor::new(&[1, 63]); + assert_matches!(Frame::decode(&mut buf, 63), Err(FrameError::Incomplete(_))); + } + + /// A reserved HTTP/2 type is a connection error decided by the type, so its + /// payload must never be waited for either. + #[test] + fn a_reserved_frame_is_refused_without_its_payload() { + // H2_PING (0x6), declared length 63, no payload. + let mut buf = Cursor::new(&[6, 63]); + assert_matches!( + Frame::decode(&mut buf, u64::MAX), + Err(FrameError::UnsupportedFrame(6)) + ); + } + + /// DATA is untouched: it declares a length and streams its body, so the + /// ceiling does not apply and request bodies keep their own bound. + #[test] + fn data_frames_are_not_subject_to_the_buffered_ceiling() { + let mut buf = Cursor::new(&[0, 63]); + assert_matches!(Frame::decode(&mut buf, 1), Ok(Frame::Data(PayloadLen(63)))); } #[test] fn len_unexpected_end() { let mut buf = Cursor::new(&[0, 255]); - let decoded = Frame::decode(&mut buf); + let decoded = Frame::decode(&mut buf, u64::MAX); assert_matches!(decoded, Err(FrameError::Incomplete(3))); } #[test] fn type_unexpected_end() { let mut buf = Cursor::new(&[255]); - let decoded = Frame::decode(&mut buf); + let decoded = Frame::decode(&mut buf, u64::MAX); assert_matches!(decoded, Err(FrameError::Incomplete(2))); } #[test] fn buffer_too_short() { let mut buf = Cursor::new(&[4, 4, 0, 255, 128]); - let decoded = Frame::decode(&mut buf); + let decoded = Frame::decode(&mut buf, u64::MAX); assert_matches!(decoded, Err(FrameError::Incomplete(6))); } @@ -653,7 +810,7 @@ mod tests { assert_eq!(&buf, &wire); let mut read = Cursor::new(&buf); - let decoded = Frame::decode(&mut read).unwrap(); + let decoded = Frame::decode(&mut read, u64::MAX).unwrap(); assert_eq!(check_frame, decoded); } @@ -756,7 +913,10 @@ mod tests { VarInt::from_u32(0x21 + 2 * 0x1f).encode(&mut raw); raw.extend(&[6, 0, 255, 128, 0, 250, 218]); let mut buf = Cursor::new(&raw); - let decoded = Frame::decode(&mut buf); - assert_matches!(decoded, Err(FrameError::UnknownFrame(95))); + let decoded = Frame::decode(&mut buf, u64::MAX); + // Reported from the header now rather than after the payload has been + // buffered: a GREASE type is unknown, and ignoring an unknown frame no + // longer costs its declared length in memory. + assert_matches!(decoded, Err(FrameError::SkipUnknown { ty: 95, len: 6 })); } } diff --git a/h3/src/server/connection.rs b/h3/src/server/connection.rs index 1a3a35d8..f4dee369 100644 --- a/h3/src/server/connection.rs +++ b/h3/src/server/connection.rs @@ -127,7 +127,11 @@ where pub async fn accept(&mut self) -> Result>, ConnectionError> { // Accept the incoming stream let stream = match poll_fn(|cx| self.poll_accept_request_stream_internal(cx)).await? { - Some(s) => FrameStream::new(BufRecvStream::new(s)), + Some(s) => FrameStream::new_with_max_buffered_frame( + BufRecvStream::new(s), + self.max_field_section_size + .max(crate::frame::DEFAULT_MAX_BUFFERED_FRAME), + ), None => { // We always send a last GoAway frame to the client, so it knows which was the last // non-rejected request. diff --git a/h3/src/server/request.rs b/h3/src/server/request.rs index 01599921..bf291aa5 100644 --- a/h3/src/server/request.rs +++ b/h3/src/server/request.rs @@ -125,6 +125,16 @@ where ); } Err(e) => { + if matches!( + e, + FrameStreamError::PayloadTooLarge { + frame_type: 0x1, + .. + } + ) { + self.frame_stream.stop_sending(Code::H3_REQUEST_CANCELLED); + self.frame_stream.reset(Code::H3_REQUEST_CANCELLED.value()); + } return Err(self.handle_frame_stream_error_on_request_stream(e)); } };