Ensure QUIC only sends ACK timestamps for actually acked packets Section 4.1 of the draft says "the packet number MUST have been included in an ACK Range in the current or a previously sent ACK". While this is impossible to enforce in practice, we should not send ACKs for packets that got truncated from the ACK ranges regardless. This CL also adds a QUIC_BUG if any of the packets in the timestamps list are not in the list of packets actually being acknowledged (this should be always true due to the newly-added pruning code). PiperOrigin-RevId: 987232013
diff --git a/quiche/quic/core/quic_ack_timestamp_list.cc b/quiche/quic/core/quic_ack_timestamp_list.cc index 026deaf..ff178a0 100644 --- a/quiche/quic/core/quic_ack_timestamp_list.cc +++ b/quiche/quic/core/quic_ack_timestamp_list.cc
@@ -14,6 +14,7 @@ #include "quiche/quic/core/quic_packet_number.h" #include "quiche/quic/core/quic_time.h" #include "quiche/quic/core/quic_types.h" +#include "quiche/quic/platform/api/quic_bug_tracker.h" #include "quiche/quic/platform/api/quic_logging.h" #include "quiche/common/platform/api/quiche_logging.h" @@ -88,6 +89,19 @@ return; } + // There is nothing in the logic below that depends on `ack.packets` + // actually containing the packet in question; furthermore, while the + // specification requires timestamped packets to be acknowledged, they don't + // have to be acknowledged in the same ACK packet, and due to memory + // limitations and reordering, this requirement is effectively not + // enforceable by the receiver of the ACK. + // Because of that, we merely log a bug instead of aborting serialization as + // with the preconditions above. + QUIC_BUG_IF(quic_ack_timestamp_not_in_ack_frame, + !ack.packets.Contains(packet_number)) + << "Sending receive timestamp for unacknowledged packet " + << packet_number; + bool should_open_new_range; if (i >= 1) { QUICHE_DCHECK(effective_previous_time.IsInitialized());
diff --git a/quiche/quic/core/quic_ack_timestamp_list_test.cc b/quiche/quic/core/quic_ack_timestamp_list_test.cc index ef89df9..bc1ab42 100644 --- a/quiche/quic/core/quic_ack_timestamp_list_test.cc +++ b/quiche/quic/core/quic_ack_timestamp_list_test.cc
@@ -14,6 +14,7 @@ #include "quiche/quic/core/quic_packet_number.h" #include "quiche/quic/core/quic_time.h" #include "quiche/quic/core/quic_types.h" +#include "quiche/quic/platform/api/quic_expect_bug.h" #include "quiche/common/platform/api/quiche_test.h" #include "quiche/common/quiche_text_utils.h" @@ -23,7 +24,10 @@ class QuicAckTimestampListTest : public quiche::test::QuicheTest { protected: QuicAckTimestampListTest() - : basis_(QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(1000)) {} + : basis_(QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(1000)) { + // Packets numbers higher than 1000 will trigger a QUIC_BUG. + ack_.packets.AddRange(QuicPacketNumber(1), QuicPacketNumber(1001)); + } QuicTime TimePlus(uint64_t offset_us) { return basis_ + QuicTime::Delta::FromMicroseconds(offset_us); @@ -376,5 +380,25 @@ } } +TEST_F(QuicAckTimestampListTest, UnacknowledgedPacketTriggersQuicBug) { + ack_.largest_acked = QuicPacketNumber(100); + ack_.packets.Clear(); + ack_.packets.AddRange(QuicPacketNumber(99), QuicPacketNumber(101)); + ack_.received_packet_times = { + {QuicPacketNumber(90), TimePlus(5)}, + {QuicPacketNumber(99), TimePlus(10)}, + {QuicPacketNumber(100), TimePlus(20)}, + }; + + EXPECT_QUIC_BUG( + { + QuicAckTimestampList list(ack_, /*max_ack_count=*/10, /*exponent=*/0, + basis_); + EXPECT_EQ(list.error(), ""); + ExpectBinaryEncoding(list, "020002140a0a0105"); + }, + "Sending receive timestamp for unacknowledged packet 90"); +} + } // namespace } // namespace quic::test
diff --git a/quiche/quic/core/quic_framer_test.cc b/quiche/quic/core/quic_framer_test.cc index 4bafd02..6e08dbc 100644 --- a/quiche/quic/core/quic_framer_test.cc +++ b/quiche/quic/core/quic_framer_test.cc
@@ -7608,7 +7608,7 @@ /*max_num_gaps=*/200, /*largest_acked=*/kMaxIetfVarInt); ack_frame.received_packet_times = PacketTimeVector{ - {QuicPacketNumber(kMaxIetfVarInt) - 2, CreationTimePlus((0x29ff << 3))}, + {QuicPacketNumber(kMaxIetfVarInt), CreationTimePlus((0x29ff << 3))}, }; QuicFrames frames = {QuicFrame(&ack_frame)}; // Build an ACK packet.
diff --git a/quiche/quic/core/quic_received_packet_manager.cc b/quiche/quic/core/quic_received_packet_manager.cc index e56ece2..6d23fe7 100644 --- a/quiche/quic/core/quic_received_packet_manager.cc +++ b/quiche/quic/core/quic_received_packet_manager.cc
@@ -8,6 +8,7 @@ #include <limits> #include <optional> #include <utility> +#include <vector> #include "quiche/quic/core/congestion_control/rtt_stats.h" #include "quiche/quic/core/crypto/crypto_protocol.h" @@ -197,6 +198,14 @@ ack_frame_.packets.RemoveSmallestInterval(); } + // Remove timestamps for packets that got removed from the packet ranges. + if (!ack_frame_.received_packet_times.empty()) { + std::erase_if(ack_frame_.received_packet_times, + [&](const std::pair<QuicPacketNumber, QuicTime>& entry) { + return !ack_frame_.packets.Contains(entry.first); + }); + } + #if QUIC_FRAME_DEBUG QuicFrame frame = QuicFrame(&ack_frame_); frame.delete_forbidden = true;
diff --git a/quiche/quic/core/quic_received_packet_manager_test.cc b/quiche/quic/core/quic_received_packet_manager_test.cc index 6014f3e..bfb30ba 100644 --- a/quiche/quic/core/quic_received_packet_manager_test.cc +++ b/quiche/quic/core/quic_received_packet_manager_test.cc
@@ -198,6 +198,45 @@ } } +TEST_F(QuicReceivedPacketManagerTest, TrimmedAckRangesRemovedFromTimestamps) { + received_manager_.set_save_timestamps(true); + received_manager_.set_max_ack_ranges(2); + + RecordPacketReceipt(3, + QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(1)); + RecordPacketReceipt(5, + QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(2)); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + EXPECT_EQ(2u, received_manager_.ack_frame().packets.NumIntervals()); + EXPECT_EQ(2u, received_manager_.ack_frame().received_packet_times.size()); + + // Receiving packet 7 creates a 3rd interval, trimming interval [3, 4) and + // pruning packet 3's timestamp. + RecordPacketReceipt(7, + QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(3)); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + EXPECT_EQ(2u, received_manager_.ack_frame().packets.NumIntervals()); + EXPECT_EQ(QuicPacketNumber(5), received_manager_.ack_frame().packets.Min()); + ASSERT_EQ(2u, received_manager_.ack_frame().received_packet_times.size()); + EXPECT_EQ(QuicPacketNumber(5), + received_manager_.ack_frame().received_packet_times[0].first); + EXPECT_EQ(QuicPacketNumber(7), + received_manager_.ack_frame().received_packet_times[1].first); + + // Receiving an out-of-order packet 1 creates a new smallest interval [1, 2) + // that is immediately trimmed along with its timestamp. + RecordPacketReceipt(1, + QuicTime::Zero() + QuicTime::Delta::FromMilliseconds(4)); + received_manager_.GetUpdatedAckFrame(QuicTime::Zero()); + EXPECT_EQ(2u, received_manager_.ack_frame().packets.NumIntervals()); + EXPECT_EQ(QuicPacketNumber(5), received_manager_.ack_frame().packets.Min()); + ASSERT_EQ(2u, received_manager_.ack_frame().received_packet_times.size()); + EXPECT_EQ(QuicPacketNumber(5), + received_manager_.ack_frame().received_packet_times[0].first); + EXPECT_EQ(QuicPacketNumber(7), + received_manager_.ack_frame().received_packet_times[1].first); +} + TEST_F(QuicReceivedPacketManagerTest, IgnoreOutOfOrderTimestamps) { EXPECT_FALSE(received_manager_.ack_frame_updated()); RecordPacketReceipt(1, QuicTime::Zero());