Skip to content

Commit

Permalink
Add a test-only send_settings config option (#207)
Browse files Browse the repository at this point in the history
* tests: Remove incorrect driver comments

In all of these test cases, the driver _is_ polled.

* tests: Poll driver in header_too_big_discard_from_client

It is actually safe to poll the driver, since:

1. It only matters that the server has a false impression of the
   client's max_field_section_size and
2. It doesn't matter if the client knows about the server's
   max_field_section_size or not

* Add a test-only send_settings config option

Instead of reading into the server's RequestStream and manually
overriding its shared state, just have a test-only config option to have
the client not send its settings.
  • Loading branch information
dongcarl authored Jul 20, 2023
1 parent dccb3cd commit 3991dca
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 55 deletions.
6 changes: 6 additions & 0 deletions h3/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,12 @@ impl Builder {
}
}

#[cfg(test)]
pub fn send_settings(&mut self, value: bool) -> &mut Self {
self.config.send_settings = value;
self
}

/// Set the maximum header size this client is willing to accept
///
/// See [header size constraints] section of the specification for details.
Expand Down
6 changes: 6 additions & 0 deletions h3/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ pub struct Config {
/// In HTTP/3, the concept of grease is used to ensure that the protocol can evolve
/// and accommodate future changes without breaking existing implementations.
pub(crate) send_grease: bool,

#[cfg(test)]
pub(crate) send_settings: bool,

/// The MAX_FIELD_SECTION_SIZE in HTTP/3 refers to the maximum size of the dynamic table used in HPACK compression.
/// HPACK is the compression algorithm used in HTTP/3 to reduce the size of the header fields in HTTP requests and responses.

Expand Down Expand Up @@ -56,6 +60,8 @@ impl Default for Config {
Self {
max_field_section_size: VarInt::MAX.0,
send_grease: true,
#[cfg(test)]
send_settings: true,
enable_webtransport: false,
enable_extended_connect: false,
enable_datagram: false,
Expand Down
44 changes: 28 additions & 16 deletions h3/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,45 +142,40 @@ where
C: quic::Connection<B>,
B: Buf,
{
/// Initiates the connection and opens a control stream
pub async fn new(mut conn: C, shared: SharedStateRef, config: Config) -> Result<Self, Error> {
//= https://www.rfc-editor.org/rfc/rfc9114#section-6.2
//# Endpoints SHOULD create the HTTP control stream as well as the
//# unidirectional streams required by mandatory extensions (such as the
//# QPACK encoder and decoder streams) first, and then create additional
//# streams as allowed by their peer.
let mut control_send = future::poll_fn(|cx| conn.poll_open_send(cx))
.await
.map_err(|e| Code::H3_STREAM_CREATION_ERROR.with_transport(e))?;
pub async fn send_settings(&mut self) -> Result<(), Error> {
#[cfg(test)]
if !self.config.send_settings {
return Ok(());
}

let mut settings = Settings::default();

settings
.insert(
SettingId::MAX_HEADER_LIST_SIZE,
config.max_field_section_size,
self.config.max_field_section_size,
)
.map_err(|e| Code::H3_INTERNAL_ERROR.with_cause(e))?;

settings
.insert(
SettingId::ENABLE_CONNECT_PROTOCOL,
config.enable_extended_connect as u64,
self.config.enable_extended_connect as u64,
)
.map_err(|e| Code::H3_INTERNAL_ERROR.with_cause(e))?;
settings
.insert(
SettingId::ENABLE_WEBTRANSPORT,
config.enable_webtransport as u64,
self.config.enable_webtransport as u64,
)
.map_err(|e| Code::H3_INTERNAL_ERROR.with_cause(e))?;
settings
.insert(SettingId::H3_DATAGRAM, config.enable_datagram as u64)
.insert(SettingId::H3_DATAGRAM, self.config.enable_datagram as u64)
.map_err(|e| Code::H3_INTERNAL_ERROR.with_cause(e))?;

tracing::debug!("Sending server settings: {:#x?}", settings);

if config.send_grease {
if self.config.send_grease {
// Grease Settings (https://www.rfc-editor.org/rfc/rfc9114.html#name-defined-settings-parameters)
//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.4.1
//# Setting identifiers of the format 0x1f * N + 0x21 for non-negative
Expand Down Expand Up @@ -228,11 +223,25 @@ where
//# as soon as the transport is ready to send data.
trace!("Sending Settings frame: {:#x?}", settings);
stream::write(
&mut control_send,
&mut self.control_send,
WriteBuf::from(UniStreamHeader::Control(settings)),
)
.await?;

Ok(())
}

/// Initiates the connection and opens a control stream
pub async fn new(mut conn: C, shared: SharedStateRef, config: Config) -> Result<Self, Error> {
//= https://www.rfc-editor.org/rfc/rfc9114#section-6.2
//# Endpoints SHOULD create the HTTP control stream as well as the
//# unidirectional streams required by mandatory extensions (such as the
//# QPACK encoder and decoder streams) first, and then create additional
//# streams as allowed by their peer.
let control_send = future::poll_fn(|cx| conn.poll_open_send(cx))
.await
.map_err(|e| Code::H3_STREAM_CREATION_ERROR.with_transport(e))?;

//= https://www.rfc-editor.org/rfc/rfc9114#section-6.2.1
//= type=implication
//# The
Expand All @@ -251,6 +260,9 @@ where
config,
accepted_streams: Default::default(),
};

conn_inner.send_settings().await?;

// start a grease stream
if config.send_grease {
//= https://www.rfc-editor.org/rfc/rfc9114#section-7.2.8
Expand Down
6 changes: 6 additions & 0 deletions h3/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,12 @@ impl Builder {
}
}

#[cfg(test)]
pub fn send_settings(&mut self, value: bool) -> &mut Self {
self.config.send_settings = value;
self
}

/// Set the maximum header size this client is willing to accept
///
/// See [header size constraints] section of the specification for details.
Expand Down
68 changes: 29 additions & 39 deletions h3/src/tests/request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@ async fn header_too_big_response_from_server() {
let mut server = pair.server();

let client_fut = async {
// Do not poll driver so client doesn't know about server's max_field section size setting
let (mut driver, mut client) = client::new(pair.client().await).await.expect("client init");
let drive_fut = async { future::poll_fn(|cx| driver.poll_close(cx)).await };
let req_fut = async {
Expand Down Expand Up @@ -310,7 +309,6 @@ async fn header_too_big_response_from_server_trailers() {
let mut server = pair.server();

let client_fut = async {
// Do not poll driver so client doesn't know about server's max_field_section_size setting
let (mut driver, mut client) = client::new(pair.client().await).await.expect("client init");
let drive_fut = async { future::poll_fn(|cx| driver.poll_close(cx)).await };
let req_fut = async {
Expand Down Expand Up @@ -426,7 +424,6 @@ async fn header_too_big_client_error_trailer() {
let mut server = pair.server();

let client_fut = async {
// Do not poll driver so client doesn't know about server's max_field_section_size setting
let (mut driver, mut client) = client::new(pair.client().await).await.expect("client init");
let drive_fut = async { future::poll_fn(|cx| driver.poll_close(cx)).await };
let req_fut = async {
Expand Down Expand Up @@ -505,46 +502,45 @@ async fn header_too_big_discard_from_client() {
//# that exceeds the indicated size, as the peer will likely refuse to
//# process it.

// Do not poll driver so client doesn't know about server's max_field section size setting
let (_conn, mut client) = client::builder()
let (mut driver, mut client) = client::builder()
.max_field_section_size(12)
// Don't send settings, so server doesn't know about the low max_field_section_size
.send_settings(false)
.build::<_, _, Bytes>(pair.client().await)
.await
.expect("client init");
let mut request_stream = client
.send_request(Request::get("http://localhost/salut").body(()).unwrap())
.await
.expect("request");
request_stream.finish().await.expect("client finish");
let err_kind = request_stream.recv_response().await.unwrap_err().kind();
assert_matches!(
err_kind,
Kind::HeaderTooBig {
actual_size: 42,
max_size: 12,
..
}
);
let drive_fut = async { future::poll_fn(|cx| driver.poll_close(cx)).await };
let req_fut = async {
let mut request_stream = client
.send_request(Request::get("http://localhost/salut").body(()).unwrap())
.await
.expect("request");
request_stream.finish().await.expect("client finish");
let err_kind = request_stream.recv_response().await.unwrap_err().kind();
assert_matches!(
err_kind,
Kind::HeaderTooBig {
actual_size: 42,
max_size: 12,
..
}
);

let mut request_stream = client
.send_request(Request::get("http://localhost/salut").body(()).unwrap())
.await
.expect("request");
request_stream.finish().await.expect("client finish");
let _ = request_stream.recv_response().await.unwrap_err();
let mut request_stream = client
.send_request(Request::get("http://localhost/salut").body(()).unwrap())
.await
.expect("request");
request_stream.finish().await.expect("client finish");
let _ = request_stream.recv_response().await.unwrap_err();
};
tokio::select! {biased; _ = req_fut => (), _ = drive_fut => () }
};

let server_fut = async {
let conn = server.next().await;
let mut incoming_req = server::Connection::new(conn).await.unwrap();

let (_request, mut request_stream) = incoming_req.accept().await.expect("accept").unwrap();
// pretend server didn't receive settings
incoming_req
.shared_state()
.write("client")
.peer_config
.max_field_section_size = u64::MAX;
request_stream
.send_response(
Response::builder()
Expand Down Expand Up @@ -591,9 +587,10 @@ async fn header_too_big_discard_from_client_trailers() {
//# that exceeds the indicated size, as the peer will likely refuse to
//# process it.

// Do not poll driver so client doesn't know about server's max_field section size setting
let (mut driver, mut client) = client::builder()
.max_field_section_size(200)
// Don't send settings, so server doesn't know about the low max_field_section_size
.send_settings(false)
.build::<_, _, Bytes>(pair.client().await)
.await
.expect("client init");
Expand Down Expand Up @@ -627,13 +624,6 @@ async fn header_too_big_discard_from_client_trailers() {

let (_request, mut request_stream) = incoming_req.accept().await.expect("accept").unwrap();

// pretend server didn't receive settings
incoming_req
.shared_state()
.write("server")
.peer_config
.max_field_section_size = u64::MAX;

request_stream
.send_response(
Response::builder()
Expand Down

0 comments on commit 3991dca

Please sign in to comment.