Do not add extra padding when reserializing INITIAL packets.
Protected by FLAGS_quic_reloadable_flag_quic_close_connection_if_fail_to_serialzie_coalesced_packet2.
PiperOrigin-RevId: 436302707
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 0ceafd9..72e47a3 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -5909,11 +5909,11 @@
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) &&
+ if (packet_creator_
+ .close_connection_if_fail_to_serialzie_coalesced_packet() &&
connected_) {
- QUIC_RELOADABLE_FLAG_COUNT(
- quic_close_connection_if_fail_to_serialzie_coalesced_packet);
+ QUIC_RELOADABLE_FLAG_COUNT_N(
+ quic_close_connection_if_fail_to_serialzie_coalesced_packet2, 2, 2);
CloseConnection(QUIC_FAILED_TO_SERIALIZE_PACKET,
"Failed to serialize coalesced packet.",
ConnectionCloseBehavior::SILENT_CLOSE);
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 337e6b7..93a2364 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -198,10 +198,16 @@
SetEncrypter(ENCRYPTION_FORWARD_SECURE,
std::make_unique<NullEncrypter>(perspective));
SetDataProducer(&producer_);
+ ON_CALL(*this, OnSerializedPacket(_))
+ .WillByDefault([this](SerializedPacket packet) {
+ QuicConnection::OnSerializedPacket(std::move(packet));
+ });
}
TestConnection(const TestConnection&) = delete;
TestConnection& operator=(const TestConnection&) = delete;
+ MOCK_METHOD(void, OnSerializedPacket, (SerializedPacket packet), (override));
+
void SetSendAlgorithm(SendAlgorithmInterface* send_algorithm) {
QuicConnectionPeer::SetSendAlgorithm(this, send_algorithm);
}
@@ -10281,7 +10287,7 @@
EXPECT_CALL(visitor_, OnHandshakePacketSent());
if (GetQuicReloadableFlag(
- quic_close_connection_if_fail_to_serialzie_coalesced_packet)) {
+ quic_close_connection_if_fail_to_serialzie_coalesced_packet2)) {
EXPECT_CALL(visitor_,
OnConnectionClosed(_, ConnectionCloseSource::FROM_SELF))
.WillOnce(Invoke(this, &QuicConnectionTest::SaveConnectionCloseFrame));
@@ -10332,7 +10338,7 @@
EXPECT_QUIC_BUG(test_body(), "SerializeCoalescedPacket failed.");
if (GetQuicReloadableFlag(
- quic_close_connection_if_fail_to_serialzie_coalesced_packet)) {
+ quic_close_connection_if_fail_to_serialzie_coalesced_packet2)) {
EXPECT_FALSE(connection_.connected());
EXPECT_THAT(saved_connection_close_frame_.quic_error_code,
IsError(QUIC_FAILED_TO_SERIALIZE_PACKET));
@@ -15412,6 +15418,114 @@
EXPECT_EQ(rtt_stats->latest_rtt(), kTestRTT);
}
+// Regression test for b/112480134.
+TEST_P(QuicConnectionTest, NoExtraPaddingInReserializedInitial) {
+ // EXPECT_QUIC_BUG tests are expensive so only run one instance of them.
+ if (!IsDefaultTestConfiguration() ||
+ !connection_.version().CanSendCoalescedPackets()) {
+ return;
+ }
+
+ set_perspective(Perspective::IS_SERVER);
+ MockQuicConnectionDebugVisitor debug_visitor;
+ connection_.set_debug_visitor(&debug_visitor);
+
+ uint64_t debug_visitor_sent_count = 0;
+ EXPECT_CALL(debug_visitor, OnPacketSent(_, _, _, _, _, _, _, _))
+ .WillRepeatedly([&]() { debug_visitor_sent_count++; });
+
+ EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber());
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber());
+ use_tagging_decrypter();
+
+ // Received INITIAL 1.
+ ProcessCryptoPacketAtLevel(1, ENCRYPTION_INITIAL);
+
+ peer_framer_.SetEncrypter(ENCRYPTION_ZERO_RTT,
+ std::make_unique<TaggingEncrypter>(0x02));
+
+ connection_.SetEncrypter(ENCRYPTION_INITIAL,
+ std::make_unique<TaggingEncrypter>(0x01));
+ connection_.SetEncrypter(ENCRYPTION_HANDSHAKE,
+ std::make_unique<TaggingEncrypter>(0x03));
+ SetDecrypter(ENCRYPTION_HANDSHAKE,
+ std::make_unique<StrictTaggingDecrypter>(0x03));
+ SetDecrypter(ENCRYPTION_ZERO_RTT,
+ std::make_unique<StrictTaggingDecrypter>(0x02));
+ connection_.SetEncrypter(ENCRYPTION_FORWARD_SECURE,
+ std::make_unique<TaggingEncrypter>(0x04));
+
+ // Received ENCRYPTION_ZERO_RTT 2.
+ ProcessDataPacketAtLevel(2, !kHasStopWaiting, ENCRYPTION_ZERO_RTT);
+
+ {
+ QuicConnection::ScopedPacketFlusher flusher(&connection_);
+ // Send INITIAL 1.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_INITIAL);
+ connection_.SendCryptoDataWithString("foo", 0, ENCRYPTION_INITIAL);
+ // Send HANDSHAKE 2.
+ EXPECT_CALL(visitor_, OnHandshakePacketSent()).Times(1);
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_HANDSHAKE);
+ connection_.SendCryptoDataWithString(std::string(200, 'a'), 0,
+ ENCRYPTION_HANDSHAKE);
+ // Send 1-RTT 3.
+ connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE);
+ connection_.SendStreamDataWithString(0, std::string(400, 'b'), 0, NO_FIN);
+ }
+
+ // Arrange the stream data to be sent in response to ENCRYPTION_INITIAL 3.
+ const std::string data4(1000, '4'); // Data to send in stream id 4
+ const std::string data8(3000, '8'); // Data to send in stream id 8
+ EXPECT_CALL(visitor_, OnCanWrite()).WillOnce([&]() {
+ connection_.producer()->SaveStreamData(4, data4);
+ connection_.producer()->SaveStreamData(8, data8);
+
+ notifier_.WriteOrBufferData(4, data4.size(), FIN_AND_PADDING);
+
+ // This should trigger FlushCoalescedPacket.
+ notifier_.WriteOrBufferData(8, data8.size(), FIN);
+ });
+
+ QuicByteCount pending_padding_after_serialize_2nd_1rtt_packet = 0;
+ QuicPacketCount num_1rtt_packets_serialized = 0;
+ EXPECT_CALL(connection_, OnSerializedPacket(_))
+ .WillRepeatedly([&](SerializedPacket packet) {
+ if (packet.encryption_level == ENCRYPTION_FORWARD_SECURE) {
+ num_1rtt_packets_serialized++;
+ if (num_1rtt_packets_serialized == 2) {
+ pending_padding_after_serialize_2nd_1rtt_packet =
+ connection_.packet_creator().pending_padding_bytes();
+ }
+ }
+ connection_.QuicConnection::OnSerializedPacket(std::move(packet));
+ });
+
+ // Server receives INITIAL 3, this will serialzie FS 7 (stream 4, stream 8),
+ // which will trigger a flush of a coalesced packet consists of INITIAL 4,
+ // HS 5 and FS 6 (stream 4).
+ if (GetQuicReloadableFlag(
+ quic_close_connection_if_fail_to_serialzie_coalesced_packet2)) {
+ // Expect no QUIC_BUG.
+ ProcessDataPacketAtLevel(3, !kHasStopWaiting, ENCRYPTION_INITIAL);
+ EXPECT_EQ(
+ debug_visitor_sent_count,
+ connection_.sent_packet_manager().GetLargestSentPacket().ToUint64());
+ } else {
+ // Expect QUIC_BUG due to extra padding.
+ EXPECT_QUIC_BUG(
+ { ProcessDataPacketAtLevel(3, !kHasStopWaiting, ENCRYPTION_INITIAL); },
+ "Reserialize initial packet in coalescer has unexpected size");
+ EXPECT_EQ(
+ debug_visitor_sent_count + 1,
+ connection_.sent_packet_manager().GetLargestSentPacket().ToUint64());
+ }
+
+ // The error only happens if after serializing the second 1RTT packet(pkt #7),
+ // the pending padding bytes is non zero.
+ EXPECT_GT(pending_padding_after_serialize_2nd_1rtt_packet, 0u);
+ EXPECT_TRUE(connection_.connected());
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 5bb98cc..f0f472d 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -24,7 +24,7 @@
// 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)
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_close_connection_if_fail_to_serialzie_coalesced_packet2, 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 5fa63b7..05ec64a 100644
--- a/quic/core/quic_packet_creator.cc
+++ b/quic/core/quic_packet_creator.cc
@@ -479,7 +479,8 @@
}
QUICHE_DCHECK_EQ(nullptr, packet_.encrypted_buffer) << ENDPOINT;
- if (!SerializePacket(std::move(external_buffer), kMaxOutgoingPacketSize)) {
+ if (!SerializePacket(std::move(external_buffer), kMaxOutgoingPacketSize,
+ /*allow_padding=*/true)) {
return;
}
OnSerializedPacket();
@@ -525,6 +526,14 @@
<< ENDPOINT
<< "Attempt to serialize empty ENCRYPTION_INITIAL packet in coalesced "
"packet";
+
+ if (close_connection_if_fail_to_serialzie_coalesced_packet_ &&
+ HasPendingFrames()) {
+ QUIC_BUG(quic_packet_creator_unexpected_queued_frames)
+ << "Unexpected queued frames: " << GetPendingFramesInfo();
+ return 0;
+ }
+
ScopedPacketContextSwitcher switcher(
packet.packet_number -
1, // -1 because serialize packet increase packet number.
@@ -555,7 +564,16 @@
return 0;
}
}
- if (!SerializePacket(QuicOwnedPacketBuffer(buffer, nullptr), buffer_len)) {
+
+ if (close_connection_if_fail_to_serialzie_coalesced_packet_) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(
+ quic_close_connection_if_fail_to_serialzie_coalesced_packet2, 1, 2);
+ }
+
+ if (!SerializePacket(
+ QuicOwnedPacketBuffer(buffer, nullptr), buffer_len,
+ /*allow_padding=*/
+ !close_connection_if_fail_to_serialzie_coalesced_packet_)) {
return 0;
}
const size_t encrypted_length = packet_.encrypted_length;
@@ -788,7 +806,8 @@
}
bool QuicPacketCreator::SerializePacket(QuicOwnedPacketBuffer encrypted_buffer,
- size_t encrypted_buffer_len) {
+ size_t encrypted_buffer_len,
+ bool allow_padding) {
if (packet_.encrypted_buffer != nullptr) {
const std::string error_details =
"Packet's encrypted buffer is not empty before serialization";
@@ -817,11 +836,14 @@
<< EncryptionLevelToString(packet_.encryption_level);
}
- MaybeAddPadding();
+ if (allow_padding) {
+ MaybeAddPadding();
+ }
QUIC_DVLOG(2) << ENDPOINT << "Serializing packet " << header
<< QuicFramesToString(queued_frames_) << " at encryption_level "
- << packet_.encryption_level;
+ << packet_.encryption_level
+ << ", allow_padding:" << allow_padding;
if (!framer_->HasEncrypterOfEncryptionLevel(packet_.encryption_level)) {
// TODO(fayang): Use QUIC_MISSING_WRITE_KEYS for serialization failures due
@@ -1919,6 +1941,13 @@
// Header protection requires a minimum plaintext packet size.
MaybeAddExtraPaddingForHeaderProtection();
+ QUIC_DVLOG(3) << "MaybeAddPadding for " << packet_.packet_number
+ << ": transmission_type:" << packet_.transmission_type
+ << ", fate:" << packet_.fate
+ << ", needs_full_padding_:" << needs_full_padding_
+ << ", pending_padding_bytes_:" << pending_padding_bytes_
+ << ", BytesFree:" << BytesFree();
+
if (!needs_full_padding_ && pending_padding_bytes_ == 0) {
// Do not need padding.
return;
@@ -1951,6 +1980,8 @@
void QuicPacketCreator::AddPendingPadding(QuicByteCount size) {
pending_padding_bytes_ += size;
+ QUIC_DVLOG(3) << "After AddPendingPadding(" << size
+ << "), pending_padding_bytes_:" << pending_padding_bytes_;
}
bool QuicPacketCreator::StreamFrameIsClientHello(
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h
index b32be86..183bf2b 100644
--- a/quic/core/quic_packet_creator.h
+++ b/quic/core/quic_packet_creator.h
@@ -30,6 +30,7 @@
#include "quic/core/quic_packets.h"
#include "quic/core/quic_types.h"
#include "quic/platform/api/quic_export.h"
+#include "quic/platform/api/quic_flags.h"
#include "common/platform/api/quiche_mem_slice.h"
#include "common/quiche_circular_deque.h"
@@ -505,6 +506,10 @@
const QuicSocketAddress& peer_address() const { return packet_.peer_address; }
+ bool close_connection_if_fail_to_serialzie_coalesced_packet() const {
+ return close_connection_if_fail_to_serialzie_coalesced_packet_;
+ }
+
private:
friend class test::QuicPacketCreatorPeer;
@@ -553,9 +558,12 @@
// retransmitted to packet_.retransmittable_frames. All frames must fit into
// a single packet. Returns true on success, otherwise, returns false.
// Fails if |encrypted_buffer| is not large enough for the encrypted packet.
+ //
+ // Padding may be added if |allow_padding|. Currently, the only case where it
+ // is disallowed is reserializing a coalesced initial packet.
ABSL_MUST_USE_RESULT bool SerializePacket(
- QuicOwnedPacketBuffer encrypted_buffer,
- size_t encrypted_buffer_len);
+ QuicOwnedPacketBuffer encrypted_buffer, size_t encrypted_buffer_len,
+ bool allow_padding);
// Called after a new SerialiedPacket is created to call the delegate's
// OnSerializedPacket and reset state.
@@ -713,6 +721,10 @@
// Whether to attempt protecting initial packets with chaos.
bool chaos_protection_enabled_;
+
+ const bool close_connection_if_fail_to_serialzie_coalesced_packet_ =
+ GetQuicReloadableFlag(
+ quic_close_connection_if_fail_to_serialzie_coalesced_packet2);
};
} // namespace quic
diff --git a/quic/test_tools/quic_packet_creator_peer.cc b/quic/test_tools/quic_packet_creator_peer.cc
index d638959..8baef92 100644
--- a/quic/test_tools/quic_packet_creator_peer.cc
+++ b/quic/test_tools/quic_packet_creator_peer.cc
@@ -111,8 +111,9 @@
bool success = creator->AddFrame(frame, NOT_RETRANSMISSION);
QUICHE_DCHECK(success);
}
- const bool success = creator->SerializePacket(
- QuicOwnedPacketBuffer(buffer, nullptr), buffer_len);
+ const bool success =
+ creator->SerializePacket(QuicOwnedPacketBuffer(buffer, nullptr),
+ buffer_len, /*allow_padding=*/true);
QUICHE_DCHECK(success);
SerializedPacket packet = std::move(creator->packet_);
// The caller takes ownership of the QuicEncryptedPacket.
diff --git a/quic/test_tools/simple_session_notifier.cc b/quic/test_tools/simple_session_notifier.cc
index 4eca1f7..d2be965 100644
--- a/quic/test_tools/simple_session_notifier.cc
+++ b/quic/test_tools/simple_session_notifier.cc
@@ -57,8 +57,7 @@
const size_t length = stream_state.bytes_total - stream_state.bytes_sent;
connection_->SetTransmissionType(NOT_RETRANSMISSION);
QuicConsumedData consumed =
- connection_->SendStreamData(id, length, stream_state.bytes_sent,
- stream_state.fin_buffered ? FIN : NO_FIN);
+ connection_->SendStreamData(id, length, stream_state.bytes_sent, state);
QUIC_DVLOG(1) << "consumed: " << consumed;
OnStreamDataConsumed(id, stream_state.bytes_sent, consumed.bytes_consumed,
consumed.fin_consumed);
