Neuter initial packet in the coalescer when discarding initial keys. PiperOrigin-RevId: 328808677 Change-Id: I34a3dbc33c52146fb88d65c60cdfc5ca8bcf1cf7
diff --git a/quic/core/quic_coalesced_packet.cc b/quic/core/quic_coalesced_packet.cc index b92cf75..e266a76 100644 --- a/quic/core/quic_coalesced_packet.cc +++ b/quic/core/quic_coalesced_packet.cc
@@ -97,6 +97,26 @@ initial_packet_ = nullptr; } +void QuicCoalescedPacket::NeuterInitialPacket() { + if (initial_packet_ == nullptr) { + return; + } + if (length_ < initial_packet_->encrypted_length) { + QUIC_BUG << "length_: " << length_ + << ", is less than initial packet length: " + << initial_packet_->encrypted_length; + Clear(); + return; + } + length_ -= initial_packet_->encrypted_length; + if (length_ == 0) { + Clear(); + return; + } + transmission_types_[ENCRYPTION_INITIAL] = NOT_RETRANSMISSION; + initial_packet_ = nullptr; +} + bool QuicCoalescedPacket::CopyEncryptedBuffers(char* buffer, size_t buffer_len, size_t* length_copied) const {
diff --git a/quic/core/quic_coalesced_packet.h b/quic/core/quic_coalesced_packet.h index 6cf712d..64275f6 100644 --- a/quic/core/quic_coalesced_packet.h +++ b/quic/core/quic_coalesced_packet.h
@@ -27,6 +27,9 @@ // Clears this coalesced packet. void Clear(); + // Clears all state associated with initial_packet_. + void NeuterInitialPacket(); + // Copies encrypted_buffers_ to |buffer| and sets |length_copied| to the // copied amount. Returns false if copy fails (i.e., |buffer_len| is not // enough).
diff --git a/quic/core/quic_coalesced_packet_test.cc b/quic/core/quic_coalesced_packet_test.cc index 81e6b27..dd1db44 100644 --- a/quic/core/quic_coalesced_packet_test.cc +++ b/quic/core/quic_coalesced_packet_test.cc
@@ -125,6 +125,84 @@ length_copied, expected, 1000); } +TEST(QuicCoalescedPacketTest, NeuterInitialPacket) { + QuicCoalescedPacket coalesced; + EXPECT_EQ("total_length: 0 padding_size: 0 packets: {}", + coalesced.ToString(0)); + // Noop when neutering initial packet on a empty coalescer. + coalesced.NeuterInitialPacket(); + EXPECT_EQ("total_length: 0 padding_size: 0 packets: {}", + coalesced.ToString(0)); + + SimpleBufferAllocator allocator; + EXPECT_EQ(0u, coalesced.length()); + char buffer[1000]; + QuicSocketAddress self_address(QuicIpAddress::Loopback4(), 1); + QuicSocketAddress peer_address(QuicIpAddress::Loopback4(), 2); + SerializedPacket packet1(QuicPacketNumber(1), PACKET_4BYTE_PACKET_NUMBER, + buffer, 500, false, false); + packet1.transmission_type = PTO_RETRANSMISSION; + QuicAckFrame ack_frame(InitAckFrame(1)); + packet1.nonretransmittable_frames.push_back(QuicFrame(&ack_frame)); + packet1.retransmittable_frames.push_back( + QuicFrame(QuicStreamFrame(1, true, 0, 100))); + ASSERT_TRUE(coalesced.MaybeCoalescePacket(packet1, self_address, peer_address, + &allocator, 1500)); + EXPECT_EQ(PTO_RETRANSMISSION, + coalesced.TransmissionTypeOfPacket(ENCRYPTION_INITIAL)); + EXPECT_EQ(1500u, coalesced.max_packet_length()); + EXPECT_EQ(500u, coalesced.length()); + EXPECT_EQ( + "total_length: 1500 padding_size: 1000 packets: {ENCRYPTION_INITIAL}", + coalesced.ToString(1500)); + // Neuter initial packet. + coalesced.NeuterInitialPacket(); + EXPECT_EQ(0u, coalesced.max_packet_length()); + EXPECT_EQ(0u, coalesced.length()); + EXPECT_EQ("total_length: 0 padding_size: 0 packets: {}", + coalesced.ToString(0)); + + // Coalesce initial packet again. + ASSERT_TRUE(coalesced.MaybeCoalescePacket(packet1, self_address, peer_address, + &allocator, 1500)); + + SerializedPacket packet2(QuicPacketNumber(3), PACKET_4BYTE_PACKET_NUMBER, + buffer, 500, false, false); + packet2.nonretransmittable_frames.push_back(QuicFrame(QuicPaddingFrame(100))); + packet2.encryption_level = ENCRYPTION_ZERO_RTT; + packet2.transmission_type = LOSS_RETRANSMISSION; + ASSERT_TRUE(coalesced.MaybeCoalescePacket(packet2, self_address, peer_address, + &allocator, 1500)); + EXPECT_EQ(1500u, coalesced.max_packet_length()); + EXPECT_EQ(1000u, coalesced.length()); + EXPECT_EQ(LOSS_RETRANSMISSION, + coalesced.TransmissionTypeOfPacket(ENCRYPTION_ZERO_RTT)); + EXPECT_EQ( + "total_length: 1500 padding_size: 500 packets: {ENCRYPTION_INITIAL, " + "ENCRYPTION_ZERO_RTT}", + coalesced.ToString(1500)); + + // Neuter initial packet. + coalesced.NeuterInitialPacket(); + EXPECT_EQ(1500u, coalesced.max_packet_length()); + EXPECT_EQ(500u, coalesced.length()); + EXPECT_EQ( + "total_length: 1500 padding_size: 1000 packets: {ENCRYPTION_ZERO_RTT}", + coalesced.ToString(1500)); + + SerializedPacket packet3(QuicPacketNumber(5), PACKET_4BYTE_PACKET_NUMBER, + buffer, 501, false, false); + packet3.encryption_level = ENCRYPTION_FORWARD_SECURE; + EXPECT_TRUE(coalesced.MaybeCoalescePacket(packet3, self_address, peer_address, + &allocator, 1500)); + EXPECT_EQ(1500u, coalesced.max_packet_length()); + EXPECT_EQ(1001u, coalesced.length()); + // Neuter initial packet. + coalesced.NeuterInitialPacket(); + EXPECT_EQ(1500u, coalesced.max_packet_length()); + EXPECT_EQ(1001u, coalesced.length()); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 8b71b3a..84396a0 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -2476,6 +2476,13 @@ void QuicConnection::NeuterUnencryptedPackets() { sent_packet_manager_.NeuterUnencryptedPackets(); + if (GetQuicReloadableFlag( + quic_neuter_initial_packet_in_coalescer_with_initial_key_discarded) && + version().CanSendCoalescedPackets()) { + QUIC_RELOADABLE_FLAG_COUNT( + quic_neuter_initial_packet_in_coalescer_with_initial_key_discarded); + coalesced_packet_.NeuterInitialPacket(); + } // This may have changed the retransmission timer, so re-arm it. SetRetransmissionAlarm(); if (default_enable_5rto_blackhole_detection_) {
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index e4cc987..7133bab 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -11880,6 +11880,45 @@ EXPECT_EQ(clock_.Now() + timeout, connection_.GetTimeoutAlarm()->deadline()); } +// Regression test for b/166255274 +TEST_P(QuicConnectionTest, + ReserializeInitialPacketInCoalescerAfterDiscardingInitialKey) { + SetQuicReloadableFlag( + quic_neuter_initial_packet_in_coalescer_with_initial_key_discarded, true); + if (!connection_.version().CanSendCoalescedPackets()) { + return; + } + use_tagging_decrypter(); + connection_.SetEncrypter(ENCRYPTION_INITIAL, + std::make_unique<TaggingEncrypter>(0x01)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_INITIAL); + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(1); + ProcessCryptoPacketAtLevel(1, ENCRYPTION_INITIAL); + EXPECT_TRUE(connection_.HasPendingAcks()); + connection_.SetEncrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<TaggingEncrypter>(0x02)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE); + { + QuicConnection::ScopedPacketFlusher flusher(&connection_); + connection_.GetAckAlarm()->Fire(); + // Verify this ACK packet is on hold. + EXPECT_EQ(0u, writer_->packets_write_attempts()); + + // Discard INITIAL key while there is an INITIAL packet in the coalescer. + connection_.RemoveEncrypter(ENCRYPTION_INITIAL); + connection_.NeuterUnencryptedPackets(); + } + // If not setting + // quic_neuter_initial_packet_in_coalescer_with_initial_key_discarded, there + // will be pending frames in the creator. + EXPECT_FALSE(connection_.packet_creator().HasPendingFrames()); + // The ACK frame is deleted along with initial_packet_ in coalescer. Sending + // connection close would cause this (released) ACK frame be serialized (and + // crashes). + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(1); + ProcessDataPacketAtLevel(1000, false, ENCRYPTION_FORWARD_SECURE); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index 358a4ae..0bf8bdc 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -544,6 +544,7 @@ } } SerializePacket(QuicOwnedPacketBuffer(buffer, nullptr), buffer_len); + // TODO(b/166255274): report unrecoverable error on serialization failures. const size_t encrypted_length = packet_.encrypted_length; // Clear frames in packet_. No need to DeleteFrames since frames are owned by // initial_packet. @@ -739,6 +740,10 @@ void QuicPacketCreator::SerializePacket(QuicOwnedPacketBuffer encrypted_buffer, size_t encrypted_buffer_len) { + const bool use_queued_frames_cleaner = GetQuicReloadableFlag( + quic_neuter_initial_packet_in_coalescer_with_initial_key_discarded); + ScopedQueuedFramesCleaner cleaner(use_queued_frames_cleaner ? this : nullptr); + DCHECK_LT(0u, encrypted_buffer_len); QUIC_BUG_IF(queued_frames_.empty() && pending_padding_bytes_ == 0) << "Attempt to serialize empty packet"; @@ -810,7 +815,9 @@ } packet_size_ = 0; - queued_frames_.clear(); + if (!use_queued_frames_cleaner) { + queued_frames_.clear(); + } packet_.encrypted_buffer = encrypted_buffer.buffer; packet_.encrypted_length = encrypted_length; @@ -1976,6 +1983,17 @@ creator_->SetDefaultPeerAddress(old_peer_address_); } +QuicPacketCreator::ScopedQueuedFramesCleaner::ScopedQueuedFramesCleaner( + QuicPacketCreator* creator) + : creator_(creator) {} + +QuicPacketCreator::ScopedQueuedFramesCleaner::~ScopedQueuedFramesCleaner() { + if (creator_ == nullptr) { + return; + } + creator_->queued_frames_.clear(); +} + void QuicPacketCreator::set_encryption_level(EncryptionLevel level) { DCHECK(level == packet_.encryption_level || !HasPendingFrames()) << "Cannot update encryption level from " << packet_.encryption_level
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h index 19f0c2a..02bc9e3 100644 --- a/quic/core/quic_packet_creator.h +++ b/quic/core/quic_packet_creator.h
@@ -472,6 +472,16 @@ private: friend class test::QuicPacketCreatorPeer; + // Used to clear queued_frames_ of creator upon exiting the scope. + class QUIC_EXPORT_PRIVATE ScopedQueuedFramesCleaner { + public: + explicit ScopedQueuedFramesCleaner(QuicPacketCreator* creator); + ~ScopedQueuedFramesCleaner(); + + private: + QuicPacketCreator* creator_; // Unowned. + }; + // Creates a stream frame which fits into the current open packet. If // |data_size| is 0 and fin is true, the expected behavior is to consume // the fin.