discard 1-RTT packets received before handshake completion
Even if we only decrypt the packets but not process the frames they
carry, there is still some risk for side effects (see e.g.
a0e69eda9da97ebb03ccda38f4bb58cfea572163).
In the future we may want to buffer received packets so we can later
process them as an optimization.
diff --git a/src/lib.rs b/src/lib.rs
index 6f31093..b89dd9f 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -692,14 +692,6 @@
/// Received address verification token.
token: Option<Vec<u8>>,
- /// List of frames from Application packets received before the handshake
- /// was completed.
- early_app_frames: Vec<frame::Frame>,
-
- /// Number of Application packets received before the handshake was
- /// completed.
- early_app_pkts: usize,
-
/// Error code to be sent to the peer in CONNECTION_CLOSE.
error: Option<u64>,
@@ -967,10 +959,6 @@
token: None,
- early_app_frames: Vec::new(),
-
- early_app_pkts: 0,
-
error: None,
app_error: None,
@@ -1232,6 +1220,15 @@
return Err(Error::Done);
}
+ // Discard 1-RTT packets received before handshake is completed (even
+ // if the frames are not immediately processed) to avoid potential side
+ // effects.
+ //
+ // TODO: buffer packets instead of discarding as an optimization.
+ if hdr.ty == packet::Type::Short && !self.is_established() {
+ return Ok(b.len());
+ }
+
if self.is_server && !self.did_version_negotiation {
if !version_is_supported(hdr.version) {
return Err(Error::UnknownVersion);
@@ -1370,22 +1367,6 @@
return Err(Error::Done);
}
- // Keep track of the number of Application packets received before the
- // handshake is completed, and drop any that exceed the initial
- // congestion window packet count.
- if hdr.ty == packet::Type::Short && !self.is_established() {
- self.early_app_pkts += 1;
-
- if self.early_app_pkts > recovery::INITIAL_WINDOW_PACKETS {
- error!(
- "{} dropped early application packet len={} pn={}",
- self.trace_id, payload_len, pn,
- );
-
- return Ok(header_len + payload_len);
- }
- }
-
// To avoid sending an ACK in response to an ACK-only packet, we need
// to keep track of whether this packet contains any frame other than
// ACK and PADDING.
@@ -1399,13 +1380,6 @@
ack_elicited = true;
}
- // If the packet this frame belongs to is an early Application one,
- // buffer the frame for later processing.
- if hdr.ty == packet::Type::Short && !self.is_established() {
- self.early_app_frames.push(frame);
- continue;
- }
-
self.process_frame(frame, epoch, now)?;
}
@@ -1557,7 +1531,7 @@
let is_closing = self.error.is_some() || self.app_error.is_some();
if !is_closing {
- self.do_handshake(now)?;
+ self.do_handshake()?;
}
// Use max_packet_size as sent by the peer, except during the handshake
@@ -2464,7 +2438,7 @@
/// Continues the handshake.
///
/// If the connection is already established, it does nothing.
- fn do_handshake(&mut self, now: time::Instant) -> Result<()> {
+ fn do_handshake(&mut self) -> Result<()> {
if !self.handshake_completed {
match self.handshake.do_handshake() {
Ok(_) => {
@@ -2510,15 +2484,6 @@
self.handshake.sigalg(),
self.is_resumed(),
self.peer_transport_params);
-
- // Process outstanding frames from early Application packets.
- for f in self
- .early_app_frames
- .drain(..)
- .collect::<Vec<frame::Frame>>()
- {
- self.process_frame(f, packet::EPOCH_APPLICATION, now)?;
- }
},
Err(Error::Done) => (),
@@ -2688,7 +2653,7 @@
self.handshake.provide_data(level, &recv_buf)?;
}
- self.do_handshake(now)?;
+ self.do_handshake()?;
},
// TODO: implement stateless retry
@@ -4185,9 +4150,9 @@
}
#[test]
- /// Simulates reception of an early Application packet on the server, by
+ /// Simulates reception of an early 1-RTT packet on the server, by
/// delaying the client's Handshake packet that completes the handshake.
- fn buffer_early_app_frames() {
+ fn early_1rtt_packet() {
let mut buf = [0; 65535];
let mut pipe = testing::Pipe::default().unwrap();
@@ -4207,90 +4172,39 @@
testing::recv_send(&mut pipe.server, &mut buf, 0).unwrap();
assert!(pipe.client.is_established());
- assert_eq!(pipe.client.streams.iter_mut().len(), 0);
+ // Send 1-RTT packet #0.
+ assert_eq!(pipe.client.stream_send(0, b"hello, world", true), Ok(12));
+ assert_eq!(pipe.advance(&mut buf), Ok(()));
+
+ // Send 1-RTT packet #1.
assert_eq!(pipe.client.stream_send(4, b"hello, world", true), Ok(12));
assert_eq!(pipe.advance(&mut buf), Ok(()));
- assert_eq!(pipe.client.streams.iter_mut().len(), 1);
-
assert!(!pipe.server.is_established());
- assert_eq!(pipe.server.streams.iter_mut().len(), 0);
+
+ // Client sent 1-RTT packets 0 and 1, but server hasn't received them.
+ //
+ // Note that `largest_rx_pkt_num` is initialized to 0, so we need to
+ // send another 1-RTT packet to make this check meaningful.
+ assert_eq!(
+ pipe.server.pkt_num_spaces[packet::EPOCH_APPLICATION]
+ .largest_rx_pkt_num,
+ 0
+ );
// Process delayed packet.
pipe.server.recv(&mut delayed).unwrap();
assert!(pipe.server.is_established());
- assert_eq!(pipe.server.streams.iter_mut().len(), 1);
- assert_eq!(pipe.client.stats().sent, pipe.server.stats().recv);
- }
+ assert_eq!(
+ pipe.server.pkt_num_spaces[packet::EPOCH_APPLICATION]
+ .largest_rx_pkt_num,
+ 0
+ );
- #[test]
- /// Simulates reception of multiple early Application packets on the server
- /// exceeding the limit imposed for buffering of their frames.
- fn buffer_early_app_frames_limit() {
- let mut buf = [0; 65535];
-
- let mut config = Config::new(crate::PROTOCOL_VERSION).unwrap();
- config
- .load_cert_chain_from_pem_file("examples/cert.crt")
- .unwrap();
- config
- .load_priv_key_from_pem_file("examples/cert.key")
- .unwrap();
- config
- .set_application_protos(b"\x06proto1\x06proto2")
- .unwrap();
- config.set_initial_max_data(256);
- config.set_initial_max_stream_data_bidi_local(256);
- config.set_initial_max_stream_data_bidi_remote(256);
- config.set_initial_max_streams_bidi(13);
- config.set_initial_max_streams_uni(13);
- config.verify_peer(false);
-
- let mut pipe = testing::Pipe::with_config(&mut config).unwrap();
-
- // Client sends initial flight
- let mut len = pipe.client.send(&mut buf).unwrap();
-
- // Server sends initial flight..
- len = testing::recv_send(&mut pipe.server, &mut buf, len).unwrap();
-
- // Client sends Handshake packet.
- len = testing::recv_send(&mut pipe.client, &mut buf, len).unwrap();
-
- // Emulate handshake packet delay by not making server process client
- // packet.
- let mut delayed = (&buf[..len]).to_vec();
- testing::recv_send(&mut pipe.server, &mut buf, 0).unwrap();
-
- assert!(pipe.client.is_established());
- assert_eq!(pipe.client.streams.iter_mut().len(), 0);
-
- // Client sends `INITIAL_WINDOW_PACKETS` + 1 Application packets to
- // trigger the server's limit.
- for i in 1..=recovery::INITIAL_WINDOW_PACKETS + 1 {
- pipe.client
- .stream_send(i as u64 * 4, b"hello, world", true)
- .unwrap();
- pipe.advance(&mut buf).unwrap();
- }
-
- assert_eq!(pipe.client.streams.iter_mut().len(), 11);
-
- assert!(!pipe.server.is_established());
- assert_eq!(pipe.server.streams.iter_mut().len(), 0);
-
- // Process delayed packet.
- pipe.server.recv(&mut delayed).unwrap();
-
- // Server received `INITIAL_WINDOW_PACKETS` Application packets and
- // dropped the 11th.
- assert!(pipe.server.is_established());
- assert_eq!(pipe.server.streams.iter_mut().len(), 10);
-
- assert_eq!(pipe.client.stats().sent, pipe.server.stats().recv + 1);
+ assert_eq!(pipe.client.stats().sent, pipe.server.stats().recv + 2);
}
#[test]
diff --git a/src/stream.rs b/src/stream.rs
index 4d77e55..9299622 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -358,12 +358,6 @@
self.local_max_streams_uni_next / 2 >
self.local_max_streams_uni - self.peer_opened_streams_uni
}
-
- /// Creates an iterator over all streams.
- #[cfg(test)]
- pub fn iter_mut(&mut self) -> hash_map::IterMut<u64, Stream> {
- self.streams.iter_mut()
- }
}
/// A QUIC stream.
