Close connection on packet serialization failures in SerializePacket. Protected by FLAGS_quic_reloadable_flag_quic_close_connection_on_serialization_failure. PiperOrigin-RevId: 329331250 Change-Id: I04f27188e8c3288a58697f2b9bbaf825c0e3c652
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc index 0bf8bdc..c06d167 100644 --- a/quic/core/quic_packet_creator.cc +++ b/quic/core/quic_packet_creator.cc
@@ -133,6 +133,9 @@ fully_pad_crypto_handshake_packets_(true), latched_hard_max_packet_length_(0), max_datagram_frame_size_(0) { + if (close_connection_on_serialization_failure_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_close_connection_on_serialization_failure); + } SetMaxPacketLength(kDefaultMaxPacketSize); if (!framer_->version().UsesTls()) { // QUIC+TLS negotiates the maximum datagram frame size via the @@ -470,12 +473,18 @@ } DCHECK_EQ(nullptr, packet_.encrypted_buffer); - SerializePacket(std::move(external_buffer), kMaxOutgoingPacketSize); + const bool success = + SerializePacket(std::move(external_buffer), kMaxOutgoingPacketSize); + if (close_connection_on_serialization_failure_ && !success) { + return; + } OnSerializedPacket(); } void QuicPacketCreator::OnSerializedPacket() { - if (packet_.encrypted_buffer == nullptr) { + if (close_connection_on_serialization_failure_) { + QUIC_BUG_IF(packet_.encrypted_buffer == nullptr); + } else if (packet_.encrypted_buffer == nullptr) { const std::string error_details = "Failed to SerializePacket."; QUIC_BUG << error_details; delegate_->OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, @@ -543,8 +552,11 @@ return 0; } } - SerializePacket(QuicOwnedPacketBuffer(buffer, nullptr), buffer_len); - // TODO(b/166255274): report unrecoverable error on serialization failures. + const bool success = + SerializePacket(QuicOwnedPacketBuffer(buffer, nullptr), buffer_len); + if (close_connection_on_serialization_failure_ && !success) { + return 0; + } const size_t encrypted_length = packet_.encrypted_length; // Clear frames in packet_. No need to DeleteFrames since frames are owned by // initial_packet. @@ -562,6 +574,7 @@ bool fin, TransmissionType transmission_type, size_t* num_bytes_consumed) { + // TODO(b/167222597): consider using ScopedSerializationFailureHandler. DCHECK(queued_frames_.empty()); DCHECK(!QuicUtils::IsCryptoStreamId(transport_version(), id)); // Write out the packet header @@ -738,11 +751,22 @@ return false; } -void QuicPacketCreator::SerializePacket(QuicOwnedPacketBuffer encrypted_buffer, +bool 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); + if (close_connection_on_serialization_failure_ && + packet_.encrypted_buffer != nullptr) { + const std::string error_details = + "Packet's encrypted buffer is not empty before serialization"; + QUIC_BUG << error_details; + delegate_->OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, + error_details); + return false; + } + const bool use_handler = + GetQuicReloadableFlag( + quic_neuter_initial_packet_in_coalescer_with_initial_key_discarded) || + close_connection_on_serialization_failure_; + ScopedSerializationFailureHandler handler(use_handler ? this : nullptr); DCHECK_LT(0u, encrypted_buffer_len); QUIC_BUG_IF(queued_frames_.empty() && pending_padding_bytes_ == 0) @@ -772,7 +796,7 @@ << QuicFramesToString(queued_frames_) << " at missing encryption_level " << packet_.encryption_level << " using " << framer_->version(); - return; + return false; } DCHECK_GE(max_plaintext_size_, packet_size_); @@ -790,7 +814,7 @@ << latched_hard_max_packet_length_ << ", max_packet_length_: " << max_packet_length_ << ", header: " << header; - return; + return false; } // ACK Frames will be truncated due to length only if they're the only frame @@ -811,11 +835,11 @@ encrypted_buffer_len, encrypted_buffer.buffer); if (encrypted_length == 0) { QUIC_BUG << "Failed to encrypt packet number " << packet_.packet_number; - return; + return false; } packet_size_ = 0; - if (!use_queued_frames_cleaner) { + if (!use_handler) { queued_frames_.clear(); } packet_.encrypted_buffer = encrypted_buffer.buffer; @@ -823,6 +847,7 @@ encrypted_buffer.buffer = nullptr; packet_.release_encrypted_buffer = std::move(encrypted_buffer).release_buffer; + return true; } std::unique_ptr<QuicEncryptedPacket> @@ -1983,15 +2008,25 @@ creator_->SetDefaultPeerAddress(old_peer_address_); } -QuicPacketCreator::ScopedQueuedFramesCleaner::ScopedQueuedFramesCleaner( - QuicPacketCreator* creator) +QuicPacketCreator::ScopedSerializationFailureHandler:: + ScopedSerializationFailureHandler(QuicPacketCreator* creator) : creator_(creator) {} -QuicPacketCreator::ScopedQueuedFramesCleaner::~ScopedQueuedFramesCleaner() { +QuicPacketCreator::ScopedSerializationFailureHandler:: + ~ScopedSerializationFailureHandler() { if (creator_ == nullptr) { return; } + // Always clear queued_frames_. creator_->queued_frames_.clear(); + + if (creator_->close_connection_on_serialization_failure_ && + creator_->packet_.encrypted_buffer == nullptr) { + const std::string error_details = "Failed to SerializePacket."; + QUIC_BUG << error_details; + creator_->delegate_->OnUnrecoverableError(QUIC_FAILED_TO_SERIALIZE_PACKET, + error_details); + } } void QuicPacketCreator::set_encryption_level(EncryptionLevel level) {
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h index 02bc9e3..a82cec4 100644 --- a/quic/core/quic_packet_creator.h +++ b/quic/core/quic_packet_creator.h
@@ -27,6 +27,7 @@ #include "net/third_party/quiche/src/quic/core/quic_packets.h" #include "net/third_party/quiche/src/quic/core/quic_types.h" #include "net/third_party/quiche/src/quic/platform/api/quic_export.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_macros.h" #include "net/third_party/quiche/src/common/platform/api/quiche_string_piece.h" namespace quic { @@ -472,11 +473,12 @@ private: friend class test::QuicPacketCreatorPeer; - // Used to clear queued_frames_ of creator upon exiting the scope. - class QUIC_EXPORT_PRIVATE ScopedQueuedFramesCleaner { + // Used to 1) clear queued_frames_, 2) report unrecoverable error (if + // serialization fails) upon exiting the scope. + class QUIC_EXPORT_PRIVATE ScopedSerializationFailureHandler { public: - explicit ScopedQueuedFramesCleaner(QuicPacketCreator* creator); - ~ScopedQueuedFramesCleaner(); + explicit ScopedSerializationFailureHandler(QuicPacketCreator* creator); + ~ScopedSerializationFailureHandler(); private: QuicPacketCreator* creator_; // Unowned. @@ -507,10 +509,11 @@ // Serializes all frames which have been added and adds any which should be // retransmitted to packet_.retransmittable_frames. All frames must fit into - // a single packet. - // Fails if |encrypted_buffer_len| isn't long enough for the encrypted packet. - void SerializePacket(QuicOwnedPacketBuffer encrypted_buffer, - size_t encrypted_buffer_len); + // a single packet. Returns true on success, otherwise, returns false. + // Fails if |encrypted_buffer| is not large enough for the encrypted packet. + QUIC_MUST_USE_RESULT bool SerializePacket( + QuicOwnedPacketBuffer encrypted_buffer, + size_t encrypted_buffer_len); // Called after a new SerialiedPacket is created to call the delegate's // OnSerializedPacket and reset state. @@ -663,6 +666,9 @@ const bool coalesced_packet_of_higher_space_ = GetQuicReloadableFlag(quic_coalesced_packet_of_higher_space2); + + const bool close_connection_on_serialization_failure_ = + GetQuicReloadableFlag(quic_close_connection_on_serialization_failure); }; } // namespace quic
diff --git a/quic/test_tools/quic_packet_creator_peer.cc b/quic/test_tools/quic_packet_creator_peer.cc index 151575a..f94620b 100644 --- a/quic/test_tools/quic_packet_creator_peer.cc +++ b/quic/test_tools/quic_packet_creator_peer.cc
@@ -111,7 +111,9 @@ bool success = creator->AddFrame(frame, NOT_RETRANSMISSION); DCHECK(success); } - creator->SerializePacket(QuicOwnedPacketBuffer(buffer, nullptr), buffer_len); + const bool success = creator->SerializePacket( + QuicOwnedPacketBuffer(buffer, nullptr), buffer_len); + DCHECK(success); SerializedPacket packet = std::move(creator->packet_); // The caller takes ownership of the QuicEncryptedPacket. creator->packet_.encrypted_buffer = nullptr;