Flush queued frames when processing coalesced packets
This fix mimics the code in QuicConnection:: MaybeProcessUndecryptablePackets, thanks rch@ and fayang@ for the analysis!
This issue was found by Chromium Clusterfuzz:
https://bugs.chromium.org/p/chromium/issues/detail?id=992831
I've confirmed that clusterfuzz test case 5646143163072512 no longer reproduces with the fix.
The new test fails without the fix and passes with it. More generally, increased test coverage for this area of code is tracked by b/139422067.
gfe-relnote: flush queued frames when processing coalesced packets, protected by disabled quic v99 flag
PiperOrigin-RevId: 263791562
Change-Id: I52da7342f33608488785ff646cc5faf9c9195fa6
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 864f652..31c778c 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2701,6 +2701,13 @@
void QuicConnection::MaybeProcessCoalescedPackets() {
bool processed = false;
while (connected_ && !coalesced_packets_.empty()) {
+ // Making sure there are no pending frames when processing the next
+ // coalesced packet because the queued ack frame may change.
+ packet_generator_.FlushAllQueuedFrames();
+ if (!connected_) {
+ return;
+ }
+
std::unique_ptr<QuicEncryptedPacket> packet =
std::move(coalesced_packets_.front());
coalesced_packets_.pop_front();
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 5546419..836f09f 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -8695,6 +8695,69 @@
EXPECT_TRUE(connection_.connected());
}
+// Regression test for crbug.com/992831.
+TEST_P(QuicConnectionTest, CoalescedPacketThatSavesFrames) {
+ if (!QuicVersionHasLongHeaderLengths(connection_.transport_version())) {
+ // Coalesced packets can only be encoded using long header lengths.
+ return;
+ }
+ if (connection_.SupportsMultiplePacketNumberSpaces()) {
+ // TODO(b/129151114) Enable this test with multiple packet number spaces.
+ return;
+ }
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+ EXPECT_TRUE(connection_.connected());
+ if (QuicVersionUsesCryptoFrames(connection_.transport_version())) {
+ EXPECT_CALL(visitor_, OnCryptoFrame(_))
+ .Times(3)
+ .WillRepeatedly([this](const QuicCryptoFrame& /*frame*/) {
+ // QuicFrame takes ownership of the QuicBlockedFrame.
+ connection_.SendControlFrame(QuicFrame(new QuicBlockedFrame(1, 3)));
+ });
+ } else {
+ EXPECT_CALL(visitor_, OnStreamFrame(_))
+ .Times(3)
+ .WillRepeatedly([this](const QuicStreamFrame& /*frame*/) {
+ // QuicFrame takes ownership of the QuicBlockedFrame.
+ connection_.SendControlFrame(QuicFrame(new QuicBlockedFrame(1, 3)));
+ });
+ }
+
+ uint64_t packet_numbers[3] = {1, 2, 3};
+ EncryptionLevel encryption_levels[3] = {
+ ENCRYPTION_INITIAL, ENCRYPTION_INITIAL, ENCRYPTION_FORWARD_SECURE};
+ char buffer[kMaxOutgoingPacketSize] = {};
+ size_t total_encrypted_length = 0;
+ for (int i = 0; i < 3; i++) {
+ QuicPacketHeader header =
+ ConstructPacketHeader(packet_numbers[i], encryption_levels[i]);
+ QuicFrames frames;
+ if (QuicVersionUsesCryptoFrames(connection_.transport_version())) {
+ frames.push_back(QuicFrame(&crypto_frame_));
+ } else {
+ frames.push_back(QuicFrame(frame1_));
+ }
+ std::unique_ptr<QuicPacket> packet = ConstructPacket(header, frames);
+ peer_creator_.set_encryption_level(encryption_levels[i]);
+ size_t encrypted_length = peer_framer_.EncryptPayload(
+ encryption_levels[i], QuicPacketNumber(packet_numbers[i]), *packet,
+ buffer + total_encrypted_length,
+ sizeof(buffer) - total_encrypted_length);
+ EXPECT_GT(encrypted_length, 0u);
+ total_encrypted_length += encrypted_length;
+ }
+ connection_.ProcessUdpPacket(
+ kSelfAddress, kPeerAddress,
+ QuicReceivedPacket(buffer, total_encrypted_length, clock_.Now(), false));
+ if (connection_.GetSendAlarm()->IsSet()) {
+ connection_.GetSendAlarm()->Fire();
+ }
+
+ EXPECT_TRUE(connection_.connected());
+
+ SendAckPacketToPeer();
+}
+
// Regresstion test for b/138962304.
TEST_P(QuicConnectionTest, RtoAndWriteBlocked) {
if (!QuicConnectionPeer::GetSentPacketManager(&connection_)
