fix: Use correct error codes for frame errors

src/codec/framed_read.rs

@@ -160,7 +160,7 @@ fn decode_frame(
                 },
                 Err(e) => {
                     proto_err!(conn: "failed to load frame; err={:?}", e);
-                    return Err(Error::library_go_away(Reason::PROTOCOL_ERROR));
+                    return Err(Error::library_go_away(e.reason()));
                 }
             };
 
@@ -177,7 +177,7 @@ fn decode_frame(
                 },
                 Err(e) => {
                     proto_err!(conn: "failed HPACK decoding; err={:?}", e);
-                    return Err(Error::library_go_away(Reason::PROTOCOL_ERROR));
+                    return Err(Error::library_go_away(e.reason()));
                 }
             }
 
@@ -203,7 +203,7 @@ fn decode_frame(
 
             res.map_err(|e| {
                 proto_err!(conn: "failed to load SETTINGS frame; err={:?}", e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+                Error::library_go_away(e.reason())
             })?
             .into()
         }
@@ -212,7 +212,7 @@ fn decode_frame(
 
             res.map_err(|e| {
                 proto_err!(conn: "failed to load PING frame; err={:?}", e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+                Error::library_go_away(e.reason())
             })?
             .into()
         }
@@ -221,7 +221,7 @@ fn decode_frame(
 
             res.map_err(|e| {
                 proto_err!(conn: "failed to load WINDOW_UPDATE frame; err={:?}", e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+                Error::library_go_away(e.reason())
             })?
             .into()
         }
@@ -232,7 +232,7 @@ fn decode_frame(
             // TODO: Should this always be connection level? Probably not...
             res.map_err(|e| {
                 proto_err!(conn: "failed to load DATA frame; err={:?}", e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+                Error::library_go_away(e.reason())
             })?
             .into()
         }
@@ -241,15 +241,15 @@ fn decode_frame(
             let res = frame::Reset::load(head, &bytes[frame::HEADER_LEN..]);
             res.map_err(|e| {
                 proto_err!(conn: "failed to load RESET frame; err={:?}", e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+                Error::library_go_away(e.reason())
             })?
             .into()
         }
         Kind::GoAway => {
             let res = frame::GoAway::load(&bytes[frame::HEADER_LEN..]);
             res.map_err(|e| {
                 proto_err!(conn: "failed to load GO_AWAY frame; err={:?}", e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+                Error::library_go_away(e.reason())
             })?
             .into()
         }
@@ -273,7 +273,7 @@ fn decode_frame(
                 }
                 Err(e) => {
                     proto_err!(conn: "failed to load PRIORITY frame; err={:?};", e);
-                    return Err(Error::library_go_away(Reason::PROTOCOL_ERROR));
+                    return Err(Error::library_go_away(e.reason()));
                 }
             }
         }
@@ -349,7 +349,7 @@ fn decode_frame(
                 }
                 Err(e) => {
                     proto_err!(conn: "failed HPACK decoding; err={:?}", e);
-                    return Err(Error::library_go_away(Reason::PROTOCOL_ERROR));
+                    return Err(Error::library_go_away(e.reason()));
                 }
             }
 

src/frame/mod.rs

@@ -141,6 +141,9 @@ pub enum Error {
     /// An invalid setting value was provided
     InvalidSettingValue,
 
+    /// The SETTINGS_INITIAL_WINDOW_SIZE value exceeds the maximum (2^31-1).
+    InvalidInitialWindowSize,
+
     /// An invalid window update value
     InvalidWindowUpdateValue,
 
@@ -169,3 +172,22 @@ pub enum Error {
     /// Failed to perform HPACK decoding
     Hpack(hpack::DecoderError),
 }
+
+impl Error {
+    /// Returns the appropriate HTTP/2 reason code for this frame error.
+    pub(crate) fn reason(&self) -> Reason {
+        match self {
+            Error::BadFrameSize
+            | Error::InvalidPayloadLength
+            | Error::InvalidPayloadAckSettings => Reason::FRAME_SIZE_ERROR,
+            Error::InvalidInitialWindowSize => Reason::FLOW_CONTROL_ERROR,
+            Error::Hpack(_) => Reason::COMPRESSION_ERROR,
+            Error::TooMuchPadding
+            | Error::InvalidSettingValue
+            | Error::InvalidWindowUpdateValue
+            | Error::InvalidStreamId
+            | Error::MalformedMessage
+            | Error::InvalidDependencyId => Reason::PROTOCOL_ERROR,
+        }
+    }
+}

src/frame/settings.rs

@@ -174,7 +174,7 @@ impl Settings {
                 }
                 Some(InitialWindowSize(val)) => {
                     if val as usize > MAX_INITIAL_WINDOW_SIZE {
-                        return Err(Error::InvalidSettingValue);
+                        return Err(Error::InvalidInitialWindowSize);
                     } else {
                         settings.initial_window_size = Some(val);
                     }

tests/h2-support/src/frames.rs

@@ -290,6 +290,10 @@ impl Mock<frame::GoAway> {
         self.reason(frame::Reason::FRAME_SIZE_ERROR)
     }
 
+    pub fn compression(self) -> Self {
+        self.reason(frame::Reason::COMPRESSION_ERROR)
+    }
+
     pub fn calm(self) -> Self {
         self.reason(frame::Reason::ENHANCE_YOUR_CALM)
     }

tests/h2-tests/tests/server.rs

@@ -1700,3 +1700,120 @@ async fn init_window_size_smaller_than_default_should_use_default_before_ack() {
 
     join(client, h2).await;
 }
+
+/// Malformed frames with wrong payload lengths must produce
+/// GOAWAY(FRAME_SIZE_ERROR).
+#[tokio::test]
+async fn frame_size_errors_use_correct_error_code() {
+    h2_support::trace_init!();
+
+    // Each entry: raw frame bytes with an invalid payload length.
+    let cases: &[(&str, &[u8])] = &[
+        // PING with 4 bytes instead of 8
+        ("PING", &[0, 0, 4, 6, 0, 0, 0, 0, 0, 1, 2, 3, 4]),
+        // WINDOW_UPDATE with 2 bytes instead of 4
+        ("WINDOW_UPDATE", &[0, 0, 2, 8, 0, 0, 0, 0, 0, 0, 1]),
+        // RST_STREAM with 2 bytes instead of 4 on stream 1
+        ("RST_STREAM", &[0, 0, 2, 3, 0, 0, 0, 0, 1, 0, 1]),
+        // SETTINGS with 5 bytes, not a multiple of 6
+        ("SETTINGS", &[0, 0, 5, 4, 0, 0, 0, 0, 0, 1, 2, 3, 4, 5]),
+        // SETTINGS ACK with 1 byte payload instead of 0
+        ("SETTINGS_ACK", &[0, 0, 1, 4, 1, 0, 0, 0, 0, 0]),
+        // PRIORITY with 3 bytes instead of 5 on stream 1
+        ("PRIORITY", &[0, 0, 3, 2, 0, 0, 0, 0, 1, 0, 0, 0]),
+        // GOAWAY with 4 bytes instead of ≥8
+        ("GOAWAY", &[0, 0, 4, 7, 0, 0, 0, 0, 0, 0, 0, 0, 0]),
+    ];
+
+    for (name, bad_frame) in cases {
+        let (io, mut client) = mock::new();
+
+        let client = async move {
+            let settings = client.assert_server_handshake().await;
+            assert_default_settings!(settings);
+
+            // Open stream 1 so RST_STREAM/PRIORITY have a valid target.
+            client
+                .send_frame(
+                    frames::headers(1)
+                        .request("GET", "https://example.com/")
+                        .eos(),
+                )
+                .await;
+            client
+                .recv_frame(frames::headers(1).response(200).eos())
+                .await;
+            client.send_bytes(bad_frame).await;
+            client.recv_frame(frames::go_away(1).frame_size()).await;
+        };
+
+        let srv = async move {
+            let mut srv = server::handshake(io).await.expect("handshake");
+            let (_req, mut respond) = srv.next().await.unwrap().unwrap();
+            let rsp = http::Response::builder().status(200).body(()).unwrap();
+            respond.send_response(rsp, true).unwrap();
+            let err = srv.next().await.unwrap().expect_err(name);
+            assert!(err.is_go_away());
+            assert!(err.is_library());
+            assert_eq!(err.reason(), Some(Reason::FRAME_SIZE_ERROR));
+        };
+
+        join(client, srv).await;
+    }
+}
+
+/// A SETTINGS frame with INITIAL_WINDOW_SIZE exceeding 2^31-1 must
+/// produce GOAWAY(FLOW_CONTROL_ERROR).
+#[tokio::test]
+async fn settings_initial_window_size_overflow_is_flow_control_error() {
+    h2_support::trace_init!();
+    let (io, mut client) = mock::new();
+
+    let client = async move {
+        let settings = client.assert_server_handshake().await;
+        assert_default_settings!(settings);
+
+        client
+            .send_frame(frames::settings().initial_window_size(0xFFFFFFFF))
+            .await;
+        client.recv_frame(frames::go_away(0).flow_control()).await;
+    };
+
+    let srv = async move {
+        let mut srv = server::handshake(io).await.expect("handshake");
+        let err = srv.next().await.unwrap().expect_err("server");
+        assert!(err.is_go_away());
+        assert!(err.is_library());
+        assert_eq!(err.reason(), Some(Reason::FLOW_CONTROL_ERROR));
+    };
+
+    join(client, srv).await;
+}
+
+/// A HEADERS frame with an invalid HPACK block must produce
+/// GOAWAY(COMPRESSION_ERROR).
+#[tokio::test]
+async fn hpack_error_is_compression_error() {
+    h2_support::trace_init!();
+    let (io, mut client) = mock::new();
+
+    let client = async move {
+        let settings = client.assert_server_handshake().await;
+        assert_default_settings!(settings);
+
+        // HEADERS(stream 1, END_HEADERS|END_STREAM) with 0xFE:
+        // HPACK indexed reference to index 126, which doesn't exist.
+        client.send_bytes(&[0, 0, 1, 1, 5, 0, 0, 0, 1, 0xFE]).await;
+        client.recv_frame(frames::go_away(0).compression()).await;
+    };
+
+    let srv = async move {
+        let mut srv = server::handshake(io).await.expect("handshake");
+        let err = srv.next().await.unwrap().expect_err("server");
+        assert!(err.is_go_away());
+        assert!(err.is_library());
+        assert_eq!(err.reason(), Some(Reason::COMPRESSION_ERROR));
+    };
+
+    join(client, srv).await;
+}