Protocol violation in RequestStream

[Ruben2424]

Why the tests did not catch it

In h3/src/tests/request.rs is the reusable method request_sequence_check

h3/h3/src/tests/request.rs

Lines 1421 to 1472 in dbf2523

	async fn request_sequence_check<F, FC>(request: F, check: FC)
	where
	F: Fn(&mut BytesMut),
	FC: Fn(Result<(), Error>),
	{
	init_tracing();
	let mut pair = Pair::default();
	let mut server = pair.server();
	let client_fut = async {
	let connection = pair.client_inner().await;
	let (mut req_send, mut req_recv) = connection.open_bi().await.unwrap();
	let mut buf = BytesMut::new();
	request(&mut buf);
	req_send.write_all(&buf[..]).await.unwrap();
	req_send.finish().unwrap();
	let res = req_recv
	.read(&mut buf)
	.await
	.map_err(Into::<h3_quinn::ReadError>::into)
	.map_err(Into::<Error>::into)
	.map(|_| ());
	check(res);
	let (mut driver, _send) = client::new(h3_quinn::Connection::new(connection))
	.await
	.unwrap();
	let res = future::poll_fn(|cx| driver.poll_close(cx))
	.await
	.map_err(Into::<Error>::into)
	.map(|_| ());
	check(res);
	};
	let server_fut = async {
	let conn = server.next().await;
	let mut incoming = server::Connection::new(conn).await.unwrap();
	let (_, mut stream) = incoming
	.accept()
	.await?
	.expect("request stream end unexpected");
	while stream.recv_data().await?.is_some() {}
	stream.recv_trailers().await?;
	Result::<(), Error>::Ok(())
	};
	tokio::select! { res = server_fut => check(res)
	, _ = client_fut => panic!("client resolved first") };
	}

Important are the lines

h3/h3/src/tests/request.rs

Lines 1470 to 1471 in dbf2523

	tokio::select! { res = server_fut => check(res)
	, _ = client_fut => panic!("client resolved first") };

Because this is an select! instead of an join, the test often skips the check(res) calls in the client_fut.

h3/h3/src/tests/request.rs

Line 1445 in dbf2523

check(res);

If this happens, the tests, test only if the server recognizes the error, but not if the connection is closed because of the error.

Some violations within the Request stream MUST be a Connection error. The Test completely misses this.

For example https://www.rfc-editor.org/rfc/rfc9114.html#name-http-message-framing

Receipt of an invalid sequence of frames MUST be treated as a connection error of type H3_FRAME_UNEXPECTED. In particular, a DATA frame before any HEADERS frame, or a HEADERS or DATA frame after the trailing HEADERS frame, is considered invalid. Other frame types, especially unknown frame types, might be permitted subject to their own rules; see Section 9.

After fixing this race condition in the test, some tests fail. For example request_invalid_two_trailers.

The Problem

Looking at the code of poll_recv_trailers:

h3/h3/src/connection.rs

Lines 797 to 879 in dbf2523

	pub fn poll_recv_trailers(
	&mut self,
	cx: &mut Context<'_>,
	) -> Poll<Result<Option<HeaderMap>, Error>> {
	let mut trailers = if let Some(encoded) = self.trailers.take() {
	encoded
	} else {
	let frame = futures_util::ready!(self.stream.poll_next(cx))
	.map_err(|e| self.maybe_conn_err(e))?;
	match frame {
	Some(Frame::Headers(encoded)) => encoded,
	//= https://www.rfc-editor.org/rfc/rfc9114#section-4.1
	//# Receipt of an invalid sequence of frames MUST be treated as a
	//# connection error of type H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.3
	//# Receiving a
	//# CANCEL_PUSH frame on a stream other than the control stream MUST be
	//# treated as a connection error of type H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.4
	//# If an endpoint receives a SETTINGS frame on a different
	//# stream, the endpoint MUST respond with a connection error of type
	//# H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.6
	//# A client MUST treat a GOAWAY frame on a stream other than
	//# the control stream as a connection error of type H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.7
	//# The MAX_PUSH_ID frame is always sent on the control stream. Receipt
	//# of a MAX_PUSH_ID frame on any other stream MUST be treated as a
	//# connection error of type H3_FRAME_UNEXPECTED.
	Some(_) => return Poll::Ready(Err(Code::H3_FRAME_UNEXPECTED.into())),
	None => return Poll::Ready(Ok(None)),
	}
	};
	if !self.stream.is_eos() {
	// Get the trailing frame
	match self
	.stream
	.poll_next(cx)
	.map_err(|e| self.maybe_conn_err(e))?
	{
	Poll::Ready(trailing_frame) => {
	if trailing_frame.is_some() {
	// if it's not unknown or reserved, fail.
	return Poll::Ready(Err(Code::H3_FRAME_UNEXPECTED.into()));
	}
	}
	Poll::Pending => {
	// save the trailers and try again.
	self.trailers = Some(trailers);
	return Poll::Pending;
	}
	}
	}
	let qpack::Decoded { fields, .. } =
	match qpack::decode_stateless(&mut trailers, self.max_field_section_size) {
	//= https://www.rfc-editor.org/rfc/rfc9114#section-4.2.2
	//# An HTTP/3 implementation MAY impose a limit on the maximum size of
	//# the message header it will accept on an individual HTTP message.
	Err(qpack::DecoderError::HeaderTooLong(cancel_size)) => {
	return Poll::Ready(Err(Error::header_too_big(
	cancel_size,
	self.max_field_section_size,
	)))
	}
	Ok(decoded) => decoded,
	Err(e) => return Poll::Ready(Err(e.into())),
	};
	Poll::Ready(Ok(Some(Header::try_from(fields)?.into_fields())))
	}
	#[allow(missing_docs)]
	#[cfg_attr(feature = "tracing", instrument(skip_all, level = "trace"))]
	pub fn stop_sending(&mut self, err_code: Code) {
	self.stream.stop_sending(err_code);
	}
	}

Some Things regarding Protocol are validated, and errors are returned to the user of h3. But as far as I can See, neiter the stream is getting closed nor the Connection. The Error is just returned to the User.

I noticed the same for poll_recv_data

h3/h3/src/connection.rs

Lines 740 to 787 in dbf2523

	pub fn poll_recv_data(
	&mut self,
	cx: &mut Context<'_>,
	) -> Poll<Result<Option<impl Buf>, Error>> {
	if !self.stream.has_data() {
	let frame = self
	.stream
	.poll_next(cx)
	.map_err(|e| self.maybe_conn_err(e))?;
	match ready!(frame) {
	Some(Frame::Data { .. }) => (),
	Some(Frame::Headers(encoded)) => {
	self.trailers = Some(encoded);
	return Poll::Ready(Ok(None));
	}
	//= https://www.rfc-editor.org/rfc/rfc9114#section-4.1
	//# Receipt of an invalid sequence of frames MUST be treated as a
	//# connection error of type H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.3
	//# Receiving a
	//# CANCEL_PUSH frame on a stream other than the control stream MUST be
	//# treated as a connection error of type H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.4
	//# If an endpoint receives a SETTINGS frame on a different
	//# stream, the endpoint MUST respond with a connection error of type
	//# H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.6
	//# A client MUST treat a GOAWAY frame on a stream other than
	//# the control stream as a connection error of type H3_FRAME_UNEXPECTED.
	//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.7
	//# The MAX_PUSH_ID frame is always sent on the control stream. Receipt
	//# of a MAX_PUSH_ID frame on any other stream MUST be treated as a
	//# connection error of type H3_FRAME_UNEXPECTED.
	Some(_) => return Poll::Ready(Err(Code::H3_FRAME_UNEXPECTED.into())),
	None => return Poll::Ready(Ok(None)),
	}
	}
	self.stream
	.poll_data(cx)
	.map_err(|e| self.maybe_conn_err(e))
	}

[Ruben2424]

https://github.com/hyperium/h3/actions/runs/12550109930/job/34992334147?pr=271

[seanmonstar]

Nice catch!