Make sure MaybeCoalescePacketOfHigherSpace is not re-entrant to avoid infinite loop. Protected by gfe2_reloadable_flag_quic_coalesced_packet_of_higher_space2 which replaces gfe2_reloadable_flag_quic_coalesced_packet_of_higher_space.

PiperOrigin-RevId: 323822656
Change-Id: Ie651e714ced763fb7041d8e3737ae241478691bd

quic/core/quic_connection.cc

diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index f8e3f6e..2fc3098 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2429,6 +2429,13 @@
     return false;
   }
 
+  if (fill_coalesced_packet_) {
+    // Try to coalesce packet, only allow to write when creator is on soft max
+    // packet length. Given the next created packet is going to fill current
+    // coalesced packet, do not check amplification factor.
+    return packet_creator_.HasSoftMaxPacketLength();
+  }
+
   if (LimitedByAmplificationFactor()) {
     // Server is constrained by the amplification restriction.
     QUIC_CODE_COUNT(quic_throttled_by_amplification_limit);
@@ -2437,12 +2444,6 @@
     return false;
   }
 
-  if (fill_coalesced_packet_) {
-    // Try to coalesce packet, only allow to write when creator is on soft max
-    // packet length.
-    return packet_creator_.HasSoftMaxPacketLength();
-  }
-
   if (sent_packet_manager_.pending_timer_transmission_count() > 0) {
     // Force sending the retransmissions for HANDSHAKE, TLP, RTO, PROBING cases.
     return true;
@@ -3676,7 +3677,7 @@
     connection_->packet_creator_.Flush();
     if (connection_->version().CanSendCoalescedPackets()) {
       if (connection_->packet_creator().coalesced_packet_of_higher_space()) {
-        QUIC_RELOADABLE_FLAG_COUNT(quic_coalesced_packet_of_higher_space);
+        QUIC_RELOADABLE_FLAG_COUNT(quic_coalesced_packet_of_higher_space2);
         connection_->MaybeCoalescePacketOfHigherSpace();
       }
       connection_->FlushCoalescedPacket();
@@ -4365,7 +4366,9 @@
 }
 
 void QuicConnection::MaybeCoalescePacketOfHigherSpace() {
-  if (!packet_creator_.HasSoftMaxPacketLength()) {
+  if (!connected() || !packet_creator_.HasSoftMaxPacketLength() ||
+      fill_coalesced_packet_) {
+    // Make sure MaybeCoalescePacketOfHigherSpace is not re-entrant.
     return;
   }
   // INITIAL or HANDSHAKE retransmission could cause peer to derive new
@@ -4389,12 +4392,12 @@
       fill_coalesced_packet_ = true;
       sent_packet_manager_.RetransmitDataOfSpaceIfAny(
           QuicUtils::GetPacketNumberSpace(coalesced_level));
+      fill_coalesced_packet_ = false;
     }
   }
 }
 
 bool QuicConnection::FlushCoalescedPacket() {
-  fill_coalesced_packet_ = false;
   ScopedCoalescedPacketClearer clearer(&coalesced_packet_);
   if (!version().CanSendCoalescedPackets()) {
     QUIC_BUG_IF(coalesced_packet_.length() > 0);

quic/core/quic_connection_test.cc

diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 5db917f..5f1ff0c 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -10444,7 +10444,7 @@
                            _, _));
   EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1);
   connection_.GetRetransmissionAlarm()->Fire();
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     // Verify 1-RTT packet gets coalesced with handshake retransmission.
     EXPECT_EQ(0x01010101u, writer_->final_bytes_of_last_packet());
   } else {
@@ -10463,7 +10463,7 @@
   QuicPacketNumber handshake_retransmission =
       GetQuicReloadableFlag(quic_default_on_pto) ? QuicPacketNumber(5)
                                                  : QuicPacketNumber(7);
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     handshake_retransmission += 1;
     EXPECT_CALL(*send_algorithm_,
                 OnPacketSent(_, _, handshake_retransmission + 1, _, _));
@@ -10472,7 +10472,7 @@
               OnPacketSent(_, _, handshake_retransmission, _, _));
   EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1);
   connection_.GetRetransmissionAlarm()->Fire();
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     // Verify 1-RTT packet gets coalesced with handshake retransmission.
     EXPECT_EQ(0x01010101u, writer_->final_bytes_of_last_packet());
   } else {
@@ -10489,7 +10489,7 @@
   QuicPacketNumber application_retransmission =
       GetQuicReloadableFlag(quic_default_on_pto) ? QuicPacketNumber(6)
                                                  : QuicPacketNumber(9);
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     application_retransmission += 2;
   }
   EXPECT_CALL(*send_algorithm_,
@@ -11268,7 +11268,7 @@
   connection_.SetEncrypter(ENCRYPTION_HANDSHAKE,
                            std::make_unique<TaggingEncrypter>(0x02));
   connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(2);
   } else {
     EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1);
@@ -11445,7 +11445,7 @@
   connection_.SetEncrypter(ENCRYPTION_HANDSHAKE,
                            std::make_unique<TaggingEncrypter>(0x02));
   connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     // Verify HANDSHAKE packet is coalesced with INITIAL retransmission.
     EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(2);
   } else {
@@ -11475,7 +11475,7 @@
   // Because retransmitted INITIAL gets received so HANDSHAKE 2 gets processed.
   frames.clear();
   QuicAckFrame ack_frame2;
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     // HANDSHAKE 5 is also processed.
     ack_frame2 = InitAckFrame(
         {{QuicPacketNumber(2), QuicPacketNumber(3)},
@@ -11486,7 +11486,7 @@
   ack_frame2.ack_delay_time = QuicTime::Delta::Zero();
   frames.push_back(QuicFrame(&ack_frame2));
   ProcessFramesPacketAtLevel(1, frames, ENCRYPTION_HANDSHAKE);
-  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space)) {
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
     // Verify RTT inflation gets mitigated.
     EXPECT_EQ(rtt_stats->latest_rtt(), kTestRTT);
   } else {
@@ -11496,6 +11496,57 @@
   }
 }
 
+// Regression test for b/161228202
+TEST_P(QuicConnectionTest, CoalscingPacketCausesInfiniteLoop) {
+  if (!connection_.SupportsMultiplePacketNumberSpaces()) {
+    return;
+  }
+  set_perspective(Perspective::IS_SERVER);
+  use_tagging_decrypter();
+  // Receives packet 1000 in initial data.
+  if (QuicVersionUsesCryptoFrames(connection_.transport_version())) {
+    EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber());
+  }
+  EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber());
+
+  // Set anti amplification factor to 2, such that RetransmitDataOfSpaceIfAny
+  // makes no forward progress and causes infinite loop.
+  SetQuicFlag(FLAGS_quic_anti_amplification_factor, 2);
+
+  ProcessCryptoPacketAtLevel(1000, ENCRYPTION_INITIAL);
+  EXPECT_TRUE(connection_.HasPendingAcks());
+
+  connection_.SetEncrypter(ENCRYPTION_INITIAL,
+                           std::make_unique<TaggingEncrypter>(0x01));
+  connection_.SetDefaultEncryptionLevel(ENCRYPTION_INITIAL);
+  // Send INITIAL 1.
+  std::string initial_crypto_data(512, 'a');
+  connection_.SendCryptoDataWithString(initial_crypto_data, 0,
+                                       ENCRYPTION_INITIAL);
+  ASSERT_TRUE(connection_.sent_packet_manager()
+                  .GetRetransmissionTime()
+                  .IsInitialized());
+  QuicTime::Delta pto_timeout =
+      connection_.sent_packet_manager().GetRetransmissionTime() - clock_.Now();
+  // Send Handshake 2.
+  connection_.SetEncrypter(ENCRYPTION_HANDSHAKE,
+                           std::make_unique<TaggingEncrypter>(0x02));
+  connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
+  if (GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2)) {
+    // Verify HANDSHAKE packet is coalesced with INITIAL retransmission.
+    EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(2);
+  } else {
+    EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1);
+  }
+  std::string handshake_crypto_data(1024, 'a');
+  connection_.SendCryptoDataWithString(handshake_crypto_data, 0,
+                                       ENCRYPTION_HANDSHAKE);
+
+  // INITIAL 1 gets lost and PTO fires.
+  clock_.AdvanceTime(pto_timeout);
+  connection_.GetRetransmissionAlarm()->Fire();
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic

quic/core/quic_packet_creator.h

diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h
index 3cc7b64..c909cd4 100644
--- a/quic/core/quic_packet_creator.h
+++ b/quic/core/quic_packet_creator.h
@@ -655,7 +655,7 @@
       GetQuicReloadableFlag(quic_determine_serialized_packet_fate_early);
 
   const bool coalesced_packet_of_higher_space_ =
-      GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space);
+      GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2);
 };
 
 }  // namespace quic