diff --git a/neqo-common/Cargo.toml b/neqo-common/Cargo.toml index 699c799b44..c9f57eaeea 100644 --- a/neqo-common/Cargo.toml +++ b/neqo-common/Cargo.toml @@ -8,11 +8,16 @@ license = "MIT/Apache-2.0" build = "build.rs" [dependencies] -log = {version = "0.4.0", default-features = false} +log = {version = "=0.4.17", default-features = false} env_logger = {version = "0.9", default-features = false} lazy_static = "1.3.0" qlog = "0.4.0" chrono = {version = "0.4.10", default-features = false, features = ["std"]} +# Pinning dependency for CI +# error: package `enumset v1.1.5` cannot be built because it requires rustc 1.61 or newer, while the currently active rustc version is 1.57.0 +enumset = "=1.0.12" +# error[E0658]: use of unstable library feature 'mixed_integer_ops' +unicode-width = "=0.1.10" [features] default = ["deny-warnings"] diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 95f24f24b9..eece932337 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -1468,6 +1468,17 @@ impl Connection { // on the assert for doesn't exist. // OK, we have a valid packet. + // Get the next packet number we'll send, for ACK verification. + // TODO: Once PR #2118 lands, this can move to `input_frame`. For now, it needs to be here, + // because we can drop packet number spaces as we parse throught the packet, and if an ACK + // frame follows a CRYPTO frame that makes us drop a space, we need to know this + // packet number to verify the ACK against. + let next_pn = self + .crypto + .states + .select_tx(self.version, PacketNumberSpace::from(packet.packet_type())) + .map_or(0, |(_, tx)| tx.next_pn()); + let mut ack_eliciting = false; let mut probing = true; let mut d = Decoder::from(&packet[..]); @@ -1492,7 +1503,14 @@ impl Connection { ack_eliciting |= f.ack_eliciting(); probing &= f.path_probing(); let t = f.get_type(); - if let Err(e) = self.input_frame(path, packet.version(), packet.packet_type(), f, now) { + if let Err(e) = self.input_frame( + path, + packet.version(), + packet.packet_type(), + f, + next_pn, + now, + ) { self.capture_error(Some(Rc::clone(path)), now, t, Err(e))?; } } @@ -2560,6 +2578,7 @@ impl Connection { packet_version: Version, packet_type: PacketType, frame: Frame, + next_pn: PacketNumber, now: Instant, ) -> Res<()> { if !frame.is_allowed(packet_type) { @@ -2594,9 +2613,17 @@ impl Connection { first_ack_range, ack_ranges, } => { + // Ensure that the largest acknowledged packet number was actually sent. + // (If we ever start using non-contiguous packet numbers, we need to check all the + // packet numbers in the ACKed ranges.) + if largest_acknowledged >= next_pn { + qwarn!("Largest ACKed {} was never sent", largest_acknowledged); + return Err(Error::AckedUnsentPacket); + } + let ranges = Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)?; - self.handle_ack(space, largest_acknowledged, ranges, ack_delay, now); + self.handle_ack(space, ranges, ack_delay, now); } Frame::Crypto { offset, data } => { qtrace!( @@ -2771,7 +2798,6 @@ impl Connection { fn handle_ack( &mut self, space: PacketNumberSpace, - largest_acknowledged: u64, ack_ranges: R, ack_delay: u64, now: Instant, @@ -2784,11 +2810,11 @@ impl Connection { let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received( &self.paths.primary(), space, - largest_acknowledged, ack_ranges, self.decode_ack_delay(ack_delay), now, ); + let largest_acknowledged = acked_packets.first().map(SentPacket::pn); for acked in acked_packets { for token in &acked.tokens { match token { @@ -2812,7 +2838,9 @@ impl Connection { qlog::packets_lost(&mut self.qlog, &lost_packets); let stats = &mut self.stats.borrow_mut().frame_rx; stats.ack += 1; - stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged); + if let Some(largest_acknowledged) = largest_acknowledged { + stats.largest_acknowledged = max(stats.largest_acknowledged, largest_acknowledged); + } } /// When the server rejects 0-RTT we need to drop a bunch of stuff. diff --git a/neqo-transport/src/connection/tests/recovery.rs b/neqo-transport/src/connection/tests/recovery.rs index 0e3ee412f5..13b2d8d5f8 100644 --- a/neqo-transport/src/connection/tests/recovery.rs +++ b/neqo-transport/src/connection/tests/recovery.rs @@ -808,3 +808,38 @@ fn fast_pto_persistent_congestion() { client.process_input(ack.unwrap(), now); assert_eq!(cwnd(&client), CWND_MIN); } + +/// Receiving an ACK frame for a packet number that was never sent is an error. +#[test] +fn ack_for_unsent() { + /// This inserts an ACK frame into packets that ACKs a packet that was never sent. + struct AckforUnsentWriter {} + + impl FrameWriter for AckforUnsentWriter { + fn write_frames(&mut self, builder: &mut PacketBuilder) { + builder.encode_varint(FRAME_TYPE_ACK); + builder.encode_varint(666u16); // Largest ACKed + builder.encode_varint(0u8); // ACK delay + builder.encode_varint(0u8); // ACK block count + builder.encode_varint(0u8); // ACK block length + } + } + + let mut client = default_client(); + let mut server = default_server(); + connect_force_idle(&mut client, &mut server); + + server.test_frame_writer = Some(Box::new(AckforUnsentWriter {})); + let spoofed = server.process_output(now()).dgram().unwrap(); + server.test_frame_writer = None; + + // Now deliver the packet with the spoofed ACK frame + client.process_input(&spoofed, now()); + assert!(matches!( + client.state(), + State::Closing { + error: CloseReason::Transport(Error::AckedUnsentPacket), + .. + } + )); +} diff --git a/neqo-transport/src/recovery.rs b/neqo-transport/src/recovery.rs index 51becae19d..be0ea383b3 100644 --- a/neqo-transport/src/recovery.rs +++ b/neqo-transport/src/recovery.rs @@ -652,7 +652,6 @@ impl LossRecovery { &mut self, primary_path: &PathRef, pn_space: PacketNumberSpace, - largest_acked: u64, acked_ranges: R, ack_delay: Duration, now: Instant, @@ -661,36 +660,25 @@ impl LossRecovery { R: IntoIterator>, R::IntoIter: ExactSizeIterator, { - qdebug!( - [self], - "ACK for {} - largest_acked={}.", - pn_space, - largest_acked - ); - - let space = self.spaces.get_mut(pn_space); - let space = if let Some(sp) = space { - sp - } else { + let Some(space) = self.spaces.get_mut(pn_space) else { qinfo!("ACK on discarded space"); return (Vec::new(), Vec::new()); }; let (acked_packets, any_ack_eliciting) = space.remove_acked(acked_ranges, &mut self.stats.borrow_mut()); - if acked_packets.is_empty() { + let Some(largest_acked_pkt) = acked_packets.first() else { // No new information. return (Vec::new(), Vec::new()); - } + }; // Track largest PN acked per space let prev_largest_acked = space.largest_acked_sent_time; - if Some(largest_acked) > space.largest_acked { - space.largest_acked = Some(largest_acked); + if Some(largest_acked_pkt.pn()) > space.largest_acked { + space.largest_acked = Some(largest_acked_pkt.pn()); // If the largest acknowledged is newly acked and any newly acked // packet was ack-eliciting, update the RTT. (-recovery 5.1) - let largest_acked_pkt = acked_packets.first().expect("must be there"); space.largest_acked_sent_time = Some(largest_acked_pkt.time_sent); if any_ack_eliciting && largest_acked_pkt.on_primary_path() { self.rtt_sample( @@ -702,6 +690,13 @@ impl LossRecovery { } } + qdebug!( + [self], + "ACK for {} - largest_acked={}", + pn_space, + largest_acked_pkt.pn() + ); + // Perform loss detection. // PTO is used to remove lost packets from in-flight accounting. // We need to ensure that we have sent any PTO probes before they are removed @@ -1035,19 +1030,12 @@ mod tests { pub fn on_ack_received( &mut self, pn_space: PacketNumberSpace, - largest_acked: u64, - acked_ranges: Vec>, + acked_ranges: Vec>, ack_delay: Duration, now: Instant, ) -> (Vec, Vec) { - self.lr.on_ack_received( - &self.path, - pn_space, - largest_acked, - acked_ranges, - ack_delay, - now, - ) + self.lr + .on_ack_received(&self.path, pn_space, acked_ranges, ack_delay, now) } pub fn on_packet_sent(&mut self, sent_packet: SentPacket) { @@ -1191,7 +1179,6 @@ mod tests { fn ack(lr: &mut Fixture, pn: u64, delay: Duration) { lr.on_ack_received( PacketNumberSpace::ApplicationData, - pn, vec![pn..=pn], ACK_DELAY, pn_time(pn) + delay, @@ -1338,7 +1325,6 @@ mod tests { )); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 1, vec![1..=1], ACK_DELAY, pn_time(0) + (TEST_RTT * 5 / 4), @@ -1361,7 +1347,6 @@ mod tests { let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 2, vec![2..=2], ACK_DELAY, pn2_ack_time, @@ -1390,7 +1375,6 @@ mod tests { assert_eq!(super::PACKET_THRESHOLD, 3); let (_, lost) = lr.on_ack_received( PacketNumberSpace::ApplicationData, - 4, vec![2..=4], ACK_DELAY, pn_time(4), @@ -1418,7 +1402,6 @@ mod tests { lr.discard(PacketNumberSpace::Initial, now()); let (acked, lost) = lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![], Duration::from_millis(0), pn_time(0), @@ -1464,7 +1447,7 @@ mod tests { let sent_pkt = SentPacket::new(*sp, 1, pn_time(3), true, Vec::new(), ON_SENT_SIZE); let pn_space = PacketNumberSpace::from(sent_pkt.pt); lr.on_packet_sent(sent_pkt); - lr.on_ack_received(pn_space, 1, vec![1..=1], Duration::from_secs(0), pn_time(3)); + lr.on_ack_received(pn_space, vec![1..=1], Duration::from_secs(0), pn_time(3)); let mut lost = Vec::new(); lr.spaces.get_mut(pn_space).unwrap().detect_lost_packets( pn_time(3), @@ -1510,7 +1493,6 @@ mod tests { let rtt = lr.path.borrow().rtt().estimate(); lr.on_ack_received( PacketNumberSpace::Initial, - 0, vec![0..=0], Duration::new(0, 0), now() + rtt, diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index 512620904a..5f78e662b5 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -169,6 +169,11 @@ impl SentPacket { } } + /// The number of the packet. + pub const fn pn(&self) -> PacketNumber { + self.pn + } + /// Returns `true` if the packet will elicit an ACK. pub fn ack_eliciting(&self) -> bool { self.ack_eliciting