Allow timestamps for reordered packets in IETF QUIC. Almost entirely reverts cl/424344189. PiperOrigin-RevId: 933898743
diff --git a/quiche/quic/core/frames/quic_ack_frame.h b/quiche/quic/core/frames/quic_ack_frame.h index d4ccb66..b674fcd 100644 --- a/quiche/quic/core/frames/quic_ack_frame.h +++ b/quiche/quic/core/frames/quic_ack_frame.h
@@ -109,9 +109,9 @@ // sent. QuicTime::Delta ack_delay_time = QuicTime::Delta::Infinite(); - // Vector of <packet_number, time> for when packets arrived. - // For IETF versions, packet numbers and timestamps in this vector are both in - // ascending orders. Packets received out of order are not saved here. + // Vector of <packet_number, time> for when packets arrived. Outgoing + // timestamps are stored in the ascending time order; the incoming timestamps + // are stored in the descending order. PacketTimeVector received_packet_times; // Set of packets.
diff --git a/quiche/quic/core/quic_framer.cc b/quiche/quic/core/quic_framer.cc index cf184f6..a002fb8 100644 --- a/quiche/quic/core/quic_framer.cc +++ b/quiche/quic/core/quic_framer.cc
@@ -5626,10 +5626,9 @@ QUIC_DVLOG(3) << "prev_packet_number:" << prev_packet_number << ", packet_number:" << packet_number; - if (prev_receive_timestamp < receive_timestamp || - prev_packet_number <= packet_number) { - detailed_error = "Packet number and/or receive time not in order."; - QUIC_BUG(quic_framer_ack_ts_packet_out_of_order) + if (prev_receive_timestamp < receive_timestamp) { + detailed_error = "Receive time not in order."; + QUIC_BUG(quic_framer_ack_ts_time_out_of_order) << detailed_error << " packet_number:" << packet_number << ", receive_timestamp:" << receive_timestamp << ", prev_packet_number:" << prev_packet_number
diff --git a/quiche/quic/core/quic_framer_test.cc b/quiche/quic/core/quic_framer_test.cc index f342eff..037e86e 100644 --- a/quiche/quic/core/quic_framer_test.cc +++ b/quiche/quic/core/quic_framer_test.cc
@@ -6646,6 +6646,108 @@ ABSL_ARRAYSIZE(packet_ietf)); } +TEST_P(QuicFramerTest, BuildAckReceiveTimestampsFramePacketOutOfOrder) { + if (!VersionIsIetfQuic(framer_.transport_version())) { + return; + } + + QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT); + QuicPacketHeader header; + header.destination_connection_id = FramerTestConnectionId(); + header.reset_flag = false; + header.version_flag = false; + header.packet_number = kPacketNumber; + + QuicAckFrame ack_frame = InitAckFrame(kSmallLargestObserved); + ack_frame.received_packet_times = PacketTimeVector{ + {kSmallLargestObserved - 4, CreationTimePlus(0x29ffdedd)}, + {kSmallLargestObserved - 1, CreationTimePlus(0x29ffdedd + 0x10)}, + {kSmallLargestObserved - 3, CreationTimePlus(0x29ffeedd + 0x10)}, + {kSmallLargestObserved - 2, CreationTimePlus(0x29ffeedd + 0x11)}, + }; + ack_frame.ack_delay_time = QuicTime::Delta::Zero(); + QuicFrames frames = {QuicFrame(&ack_frame)}; + + unsigned char packet_ietf[] = { + // type (short header, 4 byte packet number) + 0x43, + // connection_id + 0xFE, + 0xDC, + 0xBA, + 0x98, + 0x76, + 0x54, + 0x32, + 0x10, + // packet number + 0x12, + 0x34, + 0x56, + 0x78, + + // frame type (IETF_ACK_RECEIVE_TIMESTAMPS frame) + 0x83, + 0x17, + 0x83, + 0x07, + // largest acked + kVarInt62TwoBytes + 0x12, + 0x34, // = 4660 + // Zero delta time. + kVarInt62OneByte + 0x00, + // Number of additional ack blocks. + kVarInt62OneByte + 0x00, + // First ack block length. + kVarInt62TwoBytes + 0x12, + 0x33, + + // Receive Timestamps. + + // Timestamp Range Count + kVarInt62OneByte + 0x03, + + // Timestamp range 1 (two packets). + // Delta Largest Acknowledged + kVarInt62OneByte + 0x02, + // Timestamp Range Count + kVarInt62OneByte + 0x02, + // Timestamp Delta + kVarInt62FourBytes + 0x29, + 0xff, + 0xee, + 0xee, + // Timestamp Delta + kVarInt62OneByte + 0x01, + + // Timestamp range 2 (one packet). + // Delta Largest Acknowledged + kVarInt62OneByte + 0x01, + // Timestamp Range Count + kVarInt62OneByte + 0x01, + // Timestamp Delta + kVarInt62TwoBytes + 0x10, + 0x00, + + // Timestamp range 3 (one packet). + // Delta Largest Acknowledged + kVarInt62OneByte + 0x04, + // Timestamp Range Count + kVarInt62OneByte + 0x01, + // Timestamp Delta + kVarInt62OneByte + 0x10, + }; + // clang-format on + + framer_.set_process_timestamps(true); + framer_.set_max_receive_timestamps_per_ack(8); + std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); + ASSERT_TRUE(data != nullptr); + quiche::test::CompareCharArraysWithHexError( + "constructed packet", data->data(), data->length(), AsChars(packet_ietf), + ABSL_ARRAYSIZE(packet_ietf)); +} + TEST_P(QuicFramerTest, BuildAckReceiveTimestampsAndEcnFrame) { if (!VersionIsIetfQuic(framer_.transport_version())) { return; @@ -7201,7 +7303,53 @@ })); } -TEST_P(QuicFramerTest, AckReceiveTimestampsPacketOutOfOrder) { +TEST_P(QuicFramerTest, BuildAndProcessAckReceiveTimestampsPacketOutOfOrder) { + if (!VersionIsIetfQuic(framer_.transport_version())) { + return; + } + framer_.InstallDecrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<StrictTaggingDecrypter>(/*key=*/0)); + framer_.SetKeyUpdateSupportForConnection(true); + framer_.set_process_timestamps(true); + framer_.set_max_receive_timestamps_per_ack(8); + + QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_CLIENT); + QuicPacketHeader header; + header.destination_connection_id = FramerTestConnectionId(); + header.reset_flag = false; + header.version_flag = false; + header.packet_number = kPacketNumber; + + QuicAckFrame ack_frame = InitAckFrame(kSmallLargestObserved); + ack_frame.received_packet_times = PacketTimeVector{ + {kSmallLargestObserved - 4, CreationTimePlus(0x29ffdedd)}, + {kSmallLargestObserved - 1, CreationTimePlus(0x29ffdeed)}, + {kSmallLargestObserved - 3, CreationTimePlus(0x29ffeeed)}, + {kSmallLargestObserved - 2, CreationTimePlus(0x29ffeeee)}, + }; + ack_frame.ack_delay_time = QuicTime::Delta::Zero(); + QuicFrames frames = {QuicFrame(&ack_frame)}; + + std::unique_ptr<QuicPacket> data(BuildDataPacket(header, frames)); + ASSERT_TRUE(data != nullptr); + std::unique_ptr<QuicEncryptedPacket> encrypted( + EncryptPacketWithTagAndPhase(*data, 0, false)); + ASSERT_TRUE(encrypted); + QuicFramerPeer::SetPerspective(&framer_, Perspective::IS_SERVER); + EXPECT_TRUE(framer_.ProcessPacket(*encrypted)); + EXPECT_THAT(framer_.error(), IsQuicNoError()); + + const QuicAckFrame& frame = *visitor_.ack_frames_[0]; + EXPECT_THAT(frame.received_packet_times, + ContainerEq(PacketTimeVector{ + {kSmallLargestObserved - 2, CreationTimePlus(0x29ffeeee)}, + {kSmallLargestObserved - 3, CreationTimePlus(0x29ffeeed)}, + {kSmallLargestObserved - 1, CreationTimePlus(0x29ffdeed)}, + {kSmallLargestObserved - 4, CreationTimePlus(0x29ffdedd)}, + })); +} + +TEST_P(QuicFramerTest, AckReceiveTimestampsTimeOutOfOrder) { if (!VersionIsIetfQuic(framer_.transport_version())) { return; } @@ -7219,23 +7367,17 @@ header.version_flag = false; header.packet_number = kPacketNumber; - // Use kSmallLargestObserved to make this test finished in a short time. QuicAckFrame ack_frame = InitAckFrame(kSmallLargestObserved); - // The packet numbers below are out of order, this is impossible because we - // don't record out of order packets in received_packet_times. The test is - // intended to ensure this error is raised when it happens. ack_frame.received_packet_times = PacketTimeVector{ - {kSmallLargestObserved - 5, CreationTimePlus((0x29ff << 3))}, - {kSmallLargestObserved - 2, CreationTimePlus((0x29ff << 3))}, - {kSmallLargestObserved - 4, CreationTimePlus((0x29ff << 3))}, {kSmallLargestObserved - 3, CreationTimePlus((0x29ff << 3))}, + {kSmallLargestObserved - 2, CreationTimePlus((0x29ff << 3) - 1)}, }; ack_frame.ack_delay_time = QuicTime::Delta::Zero(); QuicFrames frames = {QuicFrame(&ack_frame)}; EXPECT_QUIC_BUG(BuildDataPacket(header, frames), - "Packet number and/or receive time not in order."); + "Receive time not in order."); } TEST_P(QuicFramerTest, ProcessIetfAckReceiveTimestampsExceedsMaxTimestamps) {
diff --git a/quiche/quic/core/quic_received_packet_manager.cc b/quiche/quic/core/quic_received_packet_manager.cc index d0efc44..408d878 100644 --- a/quiche/quic/core/quic_received_packet_manager.cc +++ b/quiche/quic/core/quic_received_packet_manager.cc
@@ -45,7 +45,6 @@ max_ack_ranges_(0), time_largest_observed_(QuicTime::Zero()), save_timestamps_(false), - save_timestamps_for_in_order_packets_(false), stats_(stats), num_retransmittable_packets_received_since_last_ack_sent_(0), min_received_before_ack_decimation_(kMinReceivedBeforeAckDecimation), @@ -86,12 +85,9 @@ ack_frame_updated_ = true; ack_now_ = false; - // Whether |packet_number| is received out of order. - bool packet_reordered = false; if (LargestAcked(ack_frame_).IsInitialized() && LargestAcked(ack_frame_) > packet_number) { // Record how out of order stats. - packet_reordered = true; ++stats_->packets_reordered; stats_->max_sequence_reordering = std::max(stats_->max_sequence_reordering, @@ -110,12 +106,10 @@ MaybeTrimAckRanges(); if (save_timestamps_) { - // The timestamp format only handles packets in time order. - if (save_timestamps_for_in_order_packets_ && packet_reordered) { - QUIC_DLOG(WARNING) << "Not saving receive timestamp for packet " - << packet_number; - } else if (!ack_frame_.received_packet_times.empty() && - ack_frame_.received_packet_times.back().second > receipt_time) { + // The QUIC framer can only serialize timestamps if they are provided in the + // receive time order. + if (!ack_frame_.received_packet_times.empty() && + ack_frame_.received_packet_times.back().second > receipt_time) { QUIC_LOG(WARNING) << "Receive time went backwards from: " << ack_frame_.received_packet_times.back().second.ToDebuggingValue()
diff --git a/quiche/quic/core/quic_received_packet_manager.h b/quiche/quic/core/quic_received_packet_manager.h index 78942d0..b5d8f94 100644 --- a/quiche/quic/core/quic_received_packet_manager.h +++ b/quiche/quic/core/quic_received_packet_manager.h
@@ -116,9 +116,8 @@ max_ack_ranges_ = max_ack_ranges; } - void set_save_timestamps(bool save_timestamps, bool in_order_packets_only) { + void set_save_timestamps(bool save_timestamps) { save_timestamps_ = save_timestamps; - save_timestamps_for_in_order_packets_ = in_order_packets_only; } size_t min_received_before_ack_decimation() const { @@ -183,10 +182,6 @@ // If true, save timestamps in the ack_frame_. bool save_timestamps_; - // If true and |save_timestamps_|, only save timestamps for packets that are - // received in order. - bool save_timestamps_for_in_order_packets_; - // Least packet number received from peer. QuicPacketNumber least_received_packet_number_;
diff --git a/quiche/quic/core/quic_received_packet_manager_test.cc b/quiche/quic/core/quic_received_packet_manager_test.cc index 8df2781..7616590 100644 --- a/quiche/quic/core/quic_received_packet_manager_test.cc +++ b/quiche/quic/core/quic_received_packet_manager_test.cc
@@ -47,7 +47,7 @@ QuicReceivedPacketManagerTest() : received_manager_(&stats_) { clock_.AdvanceTime(QuicTime::Delta::FromSeconds(1)); rtt_stats_.UpdateRtt(kMinRttMs, QuicTime::Delta::Zero(), QuicTime::Zero()); - received_manager_.set_save_timestamps(true, false); + received_manager_.set_save_timestamps(true); } void RecordPacketReceipt(uint64_t packet_number) { @@ -210,8 +210,8 @@ EXPECT_EQ(2u, received_manager_.ack_frame().received_packet_times.size()); } -TEST_F(QuicReceivedPacketManagerTest, IgnoreOutOfOrderPackets) { - received_manager_.set_save_timestamps(true, true); +TEST_F(QuicReceivedPacketManagerTest, SaveOutOfOrderPackets) { + received_manager_.set_save_timestamps(true); EXPECT_FALSE(received_manager_.ack_frame_updated()); RecordPacketReceipt(1, QuicTime::Zero()); EXPECT_TRUE(received_manager_.ack_frame_updated()); @@ -222,7 +222,7 @@ RecordPacketReceipt(3, QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(3)); - EXPECT_EQ(2u, received_manager_.ack_frame().received_packet_times.size()); + EXPECT_EQ(3u, received_manager_.ack_frame().received_packet_times.size()); } TEST_F(QuicReceivedPacketManagerTest, HasMissingPackets) {
diff --git a/quiche/quic/core/uber_received_packet_manager.cc b/quiche/quic/core/uber_received_packet_manager.cc index 2f64754..cae2766 100644 --- a/quiche/quic/core/uber_received_packet_manager.cc +++ b/quiche/quic/core/uber_received_packet_manager.cc
@@ -233,8 +233,7 @@ void UberReceivedPacketManager::set_save_timestamps(bool save_timestamps) { for (auto& received_packet_manager : received_packet_managers_) { - received_packet_manager.set_save_timestamps( - save_timestamps, supports_multiple_packet_number_spaces_); + received_packet_manager.set_save_timestamps(save_timestamps); } }