Fix coalesced packet processing
Make sure we correctly process all coalesced packets without modifying the collection while we're iterating on it.
This issue was found by clusterfuzz:
https://bugs.chromium.org/p/chromium/issues/detail?id=990001
I've confirmed that the new test fails with the old code and passes with the fix, and that the fuzzer no longer crashes.
gfe-relnote: fix coalesced packet processing, protected by disabled v99 flag
PiperOrigin-RevId: 261433275
Change-Id: Iea1edf70fc84873fc7fe2f05c749759b2c5a6c9b
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 0b648e2..0c33604 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -2620,15 +2620,12 @@
void QuicConnection::MaybeProcessCoalescedPackets() {
bool processed = false;
- for (const auto& packet : coalesced_packets_) {
- if (!connected_) {
- return;
- }
+ while (connected_ && !coalesced_packets_.empty()) {
+ std::unique_ptr<QuicEncryptedPacket> packet =
+ std::move(coalesced_packets_.front());
+ coalesced_packets_.pop_front();
- // }
- // while (connected_ && !coalesced_packets_.empty()) {
QUIC_DVLOG(1) << ENDPOINT << "Processing coalesced packet";
- // QuicEncryptedPacket* packet = coalesced_packets_.front().get();
if (framer_.ProcessPacket(*packet)) {
processed = true;
} else {
@@ -2644,9 +2641,7 @@
}
}
}
- // coalesced_packets_.pop_front();
}
- coalesced_packets_.clear();
if (processed) {
MaybeProcessUndecryptablePackets();
}
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index a3ab06f..773db66 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -8562,6 +8562,53 @@
EXPECT_FALSE(ack_alarm->IsSet());
}
+// Verify that a packet containing three coalesced packets is parsed correctly.
+TEST_P(QuicConnectionTest, CoalescedPacket) {
+ if (!QuicVersionHasLongHeaderLengths(connection_.transport_version())) {
+ // Coalesced packets can only be encoded using long header lengths.
+ return;
+ }
+ EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
+ EXPECT_TRUE(connection_.connected());
+ if (QuicVersionUsesCryptoFrames(connection_.transport_version())) {
+ EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(3);
+ } else {
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(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());
+}
+
} // namespace
} // namespace test
} // namespace quic
