fix(proto): follow RFC for table size updates and frame length errors

src/codec/framed_read.rs

@@ -113,6 +113,22 @@ fn calc_max_continuation_frames(header_max: usize, frame_max: usize) -> usize {
     min_frames_for_list.saturating_add(padding).max(5)
 }
 
+/// Returns the connection error code for a frame that failed to load.
+///
+/// A payload whose length its frame type forbids is a `FRAME_SIZE_ERROR`, see
+/// <https://www.rfc-editor.org/rfc/rfc9113.html#section-4.2>. An oversized
+/// `INITIAL_WINDOW_SIZE` is a `FLOW_CONTROL_ERROR`, see
+/// <https://www.rfc-editor.org/rfc/rfc9113.html#section-6.5.2>.
+fn load_error_reason(error: &frame::Error) -> Reason {
+    match error {
+        frame::Error::BadFrameSize
+        | frame::Error::InvalidPayloadLength
+        | frame::Error::InvalidPayloadAckSettings => Reason::FRAME_SIZE_ERROR,
+        frame::Error::InvalidInitialWindowSize => Reason::FLOW_CONTROL_ERROR,
+        _ => Reason::PROTOCOL_ERROR,
+    }
+}
+
 impl FrameDecoder {
     fn new(max_frame_size: usize) -> Self {
         let max_header_list_size = DEFAULT_SETTINGS_MAX_HEADER_LIST_SIZE;
@@ -174,9 +190,9 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
             // Parse the header frame w/o parsing the payload
             let (mut frame, mut payload) = match frame::$frame::load($head, $bytes) {
                 Ok(res) => res,
-                Err(_e) => {
-                    proto_err!(conn: "failed to load frame; err={:?}", _e);
-                    return Err(Error::library_go_away(Reason::PROTOCOL_ERROR));
+                Err(e) => {
+                    proto_err!(conn: "failed to load frame; err={:?}", e);
+                    return Err(Error::library_go_away(load_error_reason(&e)));
                 }
             };
 
@@ -251,27 +267,27 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
         Kind::Settings => {
             let res = frame::Settings::load(head, &bytes[frame::HEADER_LEN..]);
 
-            res.map_err(|_e| {
-                proto_err!(conn: "failed to load SETTINGS frame; err={:?}", _e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+            res.map_err(|e| {
+                proto_err!(conn: "failed to load SETTINGS frame; err={:?}", e);
+                Error::library_go_away(load_error_reason(&e))
             })?
             .into()
         }
         Kind::Ping => {
             let res = frame::Ping::load(head, &bytes[frame::HEADER_LEN..]);
 
-            res.map_err(|_e| {
-                proto_err!(conn: "failed to load PING frame; err={:?}", _e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+            res.map_err(|e| {
+                proto_err!(conn: "failed to load PING frame; err={:?}", e);
+                Error::library_go_away(load_error_reason(&e))
             })?
             .into()
         }
         Kind::WindowUpdate => {
             let res = frame::WindowUpdate::load(head, &bytes[frame::HEADER_LEN..]);
 
-            res.map_err(|_e| {
-                proto_err!(conn: "failed to load WINDOW_UPDATE frame; err={:?}", _e);
-                Error::library_go_away(Reason::PROTOCOL_ERROR)
+            res.map_err(|e| {
+                proto_err!(conn: "failed to load WINDOW_UPDATE frame; err={:?}", e);
+                Error::library_go_away(load_error_reason(&e))
             })?
             .into()
         }
@@ -280,26 +296,26 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
             let res = frame::Data::load(head, bytes.freeze());
 
             // 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)
+            res.map_err(|e| {
+                proto_err!(conn: "failed to load DATA frame; err={:?}", e);
+                Error::library_go_away(load_error_reason(&e))
             })?
             .into()
         }
         Kind::Headers => header_block!(Headers, head, bytes),
         Kind::Reset => {
             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)
+            res.map_err(|e| {
+                proto_err!(conn: "failed to load RESET frame; err={:?}", e);
+                Error::library_go_away(load_error_reason(&e))
             })?
             .into()
         }
         Kind::GoAway => {
             let res = frame::GoAway::load(head, &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)
+            res.map_err(|e| {
+                proto_err!(conn: "failed to load GO_AWAY frame; err={:?}", e);
+                Error::library_go_away(load_error_reason(&e))
             })?
             .into()
         }
@@ -321,6 +337,14 @@ fn decode_frame(decoder: &mut FrameDecoder, mut bytes: BytesMut) -> Result<Optio
                     proto_err!(stream: "PRIORITY invalid dependency ID; stream={:?}", id);
                     return Err(Error::library_reset(id, Reason::PROTOCOL_ERROR));
                 }
+                Err(frame::Error::InvalidPayloadLength) => {
+                    // A PRIORITY frame with a length other than 5 octets is a
+                    // stream error of type `FRAME_SIZE_ERROR`, see
+                    // https://www.rfc-editor.org/rfc/rfc9113.html#section-6.3
+                    let id = head.stream_id();
+                    proto_err!(stream: "PRIORITY invalid payload length; stream={:?}", id);
+                    return Err(Error::library_reset(id, Reason::FRAME_SIZE_ERROR));
+                }
                 Err(_e) => {
                     proto_err!(conn: "failed to load PRIORITY frame; err={:?};", _e);
                     return Err(Error::library_go_away(Reason::PROTOCOL_ERROR));

src/frame/headers.rs

@@ -290,7 +290,7 @@ impl Headers {
         // Read the padding length
         if flags.is_padded() {
             if src.is_empty() {
-                return Err(Error::MalformedMessage);
+                return Err(Error::InvalidPayloadLength);
             }
             pad = src[0] as usize;
 
@@ -301,7 +301,7 @@ impl Headers {
         // Read the stream dependency
         let stream_dep = if flags.is_priority() {
             if src.len() < 5 {
-                return Err(Error::MalformedMessage);
+                return Err(Error::InvalidPayloadLength);
             }
             let stream_dep = StreamDependency::load(&src[..5])?;
 
@@ -614,7 +614,7 @@ impl PushPromise {
         // Read the padding length
         if flags.is_padded() {
             if src.is_empty() {
-                return Err(Error::MalformedMessage);
+                return Err(Error::InvalidPayloadLength);
             }
 
             // TODO: Ensure payload is sized correctly
@@ -624,8 +624,9 @@ impl PushPromise {
             src.advance(1);
         }
 
-        if src.len() < 5 {
-            return Err(Error::MalformedMessage);
+        // The Promised Stream ID is the only mandatory field.
+        if src.len() < 4 {
+            return Err(Error::InvalidPayloadLength);
         }
 
         let (promised_id, _) = StreamId::parse(&src[..4]);

src/frame/mod.rs

@@ -139,7 +139,7 @@ impl<T> fmt::Debug for Frame<T> {
 /// Errors that can occur during parsing an HTTP/2 frame.
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum Error {
-    /// A length value other than 8 was set on a PING message.
+    /// A PING, WINDOW_UPDATE or GOAWAY payload has a length its type forbids.
     BadFrameSize,
 
     /// The padding length was larger than the frame-header-specified
@@ -149,14 +149,17 @@ pub enum Error {
     /// An invalid setting value was provided
     InvalidSettingValue,
 
+    /// `SETTINGS_INITIAL_WINDOW_SIZE` exceeds the maximum window of 2^31-1.
+    InvalidInitialWindowSize,
+
     /// An invalid window update value
     InvalidWindowUpdateValue,
 
     /// The payload length specified by the frame header was not the
     /// value necessary for the specific frame type.
     InvalidPayloadLength,
 
-    /// Received a payload with an ACK settings frame
+    /// Received a payload with an ACK settings frame.
     InvalidPayloadAckSettings,
 
     /// An invalid stream identifier was provided.

src/frame/settings.rs

@@ -225,6 +225,11 @@ pub struct Settings {
     flags: SettingsFlags,
     // Fields
     header_table_size: Option<u32>,
+    /// Lowest `HEADER_TABLE_SIZE` before a higher final value in the same frame.
+    ///
+    /// The encoder must signal it ahead of the final value, see
+    /// <https://www.rfc-editor.org/rfc/rfc7541.html#section-4.2>.
+    lowest_header_table_size: Option<u32>,
     enable_push: Option<u32>,
     max_concurrent_streams: Option<u32>,
     initial_window_size: Option<u32>,
@@ -340,6 +345,11 @@ impl Settings {
 
     pub fn set_header_table_size(&mut self, size: Option<u32>) {
         self.header_table_size = size;
+        self.lowest_header_table_size = None;
+    }
+
+    pub(crate) fn lowest_header_table_size(&self) -> Option<u32> {
+        self.lowest_header_table_size
     }
 
     pub fn set_no_rfc7540_priorities(&mut self, enable: bool) {
@@ -368,7 +378,7 @@ impl Settings {
         if flag.is_ack() {
             // Ensure that the payload is empty
             if !payload.is_empty() {
-                return Err(Error::InvalidPayloadLength);
+                return Err(Error::InvalidPayloadAckSettings);
             }
 
             // Return the ACK frame
@@ -378,16 +388,22 @@ impl Settings {
         // Ensure the payload length is correct, each setting is 6 bytes long.
         if payload.len() % 6 != 0 {
             tracing::debug!("invalid settings payload length; len={:?}", payload.len());
-            return Err(Error::InvalidPayloadAckSettings);
+            return Err(Error::InvalidPayloadLength);
         }
 
         let mut settings = Settings::default();
         debug_assert!(!settings.flags.is_ack());
 
+        let mut lowest_header_table_size = None;
+
         for raw in payload.chunks(6) {
             if let Some(setting) = Setting::load(raw) {
                 match setting.id {
                     SettingId::HeaderTableSize => {
+                        lowest_header_table_size = Some(
+                            lowest_header_table_size
+                                .map_or(setting.value, |lowest: u32| lowest.min(setting.value)),
+                        );
                         settings.header_table_size = Some(setting.value);
                     }
                     SettingId::EnablePush => match setting.value {
@@ -403,7 +419,7 @@ impl Settings {
                     }
                     SettingId::InitialWindowSize => {
                         if setting.value as usize > MAX_INITIAL_WINDOW_SIZE {
-                            return Err(Error::InvalidSettingValue);
+                            return Err(Error::InvalidInitialWindowSize);
                         } else {
                             settings.initial_window_size = Some(setting.value);
                         }
@@ -443,6 +459,12 @@ impl Settings {
             }
         }
 
+        // Values take effect in order, so a lower value before the final one is
+        // a real table size change, see
+        // <https://www.rfc-editor.org/rfc/rfc9113.html#section-6.5.3>.
+        settings.lowest_header_table_size = lowest_header_table_size
+            .filter(|&lowest| settings.header_table_size.is_some_and(|last| lowest < last));
+
         Ok(settings)
     }
 
@@ -472,7 +494,8 @@ impl Settings {
         for id in &self.settings_order {
             match id {
                 SettingId::HeaderTableSize => {
-                    if let Some(v) = self.header_table_size {
+                    let lowest = self.lowest_header_table_size.into_iter();
+                    for v in lowest.chain(self.header_table_size) {
                         if let Some(setting) = Setting::from_id(*id, v) {
                             f(setting);
                         }
@@ -736,4 +759,38 @@ mod tests {
             .build();
         assert_eq!(unknown.settings.len(), 1);
     }
+
+    #[test]
+    fn test_repeated_header_table_size() {
+        fn load(values: &[u32]) -> Settings {
+            let payload: Vec<u8> = values
+                .iter()
+                .flat_map(|&v| {
+                    let mut raw = vec![0, 1];
+                    raw.extend_from_slice(&v.to_be_bytes());
+                    raw
+                })
+                .collect();
+            let head = Head::new(Kind::Settings, 0, StreamId::zero());
+            Settings::load(head, &payload).unwrap()
+        }
+
+        // A lower value before the final one is kept and re-encoded first.
+        let settings = load(&[100, 8192, 4096]);
+        assert_eq!(settings.lowest_header_table_size(), Some(100));
+        assert_eq!(settings.header_table_size(), Some(4096));
+        let mut dst = BytesMut::new();
+        settings.encode(&mut dst);
+        assert_eq!(&dst[9..], [0, 1, 0, 0, 0, 100, 0, 1, 0, 0, 0x10, 0]);
+
+        // Replacing the size drops the loaded transition.
+        let mut settings = settings;
+        settings.set_header_table_size(Some(8192));
+        assert_eq!(settings.lowest_header_table_size(), None);
+
+        // Only the final value matters when it is also the lowest.
+        let settings = load(&[4096, 0]);
+        assert_eq!(settings.lowest_header_table_size(), None);
+        assert_eq!(settings.header_table_size(), Some(0));
+    }
 }

src/frame/util.rs

@@ -19,9 +19,8 @@ use bytes::{Buf, Bytes};
 pub fn strip_padding(payload: &mut Bytes) -> Result<u8, Error> {
     let payload_len = payload.len();
     if payload_len == 0 {
-        // If this is the case, the frame is invalid as no padding length can be
-        // extracted, even though the frame should be padded.
-        return Err(Error::TooMuchPadding);
+        // A padded frame too short to carry the Pad Length field.
+        return Err(Error::InvalidPayloadLength);
     }
 
     let pad_len = payload[0] as usize;

src/proto/connection.rs

@@ -742,6 +742,9 @@ where
     }
 }
 
+// SETTINGS is rare and moved straight to `recv_settings`; boxing it would
+// allocate for every received frame of that type.
+#[allow(clippy::large_enum_variant)]
 enum ReceivedFrame {
     Settings(frame::Settings),
     Continue,

src/proto/settings.rs

@@ -138,6 +138,11 @@ impl Settings {
             streams.apply_remote_settings(&settings, is_initial)?;
 
             if let Some(val) = settings.header_table_size() {
+                // A frame that lowers and then raises the size needs both
+                // updates in the next field block, lowest first.
+                if let Some(lowest) = settings.lowest_header_table_size() {
+                    dst.set_send_header_table_size(lowest as usize);
+                }
                 dst.set_send_header_table_size(val as usize);
             }