Fix for invalid header panic corrected (#695)

* Revert "fix panic on receiving invalid headers frame by making the `take_request` function return a Result"

This reverts commit 66c36c4.

* 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 <[email protected]>
Co-authored-by: Michael Rodler <[email protected]>
Co-authored-by: Daniele Ahmed <[email protected]>

Author: 3 people

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,27 +244,31 @@ 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() {
+                // Correctness: never push a stream to `pending_accept` without having the
+                // corresponding headers frame pushed to `stream.pending_recv`.
+                self.pending_accept.push(stream);
+            }
         }
 
         Ok(())
     }
 
     /// Called by the server to get the request
-    pub fn take_request(&mut self, stream: &mut store::Ptr) -> Result<Request<()>, proto::Error> {
+    ///
+    /// # 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::*;
 
         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,
+            _ => unreachable!("server stream queue must start with Headers"),
         }
     }
 

src/proto/streams/streams.rs

@@ -1178,7 +1178,7 @@ impl<B> StreamRef<B> {
     /// # Panics
     ///
     /// This function panics if the request isn't present.
-    pub fn take_request(&self) -> Result<Request<()>, proto::Error> {
+    pub fn take_request(&self) -> Request<()> {
         let mut me = self.opaque.inner.lock().unwrap();
         let me = &mut *me;
 

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

tests/h2-tests/tests/server.rs

@@ -1380,29 +1380,30 @@ async fn reject_non_authority_target_on_connect_request() {
 }
 
 #[tokio::test]
-async fn reject_response_headers_in_request() {
+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;
 
-        client.send_frame(frames::headers(1).response(128)).await;
+        let status_code = 128;
+        assert!(StatusCode::from_u16(status_code)
+            .unwrap()
+            .is_informational());
 
-        // TODO: is CANCEL the right error code to expect here?
-        client.recv_frame(frames::reset(1).cancel()).await;
+        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");
 
-        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");