Close QUIC connection if it fails to serialize a coalesced packet. Protected by FLAGS_quic_reloadable_flag_quic_close_connection_if_fail_to_serialzie_coalesced_packet. PiperOrigin-RevId: 434638558
diff --git a/quic/core/quic_coalesced_packet.cc b/quic/core/quic_coalesced_packet.cc index e8ce931..dcfcb09 100644 --- a/quic/core/quic_coalesced_packet.cc +++ b/quic/core/quic_coalesced_packet.cc
@@ -181,4 +181,17 @@ return info; } +std::vector<size_t> QuicCoalescedPacket::packet_lengths() const { + std::vector<size_t> lengths; + for (const auto& packet : encrypted_buffers_) { + if (lengths.empty()) { + lengths.push_back( + initial_packet_ == nullptr ? 0 : initial_packet_->encrypted_length); + } else { + lengths.push_back(packet.length()); + } + } + return lengths; +} + } // namespace quic
diff --git a/quic/core/quic_coalesced_packet.h b/quic/core/quic_coalesced_packet.h index 9b8295c..2e4165d 100644 --- a/quic/core/quic_coalesced_packet.h +++ b/quic/core/quic_coalesced_packet.h
@@ -9,6 +9,10 @@ namespace quic { +namespace test { +class QuicCoalescedPacketPeer; +} + // QuicCoalescedPacket is used to buffer multiple packets which can be coalesced // into the same UDP datagram. class QUIC_EXPORT_PRIVATE QuicCoalescedPacket { @@ -61,7 +65,11 @@ QuicPacketLength max_packet_length() const { return max_packet_length_; } + std::vector<size_t> packet_lengths() const; + private: + friend class test::QuicCoalescedPacketPeer; + // self/peer addresses are set when trying to coalesce the first packet. // Packets with different self/peer addresses cannot be coalesced. QuicSocketAddress self_address_;
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 4715925..0ceafd9 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -3391,6 +3391,10 @@ packet_creator_.max_packet_length())) { // Failed to coalesce packet, flush current coalesced packet. if (!FlushCoalescedPacket()) { + QUIC_BUG_IF(quic_connection_connected_after_flush_coalesced_failure, + connected_) + << "QUIC connection is still connected after failing to flush " + "coalesced packet."; // Failed to flush coalesced packet, write error has been handled. return false; } @@ -5905,6 +5909,15 @@ const size_t length = packet_creator_.SerializeCoalescedPacket( coalesced_packet_, buffer, coalesced_packet_.max_packet_length()); if (length == 0) { + if (GetQuicReloadableFlag( + quic_close_connection_if_fail_to_serialzie_coalesced_packet) && + connected_) { + QUIC_RELOADABLE_FLAG_COUNT( + quic_close_connection_if_fail_to_serialzie_coalesced_packet); + CloseConnection(QUIC_FAILED_TO_SERIALIZE_PACKET, + "Failed to serialize coalesced packet.", + ConnectionCloseBehavior::SILENT_CLOSE); + } return false; } if (debug_visitor_ != nullptr) {
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index db7ae69..d0ee223 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -13,6 +13,7 @@ #include "absl/base/macros.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "quic/core/congestion_control/loss_detection_interface.h" #include "quic/core/congestion_control/send_algorithm_interface.h" @@ -42,6 +43,7 @@ #include "quic/platform/api/quic_test.h" #include "quic/test_tools/mock_clock.h" #include "quic/test_tools/mock_random.h" +#include "quic/test_tools/quic_coalesced_packet_peer.h" #include "quic/test_tools/quic_config_peer.h" #include "quic/test_tools/quic_connection_peer.h" #include "quic/test_tools/quic_framer_peer.h" @@ -10266,6 +10268,81 @@ EXPECT_NE(nullptr, writer_->coalesced_packet()); } +TEST_P(QuicConnectionTest, FailToCoalescePacket) { + // EXPECT_QUIC_BUG tests are expensive so only run one instance of them. + if (!IsDefaultTestConfiguration() || + !connection_.version().CanSendCoalescedPackets()) { + return; + } + + set_perspective(Perspective::IS_SERVER); + use_tagging_decrypter(); + + EXPECT_CALL(visitor_, OnHandshakePacketSent()); + + if (GetQuicReloadableFlag( + quic_close_connection_if_fail_to_serialzie_coalesced_packet)) { + EXPECT_CALL(visitor_, + OnConnectionClosed(_, ConnectionCloseSource::FROM_SELF)) + .WillOnce(Invoke(this, &QuicConnectionTest::SaveConnectionCloseFrame)); + } + + ProcessDataPacketAtLevel(1, !kHasStopWaiting, ENCRYPTION_INITIAL); + auto test_body = [&] { + QuicConnection::ScopedPacketFlusher flusher(&connection_); + connection_.SetEncrypter(ENCRYPTION_INITIAL, + std::make_unique<TaggingEncrypter>(0x01)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_INITIAL); + connection_.SendCryptoDataWithString("foo", 0); + // Verify this packet is on hold. + EXPECT_EQ(0u, writer_->packets_write_attempts()); + + connection_.SetEncrypter(ENCRYPTION_HANDSHAKE, + std::make_unique<TaggingEncrypter>(0x02)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE); + connection_.SendCryptoDataWithString("bar", 3); + EXPECT_EQ(0u, writer_->packets_write_attempts()); + + connection_.SetEncrypter(ENCRYPTION_FORWARD_SECURE, + std::make_unique<TaggingEncrypter>(0x03)); + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + SendStreamDataToPeer(2, "baz", 3, NO_FIN, nullptr); + + creator_->Flush(); + + auto& coalesced_packet = + QuicConnectionPeer::GetCoalescedPacket(&connection_); + QuicPacketLength coalesced_packet_max_length = + coalesced_packet.max_packet_length(); + QuicCoalescedPacketPeer::SetMaxPacketLength(coalesced_packet, + coalesced_packet.length()); + + // Make the coalescer's FORWARD_SECURE packet longer. + *QuicCoalescedPacketPeer::GetMutableEncryptedBuffer( + coalesced_packet, ENCRYPTION_FORWARD_SECURE) += "!!! TEST !!!"; + + QUIC_LOG(INFO) << "Reduced coalesced_packet_max_length from " + << coalesced_packet_max_length << " to " + << coalesced_packet.max_packet_length() + << ", coalesced_packet.length:" << coalesced_packet.length() + << ", coalesced_packet.packet_lengths:" + << absl::StrJoin(coalesced_packet.packet_lengths(), ":"); + }; + + EXPECT_QUIC_BUG(test_body(), "SerializeCoalescedPacket failed."); + + if (GetQuicReloadableFlag( + quic_close_connection_if_fail_to_serialzie_coalesced_packet)) { + EXPECT_FALSE(connection_.connected()); + EXPECT_THAT(saved_connection_close_frame_.quic_error_code, + IsError(QUIC_FAILED_TO_SERIALIZE_PACKET)); + EXPECT_EQ(saved_connection_close_frame_.error_details, + "Failed to serialize coalesced packet."); + } else { + EXPECT_TRUE(connection_.connected()); + } +} + TEST_P(QuicConnectionTest, LegacyVersionEncapsulation) { connection_.EnableLegacyVersionEncapsulation("test.example.org");
diff --git a/quic/core/quic_crypto_stream_test.cc b/quic/core/quic_crypto_stream_test.cc index fffcb09..3760c7d 100644 --- a/quic/core/quic_crypto_stream_test.cc +++ b/quic/core/quic_crypto_stream_test.cc
@@ -107,6 +107,9 @@ &alarm_factory_, Perspective::IS_CLIENT)), session_(connection_, /*create_mock_crypto_stream=*/false) { + EXPECT_CALL(*static_cast<MockPacketWriter*>(connection_->writer()), + WritePacket(_, _, _, _, _)) + .WillRepeatedly(Return(WriteResult(WRITE_STATUS_OK, 0))); stream_ = new MockQuicCryptoStream(&session_); session_.SetCryptoStream(stream_); session_.Initialize();
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 33f9bc8..0fe1980 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -23,6 +23,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_add_cached_network_parameters_to_address_token2, true) // If true, 1) QUIC connections will use a lower minimum for trusted initial rtt, 2) When TRTT is received, QUIC server sessions will mark the initial rtt from CachedNetworkParameters as trusted. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_use_lower_min_for_trusted_irtt, false) +// If true, QUIC connection will be closed if it fails to serialize a coalesced packet. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_close_connection_if_fail_to_serialzie_coalesced_packet, true) // If true, QUIC will default enable MTU discovery at server, with a target of 1450 bytes. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_enable_mtu_discovery_at_server, false) // If true, QuicGsoBatchWriter will support release time if it is available and the process has the permission to do so.
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index 4065e7e..95149b1 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -14,6 +14,7 @@ #include "absl/base/macros.h" #include "absl/base/optimization.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "quic/core/crypto/crypto_protocol.h" @@ -1122,6 +1123,7 @@ QUIC_BUG_IF(quic_bug_12398_15, coalesced.length() == 0) << ENDPOINT << "Attempt to serialize empty coalesced packet"; size_t packet_length = 0; + size_t initial_length = 0; if (coalesced.initial_packet() != nullptr) { // Padding coalesced packet containing initial packet to full. size_t padding_size = coalesced.max_packet_length() - coalesced.length(); @@ -1132,7 +1134,7 @@ // Do not pad server initial connection close packet. padding_size = 0; } - size_t initial_length = ReserializeInitialPacketInCoalescedPacket( + initial_length = ReserializeInitialPacketInCoalescedPacket( *coalesced.initial_packet(), padding_size, buffer, buffer_len); if (initial_length == 0) { QUIC_BUG(quic_bug_10752_19) @@ -1147,6 +1149,14 @@ } size_t length_copied = 0; if (!coalesced.CopyEncryptedBuffers(buffer, buffer_len, &length_copied)) { + QUIC_BUG(quic_serialize_coalesced_packet_copy_failure) + << "SerializeCoalescedPacket failed. buffer_len:" << buffer_len + << ", initial_length:" << initial_length + << ", length_copied:" << length_copied + << ", coalesced.length:" << coalesced.length() + << ", coalesced.max_packet_length:" << coalesced.max_packet_length() + << ", coalesced.packet_lengths:" + << absl::StrJoin(coalesced.packet_lengths(), ":"); return 0; } packet_length += length_copied;
diff --git a/quic/test_tools/quic_coalesced_packet_peer.cc b/quic/test_tools/quic_coalesced_packet_peer.cc new file mode 100644 index 0000000..efd1900 --- /dev/null +++ b/quic/test_tools/quic_coalesced_packet_peer.cc
@@ -0,0 +1,23 @@ +// Copyright (c) 2022 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "quic/test_tools/quic_coalesced_packet_peer.h" + +namespace quic { +namespace test { + +// static +void QuicCoalescedPacketPeer::SetMaxPacketLength( + QuicCoalescedPacket& coalesced_packet, QuicPacketLength length) { + coalesced_packet.max_packet_length_ = length; +} + +// static +std::string* QuicCoalescedPacketPeer::GetMutableEncryptedBuffer( + QuicCoalescedPacket& coalesced_packet, EncryptionLevel encryption_level) { + return &coalesced_packet.encrypted_buffers_[encryption_level]; +} + +} // namespace test +} // namespace quic
diff --git a/quic/test_tools/quic_coalesced_packet_peer.h b/quic/test_tools/quic_coalesced_packet_peer.h new file mode 100644 index 0000000..908cf09 --- /dev/null +++ b/quic/test_tools/quic_coalesced_packet_peer.h
@@ -0,0 +1,26 @@ +// Copyright (c) 2022 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef QUICHE_QUIC_TEST_TOOLS_QUIC_COALESCED_PACKET_PEER_H_ +#define QUICHE_QUIC_TEST_TOOLS_QUIC_COALESCED_PACKET_PEER_H_ + +#include "quic/core/quic_coalesced_packet.h" +#include "quic/core/quic_types.h" + +namespace quic { +namespace test { + +class QuicCoalescedPacketPeer { + public: + static void SetMaxPacketLength(QuicCoalescedPacket& coalesced_packet, + QuicPacketLength length); + + static std::string* GetMutableEncryptedBuffer( + QuicCoalescedPacket& coalesced_packet, EncryptionLevel encryption_level); +}; + +} // namespace test +} // namespace quic + +#endif // QUICHE_QUIC_TEST_TOOLS_QUIC_COALESCED_PACKET_PEER_H_
diff --git a/quic/test_tools/quic_connection_peer.cc b/quic/test_tools/quic_connection_peer.cc index 207582f..5e60a84 100644 --- a/quic/test_tools/quic_connection_peer.cc +++ b/quic/test_tools/quic_connection_peer.cc
@@ -535,5 +535,16 @@ connection->last_decrypted_packet_level_ = level; } +// static +QuicCoalescedPacket& QuicConnectionPeer::GetCoalescedPacket( + QuicConnection* connection) { + return connection->coalesced_packet_; +} + +// static +void QuicConnectionPeer::FlushCoalescedPacket(QuicConnection* connection) { + connection->FlushCoalescedPacket(); +} + } // namespace test } // namespace quic
diff --git a/quic/test_tools/quic_connection_peer.h b/quic/test_tools/quic_connection_peer.h index 62ed60e..ff536c2 100644 --- a/quic/test_tools/quic_connection_peer.h +++ b/quic/test_tools/quic_connection_peer.h
@@ -220,6 +220,10 @@ static void SetLastDecryptedLevel(QuicConnection* connection, EncryptionLevel level); + + static QuicCoalescedPacket& GetCoalescedPacket(QuicConnection* connection); + + static void FlushCoalescedPacket(QuicConnection* connection); }; } // namespace test