Repository: bearcove/loona (language: rust)

Issue:
h2: Ignore unknown frame types
```
h2spec -p 8888 http2/4.1/1
Hypertext Transfer Protocol Version 2 (HTTP/2)
  4. HTTP Frames
    4.1. Frame Format
      × 1: Sends a frame with unknown type
        -> The endpoint MUST ignore and discard any frame that has a type that is unknown.
           Expected: PING Frame (length:8, flags:0x01, stream_id:0, opaque_data:)
             Actual: Connection closed

Failures: 

Hypertext Transfer Protocol Version 2 (HTTP/2)
  4. HTTP Frames
    4.1. Frame Format
      × 1: Sends a frame with unknown type
        -> The endpoint MUST ignore and discard any frame that has a type that is unknown.
           Expected: PING Frame (length:8, flags:0x01, stream_id:0, opaque_data:)
             Actual: Connection closed

Finished in 0.0015 seconds
1 tests, 0 passed, 0 skipped, 1 failed
```


Discussion / hints:

Fix patch:
diff --git a/src/h2/server.rs b/src/h2/server.rs
index adc76685..c3d1a833 100644
--- a/src/h2/server.rs
+++ b/src/h2/server.rs
@@ -5,7 +5,7 @@ use http::{header::HeaderName, Version};
 use nom::Finish;
 use pretty_hex::PrettyHex;
 use tokio::sync::mpsc;
-use tracing::debug;
+use tracing::{debug, trace};
 
 use crate::{
     h2::{
@@ -124,32 +124,33 @@ async fn h2_read_loop(
 
         debug!(?frame, "received h2 frame");
 
+        // TODO: there might be optimizations to be done for `Data` frames later
+        // on, but for now, let's unconditionally read the payload
+        let payload;
+        (client_buf, payload) = match read_and_parse(
+            nom::bytes::streaming::take(frame.len as usize),
+            transport.as_ref(),
+            client_buf,
+            frame.len as usize,
+        )
+        .await?
+        {
+            Some((client_buf, frame)) => (client_buf, frame),
+            None => {
+                debug!(
+                    "h2 client closed connection while reading payload for {:?}",
+                    frame.frame_type
+                );
+                return Ok(());
+            }
+        };
+
         match frame.frame_type {
             FrameType::Data(flags) => {
                 if flags.contains(DataFlags::Padded) {
                     todo!("handle padded data frame");
                 }
 
-                // TODO: not sure if worth, but: `read_and_parse` might have to
-                // issue several `read` operations, and we could send those as
-                // soon as we have them through `ev_tx`. We also have to pass
-                // flags, so this complicates things.
-                let payload;
-                (client_buf, payload) = match read_and_parse(
-                    nom::bytes::streaming::take(frame.len as usize),
-                    transport.as_ref(),
-                    client_buf,
-                    frame.len as usize,
-                )
-                .await?
-                {
-                    Some((client_buf, frame)) => (client_buf, frame),
-                    None => {
-                        debug!("h2 client closed connection while reading data body");
-                        return Ok(());
-                    }
-                };
-
                 if (ev_tx
                     .send(H2ConnEvent::ClientFrame(frame, payload.into()))
                     .await)
@@ -170,22 +171,6 @@ async fn h2_read_loop(
                     }
                 }
 
-                let payload;
-                (client_buf, payload) = match read_and_parse(
-                    nom::bytes::streaming::take(frame.len as usize),
-                    transport.as_ref(),
-                    client_buf,
-                    frame.len as usize,
-                )
-                .await?
-                {
-                    Some((client_buf, frame)) => (client_buf, frame),
-                    None => {
-                        debug!("h2 client closed connection while reading headers body");
-                        return Ok(());
-                    }
-                };
-
                 if (ev_tx
                     .send(H2ConnEvent::ClientFrame(frame, payload.into()))
                     .await)
@@ -197,22 +182,6 @@ async fn h2_read_loop(
             FrameType::Priority => todo!(),
             FrameType::RstStream => todo!(),
             FrameType::Settings(s) => {
-                let _payload;
-                (client_buf, _payload) = match read_and_parse(
-                    nom::bytes::streaming::take(frame.len as usize),
-                    transport.as_ref(),
-                    client_buf,
-                    frame.len as usize,
-                )
-                .await?
-                {
-                    Some((client_buf, frame)) => (client_buf, frame),
-                    None => {
-                        debug!("h2 client closed connection while reading settings body");
-                        return Ok(());
-                    }
-                };
-
                 if s.contains(SettingsFlags::Ack) {
                     debug!("Peer has acknowledged our settings, cool");
                 } else {
@@ -229,25 +198,13 @@ async fn h2_read_loop(
                     todo!("handle connection error: ping frame with non-zero stream id");
                 }
 
-                // ping opaque data is 64 bits, which is 8 bytes,
-                // cf. https://httpwg.org/specs/rfc9113.html#PING
-                let payload;
-                (client_buf, payload) = match read_and_parse(
-                    nom::bytes::streaming::take(8_usize),
-                    transport.as_ref(),
-                    client_buf,
-                    frame.len as usize,
-                )
-                .await?
-                {
-                    Some((client_buf, frame)) => (client_buf, frame),
-                    None => {
-                        debug!("h2 client closed connection while reading settings body");
-                        return Ok(());
-                    }
-                };
+                if frame.len != 8 {
+                    todo!("handle connection error: ping frame with invalid length");
+                }
 
                 if flags.contains(PingFlags::Ack) {
+                    // TODO: check that payload matches the one we sent?
+
                     debug!("received ping ack");
                     continue;
                 }
@@ -259,24 +216,15 @@ async fn h2_read_loop(
             FrameType::GoAway => todo!(),
             FrameType::WindowUpdate => {
                 debug!("ignoring window update");
-
-                let _payload;
-                (client_buf, _payload) = match read_and_parse(
-                    nom::bytes::streaming::take(frame.len as usize),
-                    transport.as_ref(),
-                    client_buf,
-                    frame.len as usize,
-                )
-                .await?
-                {
-                    Some((client_buf, frame)) => (client_buf, frame),
-                    None => {
-                        debug!("h2 client closed connection while reading window update body");
-                        return Ok(());
-                    }
-                };
             }
             FrameType::Continuation => todo!(),
+            FrameType::Unknown(ft) => {
+                trace!(
+                    "ignoring unknown frame with type 0x{:x}, flags 0x{:x}",
+                    ft.ty,
+                    ft.flags
+                );
+            }
         }
     }
 }
@@ -321,7 +269,7 @@ async fn h2_write_loop(
                             .finish()
                             .map_err(|err| eyre::eyre!("parsing error: {err:?}"))?;
                         payload = Piece::Roll(roll_out);
-                        debug!("got priority: {:?}", prio);
+                        debug!(exclusive = %prio.exclusive, stream_dependency = ?prio.stream_dependency, weight = %prio.weight, "received priority, exclusive");
                     }
 
                     let mut path: Option<PieceStr> = None;
diff --git a/src/h2/server/parse.rs b/src/h2/server/parse.rs
index 98417981..46d9b0e8 100644
--- a/src/h2/server/parse.rs
+++ b/src/h2/server/parse.rs
@@ -8,7 +8,7 @@ use std::fmt;
 use enum_repr::EnumRepr;
 use enumflags2::{bitflags, BitFlags};
 use nom::{
-    combinator::{map, map_res},
+    combinator::map,
     number::streaming::{be_u24, be_u8},
     sequence::tuple,
     IResult,
@@ -26,7 +26,7 @@ pub fn preface(i: Roll) -> IResult<Roll, ()> {
 
 /// See https://httpwg.org/specs/rfc9113.html#FrameTypes
 #[EnumRepr(type = "u8")]
-#[derive(Debug)]
+#[derive(Debug, Clone, Copy)]
 pub enum RawFrameType {
     Data = 0,
     Headers = 1,
@@ -53,6 +53,7 @@ pub enum FrameType {
     GoAway,
     WindowUpdate,
     Continuation,
+    Unknown(EncodedFrameType),
 }
 
 /// See https://httpwg.org/specs/rfc9113.html#DATA
@@ -91,38 +92,68 @@ pub enum PingFlags {
     Ack = 0x01,
 }
 
+#[derive(Debug, Clone, Copy)]
+pub struct EncodedFrameType {
+    pub ty: u8,
+    pub flags: u8,
+}
+
+impl EncodedFrameType {
+    fn parse(i: Roll) -> IResult<Roll, Self> {
+        let (i, (ty, flags)) = tuple((be_u8, be_u8))(i)?;
+        Ok((i, Self { ty, flags }))
+    }
+}
+
+impl From<(RawFrameType, u8)> for EncodedFrameType {
+    fn from((ty, flags): (RawFrameType, u8)) -> Self {
+        Self {
+            ty: ty.repr(),
+            flags,
+        }
+    }
+}
+
 impl FrameType {
-    pub(crate) fn encode(&self) -> (RawFrameType, u8) {
+    pub(crate) fn encode(self) -> EncodedFrameType {
         match self {
-            FrameType::Data(f) => (RawFrameType::Data, f.bits()),
-            FrameType::Headers(f) => (RawFrameType::Headers, f.bits()),
-            FrameType::Priority => (RawFrameType::Priority, 0),
-            FrameType::RstStream => (RawFrameType::RstStream, 0),
-            FrameType::Settings(f) => (RawFrameType::Settings, f.bits()),
-            FrameType::PushPromise => (RawFrameType::PushPromise, 0),
-            FrameType::Ping(f) => (RawFrameType::Ping, f.bits()),
-            FrameType::GoAway => (RawFrameType::GoAway, 0),
-            FrameType::WindowUpdate => (RawFrameType::WindowUpdate, 0),
-            FrameType::Continuation => (RawFrameType::Continuation, 0),
+            FrameType::Data(f) => (RawFrameType::Data, f.bits()).into(),
+            FrameType::Headers(f) => (RawFrameType::Headers, f.bits()).into(),
+            FrameType::Priority => (RawFrameType::Priority, 0).into(),
+            FrameType::RstStream => (RawFrameType::RstStream, 0).into(),
+            FrameType::Settings(f) => (RawFrameType::Settings, f.bits()).into(),
+            FrameType::PushPromise => (RawFrameType::PushPromise, 0).into(),
+            FrameType::Ping(f) => (RawFrameType::Ping, f.bits()).into(),
+            FrameType::GoAway => (RawFrameType::GoAway, 0).into(),
+            FrameType::WindowUpdate => (RawFrameType::WindowUpdate, 0).into(),
+            FrameType::Continuation => (RawFrameType::Continuation, 0).into(),
+            FrameType::Unknown(ft) => ft,
         }
     }
 
-    fn decode(ty: RawFrameType, flags: u8) -> Self {
-        match ty {
-            RawFrameType::Data => FrameType::Data(BitFlags::<DataFlags>::from_bits_truncate(flags)),
-            RawFrameType::Headers => {
-                FrameType::Headers(BitFlags::<HeadersFlags>::from_bits_truncate(flags))
-            }
-            RawFrameType::Priority => FrameType::Priority,
-            RawFrameType::RstStream => FrameType::RstStream,
-            RawFrameType::Settings => {
-                FrameType::Settings(BitFlags::<SettingsFlags>::from_bits_truncate(flags))
-            }
-            RawFrameType::PushPromise => FrameType::PushPromise,
-            RawFrameType::Ping => FrameType::Ping(BitFlags::<PingFlags>::from_bits_truncate(flags)),
-            RawFrameType::GoAway => FrameType::GoAway,
-            RawFrameType::WindowUpdate => FrameType::WindowUpdate,
-            RawFrameType::Continuation => FrameType::Continuation,
+    fn decode(ft: EncodedFrameType) -> Self {
+        match RawFrameType::from_repr(ft.ty) {
+            Some(ty) => match ty {
+                RawFrameType::Data => {
+                    FrameType::Data(BitFlags::<DataFlags>::from_bits_truncate(ft.flags))
+                }
+                RawFrameType::Headers => {
+                    FrameType::Headers(BitFlags::<HeadersFlags>::from_bits_truncate(ft.flags))
+                }
+                RawFrameType::Priority => FrameType::Priority,
+                RawFrameType::RstStream => FrameType::RstStream,
+                RawFrameType::Settings => {
+                    FrameType::Settings(BitFlags::<SettingsFlags>::from_bits_truncate(ft.flags))
+                }
+                RawFrameType::PushPromise => FrameType::PushPromise,
+                RawFrameType::Ping => {
+                    FrameType::Ping(BitFlags::<PingFlags>::from_bits_truncate(ft.flags))
+                }
+                RawFrameType::GoAway => FrameType::GoAway,
+                RawFrameType::WindowUpdate => FrameType::WindowUpdate,
+                RawFrameType::Continuation => FrameType::Continuation,
+            },
+            None => FrameType::Unknown(ft),
         }
     }
 }
@@ -193,19 +224,14 @@ impl Frame {
     /// slice, and copies it to the heap, which may not be ideal for a production
     /// implementation.
     pub fn parse(i: Roll) -> IResult<Roll, Self> {
-        let (i, (len, frame_type, flags, (reserved, stream_id))) = tuple((
+        let (i, (len, frame_type, (reserved, stream_id))) = tuple((
             be_u24,
-            map_res(be_u8, |u| {
-                RawFrameType::from_repr(u).ok_or(nom::error::ErrorKind::OneOf)
-            }),
-            be_u8,
+            EncodedFrameType::parse,
             parse_reserved_and_stream_id,
         ))(i)?;
 
-        let frame_type = FrameType::decode(frame_type, flags);
-
         let frame = Frame {
-            frame_type,
+            frame_type: FrameType::decode(frame_type),
             reserved,
             stream_id,
             len,
@@ -220,9 +246,9 @@ impl Frame {
             use byteorder::{BigEndian, WriteBytesExt};
             let mut header = &mut header[..];
             header.write_u24::<BigEndian>(self.len as _)?;
-            let (ty, flags) = self.frame_type.encode();
-            header.write_u8(ty.repr())?;
-            header.write_u8(flags)?;
+            let ft = self.frame_type.encode();
+            header.write_u8(ft.ty)?;
+            header.write_u8(ft.flags)?;
             // TODO: do we ever need to write the reserved bit?
             header.write_u32::<BigEndian>(self.stream_id.0)?;
         }


Test patch: