Skip to content

Commit

Permalink
fix: Handle H3 grease frame types (#1996)
Browse files Browse the repository at this point in the history 
* fix: Handle H3 grease stream types

Doesn't work yet, since it stops reading from the stream afterwards.

Fixes #1991

* Add test and fix bug

* Handle non-HEADERS first frames

* Update neqo-http3/src/stream_type_reader.rs

Co-authored-by: Martin Thomson <[email protected]>
Signed-off-by: Lars Eggert <[email protected]>

* Suggestion from @martinthomson

* In debug builds, add a grease frame before the headers.

* Revert "In debug builds, add a grease frame before the headers."

This is breaking too many tests.

This reverts commit 7b9ea7f.

* Simplify

* HFrameType is a struct now

* Bump coverage

---------

Signed-off-by: Lars Eggert <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
  • Loading branch information
@larseggert @martinthomson
larseggert and martinthomson committed Aug 12, 2024
1 parent 39c7b9f commit fe2f0d0
Show file tree
Hide file tree
Showing 9 changed files with 109 additions and 65 deletions.
14 changes: 8 additions & 6 deletions neqo-http3/src/connection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,9 @@ impl Http3Connection {
Ok(ReceiveOutput::ControlFrames(rest))
}
ReceiveOutput::NewStream(
NewStreamType::Push(_) | NewStreamType::Http | NewStreamType::WebTransportStream(_),
NewStreamType::Push(_)
| NewStreamType::Http(_)
| NewStreamType::WebTransportStream(_),
) => Ok(output),
ReceiveOutput::NewStream(_) => {
unreachable!("NewStream should have been handled already")
Expand Down Expand Up @@ -723,7 +725,7 @@ impl Http3Connection {
)),
);
}
NewStreamType::Http => {
NewStreamType::Http(_) => {
qinfo!([self], "A new http stream {}.", stream_id);
}
NewStreamType::WebTransportStream(session_id) => {
Expand Down Expand Up @@ -755,9 +757,9 @@ impl Http3Connection {
NewStreamType::Control | NewStreamType::Decoder | NewStreamType::Encoder => {
self.stream_receive(conn, stream_id)
}
NewStreamType::Push(_) | NewStreamType::Http | NewStreamType::WebTransportStream(_) => {
Ok(ReceiveOutput::NewStream(stream_type))
}
NewStreamType::Push(_)
| NewStreamType::Http(_)
| NewStreamType::WebTransportStream(_) => Ok(ReceiveOutput::NewStream(stream_type)),
NewStreamType::Unknown => Ok(ReceiveOutput::NoOutput),
}
}
Expand Down Expand Up @@ -919,7 +921,7 @@ impl Http3Connection {
message_type: MessageType::Response,
stream_type,
stream_id,
header_frame_type_read: false,
first_frame_type: None,
},
Rc::clone(&self.qpack_decoder),
recv_events,
Expand Down
4 changes: 2 additions & 2 deletions neqo-http3/src/connection_client.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1094,7 +1094,7 @@ impl Http3Client {
ReceiveOutput::NewStream(NewStreamType::Push(push_id)) => {
self.handle_new_push_stream(stream_id, push_id)
}
ReceiveOutput::NewStream(NewStreamType::Http) => Err(Error::HttpStreamCreation),
ReceiveOutput::NewStream(NewStreamType::Http(_)) => Err(Error::HttpStreamCreation),
ReceiveOutput::NewStream(NewStreamType::WebTransportStream(session_id)) => {
self.base_handler.webtransport_create_stream_remote(
StreamId::from(session_id),
Expand Down Expand Up @@ -1162,7 +1162,7 @@ impl Http3Client {
message_type: MessageType::Response,
stream_type: Http3StreamType::Push,
stream_id,
header_frame_type_read: false,
first_frame_type: None,
},
Rc::clone(&self.base_handler.qpack_decoder),
Box::new(RecvPushEvents::new(push_id, Rc::clone(&self.push_handler))),
Expand Down
4 changes: 2 additions & 2 deletions neqo-http3/src/connection_server.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ impl Http3ServerHandler {
fn handle_stream_readable(&mut self, conn: &mut Connection, stream_id: StreamId) -> Res<()> {
match self.base_handler.handle_stream_readable(conn, stream_id)? {
ReceiveOutput::NewStream(NewStreamType::Push(_)) => Err(Error::HttpStreamCreation),
ReceiveOutput::NewStream(NewStreamType::Http) => {
ReceiveOutput::NewStream(NewStreamType::Http(first_frame_type)) => {
self.base_handler.add_streams(
stream_id,
Box::new(SendMessage::new(
Expand All @@ -333,7 +333,7 @@ impl Http3ServerHandler {
message_type: MessageType::Request,
stream_type: Http3StreamType::Http,
stream_id,
header_frame_type_read: true,
first_frame_type: Some(first_frame_type),
},
Rc::clone(&self.base_handler.qpack_decoder),
Box::new(self.events.clone()),
Expand Down
2 changes: 1 addition & 1 deletion neqo-http3/src/features/extended_connect/webtransport_session.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ impl WebTransportSession {
message_type: MessageType::Response,
stream_type: Http3StreamType::ExtendedConnect,
stream_id: session_id,
header_frame_type_read: false,
first_frame_type: None,
},
qpack_decoder,
Box::new(stream_event_listener.clone()),
Expand Down
44 changes: 29 additions & 15 deletions neqo-http3/src/frames/hframe.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,31 @@ use neqo_transport::StreamId;

use crate::{frames::reader::FrameDecoder, settings::HSettings, Error, Priority, Res};

pub type HFrameType = u64;
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct HFrameType(pub u64);

pub const H3_FRAME_TYPE_DATA: HFrameType = 0x0;
pub const H3_FRAME_TYPE_HEADERS: HFrameType = 0x1;
pub const H3_FRAME_TYPE_CANCEL_PUSH: HFrameType = 0x3;
pub const H3_FRAME_TYPE_SETTINGS: HFrameType = 0x4;
pub const H3_FRAME_TYPE_PUSH_PROMISE: HFrameType = 0x5;
pub const H3_FRAME_TYPE_GOAWAY: HFrameType = 0x7;
pub const H3_FRAME_TYPE_MAX_PUSH_ID: HFrameType = 0xd;
pub const H3_FRAME_TYPE_PRIORITY_UPDATE_REQUEST: HFrameType = 0xf0700;
pub const H3_FRAME_TYPE_PRIORITY_UPDATE_PUSH: HFrameType = 0xf0701;
pub const H3_FRAME_TYPE_DATA: HFrameType = HFrameType(0x0);
pub const H3_FRAME_TYPE_HEADERS: HFrameType = HFrameType(0x1);
pub const H3_FRAME_TYPE_CANCEL_PUSH: HFrameType = HFrameType(0x3);
pub const H3_FRAME_TYPE_SETTINGS: HFrameType = HFrameType(0x4);
pub const H3_FRAME_TYPE_PUSH_PROMISE: HFrameType = HFrameType(0x5);
pub const H3_FRAME_TYPE_GOAWAY: HFrameType = HFrameType(0x7);
pub const H3_FRAME_TYPE_MAX_PUSH_ID: HFrameType = HFrameType(0xd);
pub const H3_FRAME_TYPE_PRIORITY_UPDATE_REQUEST: HFrameType = HFrameType(0xf0700);
pub const H3_FRAME_TYPE_PRIORITY_UPDATE_PUSH: HFrameType = HFrameType(0xf0701);

pub const H3_RESERVED_FRAME_TYPES: &[HFrameType] = &[0x2, 0x6, 0x8, 0x9];
pub const H3_RESERVED_FRAME_TYPES: &[HFrameType] = &[
HFrameType(0x2),
HFrameType(0x6),
HFrameType(0x8),
HFrameType(0x9),
];

impl From<HFrameType> for u64 {
fn from(t: HFrameType) -> Self {
t.0
}
}

// data for DATA frame is not read into HFrame::Data.
#[derive(PartialEq, Eq, Debug)]
Expand Down Expand Up @@ -74,7 +86,9 @@ impl HFrame {
Self::MaxPushId { .. } => H3_FRAME_TYPE_MAX_PUSH_ID,
Self::PriorityUpdateRequest { .. } => H3_FRAME_TYPE_PRIORITY_UPDATE_REQUEST,
Self::PriorityUpdatePush { .. } => H3_FRAME_TYPE_PRIORITY_UPDATE_PUSH,
Self::Grease => Decoder::from(&random::<7>()).decode_uint(7).unwrap() * 0x1f + 0x21,
Self::Grease => {
HFrameType(Decoder::from(&random::<7>()).decode_uint(7).unwrap() * 0x1f + 0x21)
}
}
}

Expand Down Expand Up @@ -143,14 +157,14 @@ impl HFrame {
}

impl FrameDecoder<Self> for HFrame {
fn frame_type_allowed(frame_type: u64) -> Res<()> {
fn frame_type_allowed(frame_type: HFrameType) -> Res<()> {
if H3_RESERVED_FRAME_TYPES.contains(&frame_type) {
return Err(Error::HttpFrameUnexpected);
}
Ok(())
}

fn decode(frame_type: u64, frame_len: u64, data: Option<&[u8]>) -> Res<Option<Self>> {
fn decode(frame_type: HFrameType, frame_len: u64, data: Option<&[u8]>) -> Res<Option<Self>> {
if frame_type == H3_FRAME_TYPE_DATA {
Ok(Some(Self::Data { len: frame_len }))
} else if let Some(payload) = data {
Expand Down Expand Up @@ -207,7 +221,7 @@ impl FrameDecoder<Self> for HFrame {
}
}

fn is_known_type(frame_type: u64) -> bool {
fn is_known_type(frame_type: HFrameType) -> bool {
matches!(
frame_type,
H3_FRAME_TYPE_DATA
Expand Down
22 changes: 12 additions & 10 deletions neqo-http3/src/frames/reader.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,25 @@ use neqo_common::{
};
use neqo_transport::{Connection, StreamId};

use super::hframe::HFrameType;
use crate::{Error, RecvStream, Res};

const MAX_READ_SIZE: usize = 4096;

pub trait FrameDecoder<T> {
fn is_known_type(frame_type: u64) -> bool;
fn is_known_type(frame_type: HFrameType) -> bool;

/// # Errors
///
/// Returns `HttpFrameUnexpected` if frames is not alowed, i.e. is a `H3_RESERVED_FRAME_TYPES`.
fn frame_type_allowed(_frame_type: u64) -> Res<()> {
fn frame_type_allowed(_frame_type: HFrameType) -> Res<()> {
Ok(())
}

/// # Errors
///
/// If a frame cannot be properly decoded.
fn decode(frame_type: u64, frame_len: u64, data: Option<&[u8]>) -> Res<Option<T>>;
fn decode(frame_type: HFrameType, frame_len: u64, data: Option<&[u8]>) -> Res<Option<T>>;
}

pub trait StreamReader {
Expand Down Expand Up @@ -95,7 +97,7 @@ enum FrameReaderState {
#[derive(Debug)]
pub struct FrameReader {
state: FrameReaderState,
frame_type: u64,
frame_type: HFrameType,
frame_len: u64,
}

Expand All @@ -112,13 +114,13 @@ impl FrameReader {
state: FrameReaderState::GetType {
decoder: IncrementalDecoderUint::default(),
},
frame_type: 0,
frame_type: HFrameType(u64::MAX),
frame_len: 0,
}
}

#[must_use]
pub fn new_with_type(frame_type: u64) -> Self {
pub fn new_with_type(frame_type: HFrameType) -> Self {
Self {
state: FrameReaderState::GetLength {
decoder: IncrementalDecoderUint::default(),
Expand Down Expand Up @@ -202,13 +204,13 @@ impl FrameReader {
FrameReaderState::GetType { decoder } => {
if let Some(v) = decoder.consume(&mut input) {
qtrace!("FrameReader::receive: read frame type {}", v);
self.frame_type_decoded::<T>(v)?;
self.frame_type_decoded::<T>(HFrameType(v))?;
}
}
FrameReaderState::GetLength { decoder } => {
if let Some(len) = decoder.consume(&mut input) {
qtrace!(
"FrameReader::receive: frame type {} length {}",
"FrameReader::receive: frame type {:?} length {}",
self.frame_type,
len
);
Expand All @@ -218,7 +220,7 @@ impl FrameReader {
FrameReaderState::GetData { decoder } => {
if let Some(data) = decoder.consume(&mut input) {
qtrace!(
"received frame {}: {}",
"received frame {:?}: {}",
self.frame_type,
hex_with_len(&data[..])
);
Expand All @@ -236,7 +238,7 @@ impl FrameReader {
}

impl FrameReader {
fn frame_type_decoded<T: FrameDecoder<T>>(&mut self, frame_type: u64) -> Res<()> {
fn frame_type_decoded<T: FrameDecoder<T>>(&mut self, frame_type: HFrameType) -> Res<()> {
T::frame_type_allowed(frame_type)?;
self.frame_type = frame_type;
self.state = FrameReaderState::GetLength {
Expand Down
9 changes: 5 additions & 4 deletions neqo-http3/src/frames/wtframe.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use neqo_common::{Decoder, Encoder};

use super::hframe::HFrameType;
use crate::{frames::reader::FrameDecoder, Error, Res};

pub type WebTransportFrameType = u64;
Expand All @@ -29,10 +30,10 @@ impl WebTransportFrame {
}

impl FrameDecoder<Self> for WebTransportFrame {
fn decode(frame_type: u64, frame_len: u64, data: Option<&[u8]>) -> Res<Option<Self>> {
fn decode(frame_type: HFrameType, frame_len: u64, data: Option<&[u8]>) -> Res<Option<Self>> {
if let Some(payload) = data {
let mut dec = Decoder::from(payload);
if frame_type == WT_FRAME_CLOSE_SESSION {
if frame_type == HFrameType(WT_FRAME_CLOSE_SESSION) {
if frame_len > WT_FRAME_CLOSE_MAX_MESSAGE_SIZE + 4 {
return Err(Error::HttpMessageError);
}
Expand All @@ -50,7 +51,7 @@ impl FrameDecoder<Self> for WebTransportFrame {
}
}

fn is_known_type(frame_type: u64) -> bool {
frame_type == WT_FRAME_CLOSE_SESSION
fn is_known_type(frame_type: HFrameType) -> bool {
frame_type == HFrameType(WT_FRAME_CLOSE_SESSION)
}
}
14 changes: 7 additions & 7 deletions neqo-http3/src/recv_message.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use neqo_qpack::decoder::QPackDecoder;
use neqo_transport::{Connection, StreamId};

use crate::{
frames::{FrameReader, HFrame, StreamReaderConnectionWrapper, H3_FRAME_TYPE_HEADERS},
frames::{hframe::HFrameType, FrameReader, HFrame, StreamReaderConnectionWrapper},
headers_checks::{headers_valid, is_interim},
priority::PriorityHandler,
push_controller::PushController,
Expand All @@ -24,7 +24,7 @@ pub struct RecvMessageInfo {
pub message_type: MessageType,
pub stream_type: Http3StreamType,
pub stream_id: StreamId,
pub header_frame_type_read: bool,
pub first_frame_type: Option<u64>,
}

/*
Expand Down Expand Up @@ -94,11 +94,11 @@ impl RecvMessage {
) -> Self {
Self {
state: RecvMessageState::WaitingForResponseHeaders {
frame_reader: if message_info.header_frame_type_read {
FrameReader::new_with_type(H3_FRAME_TYPE_HEADERS)
} else {
FrameReader::new()
},
frame_reader: message_info
.first_frame_type
.map_or_else(FrameReader::new, |frame_type| {
FrameReader::new_with_type(HFrameType(frame_type))
}),
},
message_type: message_info.message_type,
stream_type: message_info.stream_type,
Expand Down
Loading

0 comments on commit fe2f0d0

Please sign in to comment.