In quic with tls, remove the head of line blocking caused by an unprocessable packet. protected by gfe2_reloadable_flag_quic_fix_undecryptable_packets. PiperOrigin-RevId: 317090147 Change-Id: Ib1376131fa7f69bad3c9b6a16fc1fad9341a52ee
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 9901afa..b30165d 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1989,24 +1989,8 @@ ++stats_.undecryptable_packets_received_before_handshake_complete; } - bool should_enqueue = true; - if (encryption_level_ == ENCRYPTION_FORWARD_SECURE) { - // We do not expect to install any further keys. - should_enqueue = false; - } else if (undecryptable_packets_.size() >= max_undecryptable_packets_) { - // We do not queue more than max_undecryptable_packets_ packets. - should_enqueue = false; - } else if (has_decryption_key) { - // We already have the key for this decryption level, therefore no - // future keys will allow it be decrypted. - should_enqueue = false; - } else if (version().KnowsWhichDecrypterToUse() && - decryption_level <= encryption_level_) { - // On versions that know which decrypter to use, we install keys in order - // so we will not get newer keys for lower encryption levels. - should_enqueue = false; - } - + const bool should_enqueue = + ShouldEnqueueUnDecryptablePacket(decryption_level, has_decryption_key); if (should_enqueue) { QueueUndecryptablePacket(packet, decryption_level); } @@ -2017,6 +2001,31 @@ } } +bool QuicConnection::ShouldEnqueueUnDecryptablePacket( + EncryptionLevel decryption_level, + bool has_decryption_key) const { + if (encryption_level_ == ENCRYPTION_FORWARD_SECURE) { + // We do not expect to install any further keys. + return false; + } + if (undecryptable_packets_.size() >= max_undecryptable_packets_) { + // We do not queue more than max_undecryptable_packets_ packets. + return false; + } + if (has_decryption_key) { + // We already have the key for this decryption level, therefore no + // future keys will allow it be decrypted. + return false; + } + if (version().KnowsWhichDecrypterToUse() && + decryption_level <= encryption_level_) { + // On versions that know which decrypter to use, we install keys in order + // so we will not get newer keys for lower encryption levels. + return false; + } + return true; +} + void QuicConnection::ProcessUdpPacket(const QuicSocketAddress& self_address, const QuicSocketAddress& peer_address, const QuicReceivedPacket& packet) { @@ -3142,27 +3151,79 @@ return; } - while (connected_ && !undecryptable_packets_.empty()) { - // Making sure there is no pending frames when processing next undecrypted - // packet because the queued ack frame may change. - packet_creator_.FlushCurrentPacket(); - if (!connected_) { - return; + if (GetQuicReloadableFlag(quic_fix_undecryptable_packets)) { + QUIC_RELOADABLE_FLAG_COUNT(quic_fix_undecryptable_packets); + auto iter = undecryptable_packets_.begin(); + while (connected_ && iter != undecryptable_packets_.end()) { + // Making sure there is no pending frames when processing next undecrypted + // packet because the queued ack frame may change. + packet_creator_.FlushCurrentPacket(); + if (!connected_) { + return; + } + UndecryptablePacket* undecryptable_packet = &*iter; + ++iter; + if (undecryptable_packet->processed) { + continue; + } + QUIC_DVLOG(1) << ENDPOINT << "Attempting to process undecryptable packet"; + if (debug_visitor_ != nullptr) { + debug_visitor_->OnAttemptingToProcessUndecryptablePacket( + undecryptable_packet->encryption_level); + } + if (framer_.ProcessPacket(*undecryptable_packet->packet)) { + QUIC_DVLOG(1) << ENDPOINT << "Processed undecryptable packet!"; + undecryptable_packet->processed = true; + ++stats_.packets_processed; + continue; + } + const bool has_decryption_key = + version().KnowsWhichDecrypterToUse() && + framer_.HasDecrypterOfEncryptionLevel( + undecryptable_packet->encryption_level); + if (framer_.error() == QUIC_DECRYPTION_FAILURE && + ShouldEnqueueUnDecryptablePacket( + undecryptable_packet->encryption_level, has_decryption_key)) { + QUIC_DVLOG(1) + << ENDPOINT + << "Need to attempt to process this undecryptable packet later"; + continue; + } + undecryptable_packet->processed = true; } - QUIC_DVLOG(1) << ENDPOINT << "Attempting to process undecryptable packet"; - const auto& undecryptable_packet = undecryptable_packets_.front(); - if (debug_visitor_ != nullptr) { - debug_visitor_->OnAttemptingToProcessUndecryptablePacket( - undecryptable_packet.encryption_level); + // Remove processed packets. We cannot remove elements in the while loop + // above because currently QuicCircularDeque does not support removing + // mid elements. + while (!undecryptable_packets_.empty()) { + if (!undecryptable_packets_.front().processed) { + break; + } + undecryptable_packets_.pop_front(); } - if (!framer_.ProcessPacket(*undecryptable_packet.packet) && - framer_.error() == QUIC_DECRYPTION_FAILURE) { - QUIC_DVLOG(1) << ENDPOINT << "Unable to process undecryptable packet..."; - break; + } else { + while (connected_ && !undecryptable_packets_.empty()) { + // Making sure there is no pending frames when processing next undecrypted + // packet because the queued ack frame may change. + packet_creator_.FlushCurrentPacket(); + if (!connected_) { + return; + } + QUIC_DVLOG(1) << ENDPOINT << "Attempting to process undecryptable packet"; + const auto& undecryptable_packet = undecryptable_packets_.front(); + if (debug_visitor_ != nullptr) { + debug_visitor_->OnAttemptingToProcessUndecryptablePacket( + undecryptable_packet.encryption_level); + } + if (!framer_.ProcessPacket(*undecryptable_packet.packet) && + framer_.error() == QUIC_DECRYPTION_FAILURE) { + QUIC_DVLOG(1) << ENDPOINT + << "Unable to process undecryptable packet..."; + break; + } + QUIC_DVLOG(1) << ENDPOINT << "Processed undecryptable packet!"; + ++stats_.packets_processed; + undecryptable_packets_.pop_front(); } - QUIC_DVLOG(1) << ENDPOINT << "Processed undecryptable packet!"; - ++stats_.packets_processed; - undecryptable_packets_.pop_front(); } // Once forward secure encryption is in use, there will be no
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index 118e9c4..8f4b113 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -1095,12 +1095,16 @@ struct QUIC_EXPORT_PRIVATE UndecryptablePacket { UndecryptablePacket(const QuicEncryptedPacket& packet, EncryptionLevel encryption_level) - : packet(packet.Clone()), encryption_level(encryption_level) {} + : packet(packet.Clone()), + encryption_level(encryption_level), + processed(false) {} std::unique_ptr<QuicEncryptedPacket> packet; - // Currently, |encryption_level| is only used for logging and does not - // affect processing of the packet. EncryptionLevel encryption_level; + // This gets set to true if 1) connection sucessfully processed the packet + // or 2) connection failed to process the packet and will not try to process + // it later. + bool processed; }; // Notifies the visitor of the close and marks the connection as disconnected. @@ -1314,6 +1318,11 @@ bool ValidateConfigConnectionIds(const QuicConfig& config); bool ValidateConfigConnectionIdsOld(const QuicConfig& config); + // Returns true if an undecryptable packet of |decryption_level| should be + // buffered (such that connection can try to decrypt it later). + bool ShouldEnqueueUnDecryptablePacket(EncryptionLevel decryption_level, + bool has_decryption_key) const; + QuicFramer framer_; // Contents received in the current packet, especially used to identify
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index db402c7..b1409bf 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -11196,6 +11196,67 @@ } } +TEST_P(QuicConnectionTest, ProcessUndecryptablePacketsBasedOnEncryptionLevel) { + if (!connection_.SupportsMultiplePacketNumberSpaces()) { + return; + } + // SetFromConfig is always called after construction from InitializeSession. + EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(AnyNumber()); + QuicConfig config; + connection_.SetFromConfig(config); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_INITIAL); + connection_.RemoveDecrypter(ENCRYPTION_FORWARD_SECURE); + use_tagging_decrypter(); + + peer_framer_.SetEncrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<TaggingEncrypter>(0x01)); + peer_framer_.SetEncrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<TaggingEncrypter>(0x02)); + + for (uint64_t i = 1; i <= 3; ++i) { + ProcessDataPacketAtLevel(i, !kHasStopWaiting, ENCRYPTION_HANDSHAKE); + } + ProcessDataPacketAtLevel(4, !kHasStopWaiting, ENCRYPTION_FORWARD_SECURE); + for (uint64_t j = 5; j <= 7; ++j) { + ProcessDataPacketAtLevel(j, !kHasStopWaiting, ENCRYPTION_HANDSHAKE); + } + EXPECT_EQ(7u, QuicConnectionPeer::NumUndecryptablePackets(&connection_)); + EXPECT_FALSE(connection_.GetProcessUndecryptablePacketsAlarm()->IsSet()); + SetDecrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<StrictTaggingDecrypter>(0x01)); + EXPECT_TRUE(connection_.GetProcessUndecryptablePacketsAlarm()->IsSet()); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE); + connection_.SetEncrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<TaggingEncrypter>(0x01)); + if (GetQuicReloadableFlag(quic_fix_undecryptable_packets)) { + // Verify all ENCRYPTION_HANDSHAKE packets get processed. + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(6); + } else { + // Verify packets before 4 get processed. + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(3); + } + connection_.GetProcessUndecryptablePacketsAlarm()->Fire(); + EXPECT_EQ(4u, QuicConnectionPeer::NumUndecryptablePackets(&connection_)); + + SetDecrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<StrictTaggingDecrypter>(0x02)); + EXPECT_TRUE(connection_.GetProcessUndecryptablePacketsAlarm()->IsSet()); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + connection_.SetEncrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<TaggingEncrypter>(0x02)); + if (GetQuicReloadableFlag(quic_fix_undecryptable_packets)) { + // Verify the 1-RTT packet gets processed. + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(1); + } else { + // Verify all packets get processed. + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(4); + } + connection_.GetProcessUndecryptablePacketsAlarm()->Fire(); + EXPECT_EQ(0u, QuicConnectionPeer::NumUndecryptablePackets(&connection_)); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index 726f1e5..847a166 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -2007,6 +2007,10 @@ return encrypter_[level] != nullptr; } +bool QuicFramer::HasDecrypterOfEncryptionLevel(EncryptionLevel level) const { + return decrypter_[level] != nullptr; +} + bool QuicFramer::AppendPacketHeader(const QuicPacketHeader& header, QuicDataWriter* writer, size_t* length_field_offset) {
diff --git a/quic/core/quic_framer.h b/quic/core/quic_framer.h index 8ace715..0aa5bbf 100644 --- a/quic/core/quic_framer.h +++ b/quic/core/quic_framer.h
@@ -571,6 +571,8 @@ // Returns true if encrypter of |level| is available. bool HasEncrypterOfEncryptionLevel(EncryptionLevel level) const; + // Returns true if decrypter of |level| is available. + bool HasDecrypterOfEncryptionLevel(EncryptionLevel level) const; void set_validate_flags(bool value) { validate_flags_ = value; }
diff --git a/quic/test_tools/quic_connection_peer.cc b/quic/test_tools/quic_connection_peer.cc index d276f63..54616b2 100644 --- a/quic/test_tools/quic_connection_peer.cc +++ b/quic/test_tools/quic_connection_peer.cc
@@ -377,5 +377,10 @@ connection->InstallInitialCrypters(server_connection_id); } +// static +size_t QuicConnectionPeer::NumUndecryptablePackets(QuicConnection* connection) { + return connection->undecryptable_packets_.size(); +} + } // namespace test } // namespace quic
diff --git a/quic/test_tools/quic_connection_peer.h b/quic/test_tools/quic_connection_peer.h index 9882f62..a5b94ef 100644 --- a/quic/test_tools/quic_connection_peer.h +++ b/quic/test_tools/quic_connection_peer.h
@@ -147,6 +147,8 @@ static void SetServerConnectionId( QuicConnection* connection, const QuicConnectionId& server_connection_id); + + static size_t NumUndecryptablePackets(QuicConnection* connection); }; } // namespace test