Fix a QUIC off-by-one error in the calculation of peer_least_packet_awaiting_ack.
See bug description for details.
Protected by FLAGS_quic_reloadable_flag_quic_least_unacked_plus_1.
PiperOrigin-RevId: 772481514
diff --git a/quiche/common/quiche_feature_flags_list.h b/quiche/common/quiche_feature_flags_list.h
index 20b927c..12ffbdd 100755
--- a/quiche/common/quiche_feature_flags_list.h
+++ b/quiche/common/quiche_feature_flags_list.h
@@ -41,6 +41,7 @@
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_heapless_obfuscator, true, true, "If true, generates QUIC initial obfuscators with no heap allocations.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_heapless_static_parser, true, true, "If true, stops parsing immediately on unknown version, to avoid a potential malloc when parsing the connection ID")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_ignore_gquic_probing, true, true, "If true, QUIC server will not respond to gQUIC probing packet(PING + PADDING) but treat it as a regular packet.")
+QUICHE_FLAG(bool, quiche_reloadable_flag_quic_least_unacked_plus_1, false, false, "If true, sets peer_least_packet_awaiting_ack to one more than the highest confirmed acknowledgment to fix an off-by-one error.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_limit_new_streams_per_loop_2, true, true, "If true, when the peer sends connection options \\\'SLP1\\\', \\\'SLP2\\\' and \\\'SLPF\\\', internet facing GFEs will only allow a limited number of new requests to be processed per event loop, and postpone the rest to the following event loops. Also guard QuicConnection to iterate through all decrypters at each encryption level to get cipher id for a request.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_no_path_degrading_before_handshake_confirmed, true, true, "If true, an endpoint does not detect path degrading or blackholing until handshake gets confirmed.")
QUICHE_FLAG(bool, quiche_reloadable_flag_quic_no_write_control_frame_upon_connection_close, false, true, "If trrue, early return before write control frame in OnCanWrite() if the connection is already closed.")
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index 053138c..f52a3f6 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -269,7 +269,8 @@
QuicAlarmProxy(&alarms_, QuicAlarmSlot::kPing)),
multi_port_probing_interval_(kDefaultMultiPortProbingInterval),
connection_id_generator_(generator),
- received_client_addresses_cache_(kMaxReceivedClientAddressSize) {
+ received_client_addresses_cache_(kMaxReceivedClientAddressSize),
+ least_unacked_plus_1_(GetQuicReloadableFlag(quic_least_unacked_plus_1)) {
QUICHE_DCHECK(perspective_ == Perspective::IS_CLIENT ||
default_path_.self_address.IsInitialized());
@@ -5808,12 +5809,27 @@
void QuicConnection::PostProcessAfterAckFrame(bool acked_new_packet) {
if (!packet_creator_.has_ack()) {
- uber_received_packet_manager_.DontWaitForPacketsBefore(
- last_received_packet_info_.decrypted_level,
- SupportsMultiplePacketNumberSpaces()
- ? sent_packet_manager_.GetLargestPacketPeerKnowsIsAcked(
- last_received_packet_info_.decrypted_level)
- : sent_packet_manager_.largest_packet_peer_knows_is_acked());
+ if (least_unacked_plus_1_) {
+ QuicPacketNumber largest_packet_peer_knows_is_acked =
+ SupportsMultiplePacketNumberSpaces()
+ ? sent_packet_manager_.GetLargestPacketPeerKnowsIsAcked(
+ last_received_packet_info_.decrypted_level)
+ : sent_packet_manager_.largest_packet_peer_knows_is_acked();
+ if (largest_packet_peer_knows_is_acked.IsInitialized()) {
+ QUIC_RELOADABLE_FLAG_COUNT(quic_least_unacked_plus_1);
+ ++largest_packet_peer_knows_is_acked;
+ }
+ uber_received_packet_manager_.DontWaitForPacketsBefore(
+ last_received_packet_info_.decrypted_level,
+ largest_packet_peer_knows_is_acked);
+ } else {
+ uber_received_packet_manager_.DontWaitForPacketsBefore(
+ last_received_packet_info_.decrypted_level,
+ SupportsMultiplePacketNumberSpaces()
+ ? sent_packet_manager_.GetLargestPacketPeerKnowsIsAcked(
+ last_received_packet_info_.decrypted_level)
+ : sent_packet_manager_.largest_packet_peer_knows_is_acked());
+ }
}
// Always reset the retransmission alarm when an ack comes in, since we now
// have a better estimate of the current rtt than when it was set.
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h
index 3d9e734..83646d4 100644
--- a/quiche/quic/core/quic_connection.h
+++ b/quiche/quic/core/quic_connection.h
@@ -2630,7 +2630,8 @@
// If true then flow labels will be changed when a PTO fires, or when
// a PTO'd packet from a peer is detected.
bool enable_black_hole_avoidance_via_flow_label_ = false;
-
+ // If true, fixes a off-by-one error in the least unacked packet calculation.
+ bool least_unacked_plus_1_;
const bool quic_limit_new_streams_per_loop_2_ =
GetQuicReloadableFlag(quic_limit_new_streams_per_loop_2);
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index c28a3c7..c0251ab 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -882,7 +882,7 @@
if (connection_.version().KnowsWhichDecrypterToUse()) {
connection_.InstallDecrypter(
level, std::make_unique<StrictTaggingDecrypter>(level));
- } else {
+ } else if (level != connection_.last_decrypted_level()) {
connection_.SetAlternativeDecrypter(
level, std::make_unique<StrictTaggingDecrypter>(level), false);
}
@@ -17919,6 +17919,50 @@
EXPECT_TRUE(QuicConnectionPeer::CanReceiveAckFrequencyFrames(&connection_));
}
+// Regression test for b/424538505.
+TEST_P(QuicConnectionTest, LeastUnackedOffByOne) {
+ QuicPacketNumber largest_packet_sent;
+ EXPECT_CALL(connection_, OnSerializedPacket)
+ .WillOnce(Invoke([&](SerializedPacket packet) {
+ largest_packet_sent = packet.packet_number;
+ connection_.QuicConnection::OnSerializedPacket(std::move(packet));
+ }));
+ ProcessPacket(1);
+ ProcessPacket(2);
+ EXPECT_TRUE(largest_packet_sent.IsInitialized());
+ const QuicAckFrame& local_ack_frame_1 = writer_->ack_frames()[0];
+ EXPECT_EQ(local_ack_frame_1.largest_acked, QuicPacketNumber(2));
+ EXPECT_EQ(local_ack_frame_1.packets.NumIntervals(), 1);
+
+ QuicAckFrame peer_ack_frame;
+ peer_ack_frame.largest_acked = largest_packet_sent;
+ peer_ack_frame.ack_delay_time = QuicTime::Delta::Zero();
+ peer_ack_frame.packets.Add(largest_packet_sent);
+ QuicFrames peer_frames;
+ // Add a PING frame to make it ack-eliciting.
+ peer_frames.push_back(QuicFrame(QuicPingFrame()));
+ peer_frames.push_back(QuicFrame(&peer_ack_frame));
+ // When connection_ receives packet 4, it includes an ACK of (locally-sent)
+ // packet 1. Packet 1 included an ACK of (locally-received) packets 1 and 2.
+ // When packet 4 is received, the local connection is therefore confident that
+ // the peer knows that both packets 1 & 2 have been ACK'd and so it can stop
+ // ACK'ing them.
+ EXPECT_CALL(*send_algorithm_, OnCongestionEvent);
+ EXPECT_CALL(connection_, OnSerializedPacket);
+ // Create a packet number gap so that there will be two ranges if the first
+ // range is not removed.
+ ProcessFramesPacketAtLevel(4, peer_frames, ENCRYPTION_FORWARD_SECURE);
+ const QuicAckFrame& local_ack_frame_2 = writer_->ack_frames()[0];
+ EXPECT_EQ(local_ack_frame_2.largest_acked, QuicPacketNumber(4));
+ if (GetQuicReloadableFlag(quic_least_unacked_plus_1)) {
+ EXPECT_EQ(local_ack_frame_2.packets.NumIntervals(), 1);
+ EXPECT_EQ(local_ack_frame_2.packets.Min(), QuicPacketNumber(4));
+ } else {
+ EXPECT_EQ(local_ack_frame_2.packets.NumIntervals(), 2);
+ EXPECT_EQ(local_ack_frame_2.packets.Min(), QuicPacketNumber(2));
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
