fix(quinn-proto): Avoid underflow panic in packet loss Instant calculations

Author: matheus23

quinn-proto/src/connection/mod.rs

@@ -1523,7 +1523,7 @@ impl Connection {
                     Duration::from_micros(ack.delay << self.peer_params.ack_delay_exponent.0),
                 )
             };
-            let rtt = instant_saturating_sub(now, self.spaces[space].largest_acked_packet_sent);
+            let rtt = now.saturating_duration_since(self.spaces[space].largest_acked_packet_sent);
             self.path.rtt.update(ack_delay, rtt);
             if self.path.first_packet_after_rtt_sample.is_none() {
                 self.path.first_packet_after_rtt_sample =
@@ -1712,8 +1712,6 @@ impl Connection {
         let rtt = self.path.rtt.conservative();
         let loss_delay = cmp::max(rtt.mul_f32(self.config.time_threshold), TIMER_GRANULARITY);
 
-        // Packets sent before this time are deemed lost.
-        let lost_send_time = now.checked_sub(loss_delay).unwrap();
         let largest_acked_packet = self.spaces[pn_space].largest_acked_packet.unwrap();
         let packet_threshold = self.config.packet_threshold as u64;
         let mut size_of_lost_packets = 0u64;
@@ -1737,8 +1735,11 @@ impl Connection {
                 persistent_congestion_start = None;
             }
 
-            if info.time_sent <= lost_send_time || largest_acked_packet >= packet + packet_threshold
-            {
+            // Packets sent before now - loss_delay are deemed lost.
+            // However, we avoid this subtraction as it can panic and there's no
+            // saturating equivalent of this substraction operation with a Duration.
+            let packet_too_old = now.saturating_duration_since(info.time_sent) >= loss_delay;
+            if packet_too_old || largest_acked_packet >= packet + packet_threshold {
                 if Some(packet) == in_flight_mtu_probe {
                     // Lost MTU probes are not included in `lost_packets`, because they should not
                     // trigger a congestion control response
@@ -1791,6 +1792,17 @@ impl Connection {
                 lost_packets, size_of_lost_packets
             );
 
+            // Packets sent before this time are deemed lost.
+            // We avoid computing this value above, since it's possible for this to panic
+            // if the `loss_delay` value internally stores a bigger `Duration` than the
+            // `Duration` that's stored inside the `Instant`, because some platforms may
+            // implement the `Instant` with a counter relative to system or even process
+            // startup (Wasm is one such case).
+            // If we're at this point, then it must be possible to have instants that are
+            // longer ago than `loss_delay` (see the `packet_too_old` computation
+            // above).
+            let lost_send_time = now.checked_sub(loss_delay).unwrap();
+
             for &packet in &lost_packets {
                 let info = self.spaces[pn_space].take(packet).unwrap(); // safe: lost_packets is populated just above
                 self.config.qlog_sink.emit_packet_lost(
@@ -4056,10 +4068,6 @@ pub enum Event {
     DatagramsUnblocked,
 }
 
-fn instant_saturating_sub(x: Instant, y: Instant) -> Duration {
-    if x > y { x - y } else { Duration::ZERO }
-}
-
 fn get_max_ack_delay(params: &TransportParameters) -> Duration {
     Duration::from_micros(params.max_ack_delay.0 * 1000)
 }