From 89c03d5bd0ef72b273db19e96916fee1c4c5ac18 Mon Sep 17 00:00:00 2001 From: Michael Rodler Date: Wed, 14 Jun 2023 23:46:07 +0200 Subject: [PATCH 1/6] Revert "fix panic on receiving invalid headers frame by making the `take_request` function return a Result" This reverts commit 66c36c4edb04d8f75ca66b9199546308fe089c0d. --- src/proto/streams/recv.rs | 11 +++++------ src/proto/streams/streams.rs | 2 +- src/server.rs | 17 +++++------------ tests/h2-tests/tests/server.rs | 32 -------------------------------- 4 files changed, 11 insertions(+), 51 deletions(-) diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs index cd96dce2c..ec4db1c79 100644 --- a/src/proto/streams/recv.rs +++ b/src/proto/streams/recv.rs @@ -251,15 +251,14 @@ impl Recv { } /// Called by the server to get the request - pub fn take_request(&mut self, stream: &mut store::Ptr) -> Result, proto::Error> { + /// + /// TODO: Should this fn return `Result`? + pub fn take_request(&mut self, stream: &mut store::Ptr) -> Request<()> { use super::peer::PollMessage::*; match stream.pending_recv.pop_front(&mut self.buffer) { - Some(Event::Headers(Server(request))) => Ok(request), - _ => { - proto_err!(stream: "received invalid request; stream={:?}", stream.id); - Err(Error::library_reset(stream.id, Reason::PROTOCOL_ERROR)) - } + Some(Event::Headers(Server(request))) => request, + _ => panic!(), } } diff --git a/src/proto/streams/streams.rs b/src/proto/streams/streams.rs index d64e00970..dfc5c768b 100644 --- a/src/proto/streams/streams.rs +++ b/src/proto/streams/streams.rs @@ -1178,7 +1178,7 @@ impl StreamRef { /// # Panics /// /// This function panics if the request isn't present. - pub fn take_request(&self) -> Result, proto::Error> { + pub fn take_request(&self) -> Request<()> { let mut me = self.opaque.inner.lock().unwrap(); let me = &mut *me; diff --git a/src/server.rs b/src/server.rs index 148cad517..f1f4cf470 100644 --- a/src/server.rs +++ b/src/server.rs @@ -425,20 +425,13 @@ where if let Some(inner) = self.connection.next_incoming() { tracing::trace!("received incoming"); - match inner.take_request() { - Ok(req) => { - let (head, _) = req.into_parts(); - let body = RecvStream::new(FlowControl::new(inner.clone_to_opaque())); + let (head, _) = inner.take_request().into_parts(); + let body = RecvStream::new(FlowControl::new(inner.clone_to_opaque())); - let request = Request::from_parts(head, body); - let respond = SendResponse { inner }; + let request = Request::from_parts(head, body); + let respond = SendResponse { inner }; - return Poll::Ready(Some(Ok((request, respond)))); - } - Err(e) => { - return Poll::Ready(Some(Err(e.into()))); - } - } + return Poll::Ready(Some(Ok((request, respond)))); } Poll::Pending diff --git a/tests/h2-tests/tests/server.rs b/tests/h2-tests/tests/server.rs index 2637011ff..c8c1c9d1c 100644 --- a/tests/h2-tests/tests/server.rs +++ b/tests/h2-tests/tests/server.rs @@ -1378,35 +1378,3 @@ async fn reject_non_authority_target_on_connect_request() { join(client, srv).await; } - -#[tokio::test] -async fn reject_response_headers_in_request() { - h2_support::trace_init!(); - - let (io, mut client) = mock::new(); - - let client = async move { - let _ = client.assert_server_handshake().await; - - client.send_frame(frames::headers(1).response(128)).await; - - // TODO: is CANCEL the right error code to expect here? - client.recv_frame(frames::reset(1).cancel()).await; - }; - - let srv = async move { - let builder = server::Builder::new(); - let mut srv = builder.handshake::<_, Bytes>(io).await.expect("handshake"); - - let res = srv.next().await; - tracing::warn!("{:?}", res); - assert!(res.is_some()); - assert!(res.unwrap().is_err()); - - poll_fn(move |cx| srv.poll_closed(cx)) - .await - .expect("server"); - }; - - join(client, srv).await; -} From 88e99ce7aee949588807deac823a38d0cf82a213 Mon Sep 17 00:00:00 2001 From: Michael Rodler Date: Wed, 14 Jun 2023 23:50:09 +0200 Subject: [PATCH 2/6] proper fix for the panic in server receiving a request with a :status pseudo-header in the informational range of status codes Signed-off-by: Michael Rodler Co-Authored-By: f0rki Reviewed-by: Daniele Ahmed --- src/proto/streams/recv.rs | 15 ++++++++++----- tests/h2-tests/tests/server.rs | 29 +++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs index ec4db1c79..6891d01b0 100644 --- a/src/proto/streams/recv.rs +++ b/src/proto/streams/recv.rs @@ -229,6 +229,11 @@ impl Recv { return Err(Error::library_reset(stream.id, Reason::PROTOCOL_ERROR).into()); } + if pseudo.status.is_some() && counts.peer().is_server() { + proto_err!(stream: "cannot use :status header for requests; stream={:?}", stream.id); + return Err(Error::library_reset(stream.id, Reason::PROTOCOL_ERROR).into()); + } + if !pseudo.is_informational() { let message = counts .peer() @@ -239,12 +244,12 @@ impl Recv { .pending_recv .push_back(&mut self.buffer, Event::Headers(message)); stream.notify_recv(); - } - // Only servers can receive a headers frame that initiates the stream. - // This is verified in `Streams` before calling this function. - if counts.peer().is_server() { - self.pending_accept.push(stream); + // Only servers can receive a headers frame that initiates the stream. + // This is verified in `Streams` before calling this function. + if counts.peer().is_server() { + self.pending_accept.push(stream); + } } Ok(()) diff --git a/tests/h2-tests/tests/server.rs b/tests/h2-tests/tests/server.rs index c8c1c9d1c..df1b0fe1a 100644 --- a/tests/h2-tests/tests/server.rs +++ b/tests/h2-tests/tests/server.rs @@ -1378,3 +1378,32 @@ async fn reject_non_authority_target_on_connect_request() { join(client, srv).await; } + +#[tokio::test] +async fn reject_informational_status_header_in_request() { + h2_support::trace_init!(); + + let (io, mut client) = mock::new(); + + let client = async move { + let _ = client.assert_server_handshake().await; + + let status_code = 128; + assert!(StatusCode::from_u16(status_code).unwrap().is_informational()); + + client.send_frame(frames::headers(1).response(status_code)).await; + + client.recv_frame(frames::reset(1).protocol_error()).await; + }; + + let srv = async move { + let builder = server::Builder::new(); + let mut srv = builder.handshake::<_, Bytes>(io).await.expect("handshake"); + + poll_fn(move |cx| srv.poll_closed(cx)) + .await + .expect("server"); + }; + + join(client, srv).await; +} From be8abe3e5dcd7cd2a19ecd90c0d3213e358e183d Mon Sep 17 00:00:00 2001 From: Michael Rodler Date: Fri, 16 Jun 2023 14:56:19 +0200 Subject: [PATCH 3/6] replace empty panic with unreachable! for more clarity Co-authored-by: 82marbag <69267416+82marbag@users.noreply.github.com> --- src/proto/streams/recv.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs index 6891d01b0..08994669d 100644 --- a/src/proto/streams/recv.rs +++ b/src/proto/streams/recv.rs @@ -263,7 +263,7 @@ impl Recv { match stream.pending_recv.pop_front(&mut self.buffer) { Some(Event::Headers(Server(request))) => request, - _ => panic!(), + _ => unreachable!("This panic should not be reached. Cut an issue at https://github.com/hyperium/h2"), } } From 9a78d6b68725c4635aa202982a275f4cdc16666b Mon Sep 17 00:00:00 2001 From: Michael Rodler Date: Fri, 16 Jun 2023 15:45:06 +0200 Subject: [PATCH 4/6] cargo fmt --- src/proto/streams/recv.rs | 4 +++- tests/h2-tests/tests/server.rs | 8 ++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs index 08994669d..aa28b4565 100644 --- a/src/proto/streams/recv.rs +++ b/src/proto/streams/recv.rs @@ -263,7 +263,9 @@ impl Recv { match stream.pending_recv.pop_front(&mut self.buffer) { Some(Event::Headers(Server(request))) => request, - _ => unreachable!("This panic should not be reached. Cut an issue at https://github.com/hyperium/h2"), + _ => unreachable!( + "This panic should not be reached. Cut an issue at https://github.com/hyperium/h2" + ), } } diff --git a/tests/h2-tests/tests/server.rs b/tests/h2-tests/tests/server.rs index df1b0fe1a..0d7bb61cc 100644 --- a/tests/h2-tests/tests/server.rs +++ b/tests/h2-tests/tests/server.rs @@ -1389,9 +1389,13 @@ async fn reject_informational_status_header_in_request() { let _ = client.assert_server_handshake().await; let status_code = 128; - assert!(StatusCode::from_u16(status_code).unwrap().is_informational()); + assert!(StatusCode::from_u16(status_code) + .unwrap() + .is_informational()); - client.send_frame(frames::headers(1).response(status_code)).await; + client + .send_frame(frames::headers(1).response(status_code)) + .await; client.recv_frame(frames::reset(1).protocol_error()).await; }; From 54f965a95c34dfc10a23cf54b190ab01916b71df Mon Sep 17 00:00:00 2001 From: Michael Rodler Date: Fri, 16 Jun 2023 16:00:40 +0200 Subject: [PATCH 5/6] added some additional comments for clarification. --- src/proto/streams/recv.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs index aa28b4565..9d41423de 100644 --- a/src/proto/streams/recv.rs +++ b/src/proto/streams/recv.rs @@ -248,6 +248,8 @@ impl Recv { // Only servers can receive a headers frame that initiates the stream. // This is verified in `Streams` before calling this function. if counts.peer().is_server() { + // Correctness: never push a stream to `pending_accept` without having the + // corresponding headers frame pushed to `stream.pending_recv`. self.pending_accept.push(stream); } } @@ -257,7 +259,10 @@ impl Recv { /// Called by the server to get the request /// - /// TODO: Should this fn return `Result`? + /// # Panics + /// + /// Panics if `stream.pending_recv` has no `Event::Headers` queued. + /// pub fn take_request(&mut self, stream: &mut store::Ptr) -> Request<()> { use super::peer::PollMessage::*; From 5cdc018947bdb02e6cddf85d613c2431900b90f0 Mon Sep 17 00:00:00 2001 From: Michael Rodler Date: Mon, 19 Jun 2023 15:56:14 +0200 Subject: [PATCH 6/6] updated message in unreachable! macro --- src/proto/streams/recv.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/proto/streams/recv.rs b/src/proto/streams/recv.rs index 9d41423de..98de1bfa7 100644 --- a/src/proto/streams/recv.rs +++ b/src/proto/streams/recv.rs @@ -268,9 +268,7 @@ impl Recv { match stream.pending_recv.pop_front(&mut self.buffer) { Some(Event::Headers(Server(request))) => request, - _ => unreachable!( - "This panic should not be reached. Cut an issue at https://github.com/hyperium/h2" - ), + _ => unreachable!("server stream queue must start with Headers"), } }