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
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);
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
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
