From 61dbea64da386ec24138a3b53fb362cfe0385ff0 Mon Sep 17 00:00:00 2001 From: Benjamin Saunders Date: Sat, 25 May 2024 11:16:08 -0700 Subject: [PATCH] Only populate local_cid_state when CIDs are in use --- quinn-proto/src/connection/cid_state.rs | 20 +----- quinn-proto/src/connection/mod.rs | 90 ++++++++++++++----------- 2 files changed, 50 insertions(+), 60 deletions(-) diff --git a/quinn-proto/src/connection/cid_state.rs b/quinn-proto/src/connection/cid_state.rs index abf577ae7..46267ddd7 100644 --- a/quinn-proto/src/connection/cid_state.rs +++ b/quinn-proto/src/connection/cid_state.rs @@ -21,19 +21,12 @@ pub(super) struct CidState { prev_retire_seq: u64, /// Sequence number to set in retire_prior_to field in NEW_CONNECTION_ID frame retire_seq: u64, - /// cid length used to decode short packet - cid_len: usize, //// cid lifetime cid_lifetime: Option, } impl CidState { - pub(crate) fn new( - cid_len: usize, - cid_lifetime: Option, - now: Instant, - issued: u64, - ) -> Self { + pub(crate) fn new(cid_lifetime: Option, now: Instant, issued: u64) -> Self { let mut active_seq = FxHashSet::default(); // Add sequence number of CIDs used in handshaking into tracking set for seq in 0..issued { @@ -45,7 +38,6 @@ impl CidState { active_seq, prev_retire_seq: 0, retire_seq: 0, - cid_len, cid_lifetime, }; // Track lifetime of CIDs used in handshaking @@ -158,11 +150,6 @@ impl CidState { sequence: u64, limit: u64, ) -> Result { - if self.cid_len == 0 { - return Err(TransportError::PROTOCOL_VIOLATION( - "RETIRE_CONNECTION_ID when CIDs aren't in use", - )); - } if sequence > self.issued { debug!( sequence, @@ -181,11 +168,6 @@ impl CidState { Ok(limit > self.active_seq.len() as u64) } - /// Length of local Connection IDs - pub(crate) fn cid_len(&self) -> usize { - self.cid_len - } - /// The value for `retire_prior_to` field in `NEW_CONNECTION_ID` frame pub(crate) fn retire_prior_to(&self) -> u64 { self.retire_seq diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index a5d498c57..aaf8bb5d3 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -231,8 +231,8 @@ pub struct Connection { streams: StreamsState, /// Surplus remote CIDs for future use on new paths rem_cids: CidQueue, - // Attributes of CIDs generated by local peer - local_cid_state: CidState, + /// Attributes of CIDs generated by local peer, if in use + local_cid_state: Option, /// State of the unreliable datagram extension datagrams: DatagramState, /// Connection level statistics @@ -281,12 +281,14 @@ impl Connection { crypto, handshake_cid: loc_cid, rem_handshake_cid: rem_cid, - local_cid_state: CidState::new( - cid_gen.cid_len(), - cid_gen.cid_lifetime(), - now, - if pref_addr_cid.is_some() { 2 } else { 1 }, - ), + local_cid_state: match cid_gen.cid_len() { + 0 => None, + _ => Some(CidState::new( + cid_gen.cid_lifetime(), + now, + if pref_addr_cid.is_some() { 2 } else { 1 }, + )), + }, path: PathData::new(remote, allow_mtud, None, now, path_validated, &config), allow_mtud, local_ip, @@ -1088,7 +1090,8 @@ impl Connection { } } NewIdentifiers(ids, now) => { - self.local_cid_state.new_cids(&ids, now); + let cid_state = self.local_cid_state.as_mut().unwrap(); + cid_state.new_cids(&ids, now); ids.into_iter().rev().for_each(|frame| { self.spaces[SpaceId::Data].pending.new_cids.push(frame); }); @@ -1098,7 +1101,9 @@ impl Connection { .get(Timer::PushNewCid) .map_or(true, |x| x <= now) { - self.reset_cid_retirement(); + if let Some(t) = cid_state.next_timeout() { + self.timers.set(Timer::PushNewCid, t); + } } } } @@ -1149,12 +1154,13 @@ impl Connection { } Timer::Pacing => trace!("pacing timer expired"), Timer::PushNewCid => { + let cid_state = self.local_cid_state.as_mut().unwrap(); // Update `retire_prior_to` field in NEW_CONNECTION_ID frame - let num_new_cid = self.local_cid_state.on_cid_timeout().into(); + let num_new_cid = cid_state.on_cid_timeout().into(); if !self.state.is_closed() { trace!( "push a new cid to peer RETIRE_PRIOR_TO field {}", - self.local_cid_state.retire_prior_to() + cid_state.retire_prior_to() ); self.endpoint_events .push_back(EndpointEventInner::NeedIdentifiers(now, num_new_cid)); @@ -1860,12 +1866,6 @@ impl Connection { self.timers.set(Timer::KeepAlive, now + interval); } - fn reset_cid_retirement(&mut self) { - if let Some(t) = self.local_cid_state.next_timeout() { - self.timers.set(Timer::PushNewCid, t); - } - } - /// Handle the already-decrypted first packet from the client /// /// Decrypting the first packet in the `Endpoint` allows stateless packet handling to be more @@ -2756,8 +2756,12 @@ impl Connection { self.streams.received_stop_sending(id, error_code); } Frame::RetireConnectionId { sequence } => { - let allow_more_cids = self - .local_cid_state + let cid_state = self.local_cid_state.as_mut().ok_or_else(|| { + TransportError::PROTOCOL_VIOLATION( + "RETIRE_CONNECTION_ID when CIDs aren't in use", + ) + })?; + let allow_more_cids = cid_state .on_cid_retirement(sequence, self.peer_params.issue_cids_limit())?; self.endpoint_events .push_back(EndpointEventInner::RetireConnectionId( @@ -2999,7 +3003,7 @@ impl Connection { /// Issue an initial set of connection IDs to the peer upon connection fn issue_first_cids(&mut self, now: Instant) { - if self.local_cid_state.cid_len() == 0 { + if self.local_cid_state.is_none() { return; } @@ -3169,25 +3173,27 @@ impl Connection { } // NEW_CONNECTION_ID - while buf.len() + 44 < max_size { - let issued = match space.pending.new_cids.pop() { - Some(x) => x, - None => break, - }; - trace!( - sequence = issued.sequence, - id = %issued.id, - "NEW_CONNECTION_ID" - ); - frame::NewConnectionId { - sequence: issued.sequence, - retire_prior_to: self.local_cid_state.retire_prior_to(), - id: issued.id, - reset_token: issued.reset_token, + if let Some(cid_state) = self.local_cid_state.as_ref() { + while buf.len() + 44 < max_size { + let issued = match space.pending.new_cids.pop() { + Some(x) => x, + None => break, + }; + trace!( + sequence = issued.sequence, + id = %issued.id, + "NEW_CONNECTION_ID" + ); + frame::NewConnectionId { + sequence: issued.sequence, + retire_prior_to: cid_state.retire_prior_to(), + id: issued.id, + reset_token: issued.reset_token, + } + .encode(buf); + sent.retransmits.get_or_create().new_cids.push(issued); + self.stats.frame_tx.new_connection_id += 1; } - .encode(buf); - sent.retransmits.get_or_create().new_cids.push(issued); - self.stats.frame_tx.new_connection_id += 1; } // RETIRE_CONNECTION_ID @@ -3481,14 +3487,16 @@ impl Connection { #[cfg(test)] pub(crate) fn active_local_cid_seq(&self) -> (u64, u64) { - self.local_cid_state.active_seq() + self.local_cid_state + .as_ref() + .map_or((u64::MAX, u64::MIN), |state| state.active_seq()) } /// Instruct the peer to replace previously issued CIDs by sending a NEW_CONNECTION_ID frame /// with updated `retire_prior_to` field set to `v` #[cfg(test)] pub(crate) fn rotate_local_cid(&mut self, v: u64, now: Instant) { - let n = self.local_cid_state.assign_retire_seq(v); + let n = self.local_cid_state.as_mut().unwrap().assign_retire_seq(v); self.endpoint_events .push_back(EndpointEventInner::NeedIdentifiers(now, n)); }