Bug 1919678 - updat neqo for esr128, r=mxinden a=RyanVM

Differential Revision: https://phabricator.services.mozilla.com/D225337

Cargo.lock

@@ -3962,7 +3962,6 @@ dependencies = [
 [[package]]
 name = "neqo-bin"
 version = "0.7.9"
-source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322"
 dependencies = [
  "clap",
  "clap-verbosity-flag",
@@ -3984,7 +3983,6 @@ dependencies = [
 [[package]]
 name = "neqo-common"
 version = "0.7.9"
-source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322"
 dependencies = [
  "enum-map",
  "env_logger",
@@ -3997,7 +3995,6 @@ dependencies = [
 [[package]]
 name = "neqo-crypto"
 version = "0.7.9"
-source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322"
 dependencies = [
  "bindgen 0.69.4",
  "log",
@@ -4012,7 +4009,6 @@ dependencies = [
 [[package]]
 name = "neqo-http3"
 version = "0.7.9"
-source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322"
 dependencies = [
  "enumset",
  "log",
@@ -4029,7 +4025,6 @@ dependencies = [
 [[package]]
 name = "neqo-qpack"
 version = "0.7.9"
-source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322"
 dependencies = [
  "log",
  "neqo-common",
@@ -4042,7 +4037,6 @@ dependencies = [
 [[package]]
 name = "neqo-transport"
 version = "0.7.9"
-source = "git+https://github.com/mozilla/neqo?tag=v0.7.9#121fe683ae4b39a5b694f671abfd397cbd9b4322"
 dependencies = [
  "enum-map",
  "indexmap 2.2.6",

Cargo.toml

@@ -236,3 +236,11 @@ mio_0_8 = { package = "mio", git = "https://github.com/glandium/mio", rev = "9a2
 # Patch `gpu-descriptor` 0.3.0 to remove unnecessary `allocator-api2` dep.:
 # Still waiting for the now-merged <https://github.com/zakarumych/gpu-descriptor/pull/40> to be released.
 gpu-descriptor = { git = "https://github.com/zakarumych/gpu-descriptor", rev = "7b71a4e47c81903ad75e2c53deb5ab1310f6ff4d" }
+
+[patch."https://github.com/mozilla/neqo"]
+neqo-bin = { path = "third_party/rust/neqo-bin" }
+neqo-common = { path = "third_party/rust/neqo-common" }
+neqo-crypto = { path = "third_party/rust/neqo-crypto" }
+neqo-http3 = { path = "third_party/rust/neqo-http3" }
+neqo-qpack = { path = "third_party/rust/neqo-qpack" }
+neqo-transport = { path = "third_party/rust/neqo-transport" }

third_party/rust/neqo-transport/src/connection/mod.rs

@@ -1600,6 +1600,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[..]);
@@ -1612,7 +1623,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))?;
             }
         }
@@ -2712,6 +2730,7 @@ impl Connection {
         packet_version: Version,
         packet_type: PacketType,
         frame: Frame,
+        next_pn: PacketNumber,
         now: Instant,
     ) -> Res<()> {
         if !frame.is_allowed(packet_type) {
@@ -2746,16 +2765,17 @@ impl Connection {
                 ack_ranges,
                 ecn_count,
             } => {
+                // 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(
+                self.handle_ack(space, ranges, ecn_count, ack_delay, now);
-                    space,
-                    largest_acknowledged,
-                    ranges,
-                    ecn_count,
-                    ack_delay,
-                    now,
-                );
             }
             Frame::Crypto { offset, data } => {
                 qtrace!(
@@ -2929,7 +2949,6 @@ impl Connection {
     fn handle_ack<R>(
         &mut self,
         space: PacketNumberSpace,
-        largest_acknowledged: PacketNumber,
         ack_ranges: R,
         ack_ecn: Option<EcnCount>,
         ack_delay: u64,
@@ -2946,12 +2965,12 @@ impl Connection {
         let (acked_packets, lost_packets) = self.loss_recovery.on_ack_received(
             &path,
             space,
-            largest_acknowledged,
             ack_ranges,
             ack_ecn,
             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 {
@@ -2975,7 +2994,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.

third_party/rust/neqo-transport/src/connection/tests/recovery.rs

@@ -802,3 +802,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),
+            ..
+        }
+    ));
+}

third_party/rust/neqo-transport/src/recovery/mod.rs

@@ -587,7 +587,6 @@ impl LossRecovery {
         &mut self,
         primary_path: &PathRef,
         pn_space: PacketNumberSpace,
-        largest_acked: PacketNumber,
         acked_ranges: R,
         ack_ecn: Option<EcnCount>,
         ack_delay: Duration,
@@ -597,13 +596,6 @@ impl LossRecovery {
         R: IntoIterator<Item = RangeInclusive<PacketNumber>>,
         R::IntoIter: ExactSizeIterator,
     {
-        qdebug!(
-            [self],
-            "ACK for {} - largest_acked={}.",
-            pn_space,
-            largest_acked
-        );
-
         let Some(space) = self.spaces.get_mut(pn_space) else {
             qinfo!("ACK on discarded space");
             return (Vec::new(), Vec::new());
@@ -618,8 +610,8 @@ impl LossRecovery {
 
         // Track largest PN acked per space
         let prev_largest_acked = space.largest_acked_sent_time;
-        if Some(largest_acked) > space.largest_acked {
+        if Some(largest_acked_pkt.pn()) > space.largest_acked {
-            space.largest_acked = Some(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)
@@ -634,6 +626,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
@@ -978,21 +977,13 @@ mod tests {
         pub fn on_ack_received(
             &mut self,
             pn_space: PacketNumberSpace,
-            largest_acked: PacketNumber,
             acked_ranges: Vec<RangeInclusive<PacketNumber>>,
             ack_ecn: Option<EcnCount>,
             ack_delay: Duration,
             now: Instant,
         ) -> (Vec<SentPacket>, Vec<SentPacket>) {
-            self.lr.on_ack_received(
+            self.lr
-                &self.path,
+                .on_ack_received(&self.path, pn_space, acked_ranges, ack_ecn, ack_delay, now)
-                pn_space,
-                largest_acked,
-                acked_ranges,
-                ack_ecn,
-                ack_delay,
-                now,
-            )
         }
 
         pub fn on_packet_sent(&mut self, sent_packet: SentPacket) {
@@ -1144,7 +1135,6 @@ mod tests {
     fn ack(lr: &mut Fixture, pn: u64, delay: Duration) {
         lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            pn,
             vec![pn..=pn],
             None,
             ACK_DELAY,
@@ -1298,7 +1288,6 @@ mod tests {
         ));
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            1,
             vec![1..=1],
             None,
             ACK_DELAY,
@@ -1322,7 +1311,6 @@ mod tests {
 
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            2,
             vec![2..=2],
             None,
             ACK_DELAY,
@@ -1352,7 +1340,6 @@ mod tests {
         assert_eq!(super::PACKET_THRESHOLD, 3);
         let (_, lost) = lr.on_ack_received(
             PacketNumberSpace::ApplicationData,
-            4,
             vec![2..=4],
             None,
             ACK_DELAY,
@@ -1381,7 +1368,6 @@ mod tests {
         lr.discard(PacketNumberSpace::Initial, now());
         let (acked, lost) = lr.on_ack_received(
             PacketNumberSpace::Initial,
-            0,
             vec![],
             None,
             Duration::from_millis(0),
@@ -1441,7 +1427,6 @@ mod tests {
             lr.on_packet_sent(sent_pkt);
             lr.on_ack_received(
                 pn_space,
-                1,
                 vec![1..=1],
                 None,
                 Duration::from_secs(0),
@@ -1494,7 +1479,6 @@ mod tests {
         let rtt = lr.path.borrow().rtt().estimate();
         lr.on_ack_received(
             PacketNumberSpace::Initial,
-            0,
             vec![0..=0],
             None,
             Duration::new(0, 0),