Fix quic::GetPacketHeaderSize when QuicVersionHasLongHeaderLengths is false
gfe-relnote: Bugfix in quic::GetPacketHeaderSize protected by gfe2_reloadable_flag_quic_fix_get_packet_header_size
PiperOrigin-RevId: 256043642
Change-Id: I1f943400e15a684b5cb84f2a11cbb294859f669e
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index 749b3b6..3c5daa9 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1141,8 +1141,10 @@
if (level == ENCRYPTION_INITIAL &&
peer_framer_.version().KnowsWhichDecrypterToUse()) {
header.version_flag = true;
- header.retry_token_length_length = VARIABLE_LENGTH_INTEGER_LENGTH_1;
- header.length_length = VARIABLE_LENGTH_INTEGER_LENGTH_2;
+ if (QuicVersionHasLongHeaderLengths(peer_framer_.transport_version())) {
+ header.retry_token_length_length = VARIABLE_LENGTH_INTEGER_LENGTH_1;
+ header.length_length = VARIABLE_LENGTH_INTEGER_LENGTH_2;
+ }
}
if ((GetQuicRestartFlag(quic_do_not_override_connection_id) ||
(level == ENCRYPTION_INITIAL &&
diff --git a/quic/core/quic_crypto_client_stream_test.cc b/quic/core/quic_crypto_client_stream_test.cc
index e0229bc..12fc9a2 100644
--- a/quic/core/quic_crypto_client_stream_test.cc
+++ b/quic/core/quic_crypto_client_stream_test.cc
@@ -315,6 +315,7 @@
}
TEST_F(QuicCryptoClientStreamTest, PreferredVersion) {
+ SetQuicReloadableFlag(quic_fix_get_packet_header_size, true);
// This mimics the case where client receives version negotiation packet, such
// that, the preferred version is different from the packets' version.
connection_ = new PacketSavingConnection(
diff --git a/quic/core/quic_crypto_stream.cc b/quic/core/quic_crypto_stream.cc
index f95a491..395b490 100644
--- a/quic/core/quic_crypto_stream.cc
+++ b/quic/core/quic_crypto_stream.cc
@@ -51,6 +51,16 @@
QuicTransportVersion version,
QuicConnectionId connection_id) {
DCHECK(QuicUtils::IsConnectionIdValidForVersion(connection_id, version));
+ QuicVariableLengthIntegerLength retry_token_length_length =
+ VARIABLE_LENGTH_INTEGER_LENGTH_1;
+ QuicVariableLengthIntegerLength length_length =
+ VARIABLE_LENGTH_INTEGER_LENGTH_2;
+ if (!QuicVersionHasLongHeaderLengths(version) &&
+ GetQuicReloadableFlag(quic_fix_get_packet_header_size)) {
+ retry_token_length_length = VARIABLE_LENGTH_INTEGER_LENGTH_0;
+ length_length = VARIABLE_LENGTH_INTEGER_LENGTH_0;
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_fix_get_packet_header_size, 2, 3);
+ }
return QuicPacketCreator::StreamFramePacketOverhead(
version, static_cast<QuicConnectionIdLength>(connection_id.length()),
PACKET_0BYTE_CONNECTION_ID,
@@ -58,7 +68,7 @@
/*include_diversification_nonce=*/true,
VersionHasIetfInvariantHeader(version) ? PACKET_4BYTE_PACKET_NUMBER
: PACKET_1BYTE_PACKET_NUMBER,
- VARIABLE_LENGTH_INTEGER_LENGTH_1, VARIABLE_LENGTH_INTEGER_LENGTH_2,
+ retry_token_length_length, length_length,
/*offset=*/0);
}
diff --git a/quic/core/quic_crypto_stream_test.cc b/quic/core/quic_crypto_stream_test.cc
index 96b2760..dd4450e 100644
--- a/quic/core/quic_crypto_stream_test.cc
+++ b/quic/core/quic_crypto_stream_test.cc
@@ -512,6 +512,23 @@
EXPECT_TRUE(session_.HasUnackedCryptoData());
}
+// Regression test for bugfix of GetPacketHeaderSize.
+TEST_F(QuicCryptoStreamTest, CryptoMessageFramingOverhead) {
+ SetQuicReloadableFlag(quic_fix_get_packet_header_size, true);
+ for (auto version : AllSupportedTransportVersions()) {
+ SCOPED_TRACE(version);
+ QuicByteCount expected_overhead = 48;
+ if (VersionHasIetfInvariantHeader(version)) {
+ expected_overhead = 52;
+ }
+ if (QuicVersionHasLongHeaderLengths(version)) {
+ expected_overhead = 55;
+ }
+ EXPECT_EQ(expected_overhead, QuicCryptoStream::CryptoMessageFramingOverhead(
+ version, TestConnectionId()));
+ }
+}
+
} // namespace
} // namespace test
} // namespace quic
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc
index b29dacb..00dbaf0 100644
--- a/quic/core/quic_packet_creator.cc
+++ b/quic/core/quic_packet_creator.cc
@@ -84,7 +84,9 @@
false),
pending_padding_bytes_(0),
needs_full_padding_(false),
- can_set_transmission_type_(false) {
+ can_set_transmission_type_(false),
+ fix_get_packet_header_size_(
+ GetQuicReloadableFlag(quic_fix_get_packet_header_size)) {
SetMaxPacketLength(kDefaultMaxPacketSize);
}
@@ -1109,9 +1111,15 @@
framer_->perspective() == Perspective::IS_SERVER;
// IETF QUIC long headers include a length on client 0RTT packets.
QuicVariableLengthIntegerLength length_length =
- framer_->perspective() == Perspective::IS_CLIENT
- ? VARIABLE_LENGTH_INTEGER_LENGTH_2
- : VARIABLE_LENGTH_INTEGER_LENGTH_0;
+ VARIABLE_LENGTH_INTEGER_LENGTH_0;
+ if (framer_->perspective() == Perspective::IS_CLIENT) {
+ length_length = VARIABLE_LENGTH_INTEGER_LENGTH_2;
+ }
+ if (!QuicVersionHasLongHeaderLengths(framer_->transport_version()) &&
+ fix_get_packet_header_size_) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_fix_get_packet_header_size, 3, 3);
+ length_length = VARIABLE_LENGTH_INTEGER_LENGTH_0;
+ }
const size_t packet_header_size = GetPacketHeaderSize(
framer_->transport_version(), GetDestinationConnectionIdLength(),
// Assume CID lengths don't change, but version may be present.
diff --git a/quic/core/quic_packet_creator.h b/quic/core/quic_packet_creator.h
index d2c94ef..2c2fffb 100644
--- a/quic/core/quic_packet_creator.h
+++ b/quic/core/quic_packet_creator.h
@@ -431,6 +431,9 @@
// If true, packet_'s transmission type is only set by
// SetPacketTransmissionType and does not get cleared in ClearPacket.
bool can_set_transmission_type_;
+
+ // Latched value of quic_fix_get_packet_header_size flag.
+ bool fix_get_packet_header_size_;
};
} // namespace quic
diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc
index 3a4978c..802c07b 100644
--- a/quic/core/quic_packet_creator_test.cc
+++ b/quic/core/quic_packet_creator_test.cc
@@ -154,6 +154,7 @@
data_("foo"),
creator_(connection_id_, &client_framer_, &delegate_, &producer_),
serialized_packet_(creator_.NoPacket()) {
+ QuicPacketCreatorPeer::EnableGetPacketHeaderSizeBugFix(&creator_);
EXPECT_CALL(delegate_, GetPacketBuffer()).WillRepeatedly(Return(nullptr));
creator_.SetEncrypter(ENCRYPTION_HANDSHAKE, QuicMakeUnique<NullEncrypter>(
Perspective::IS_CLIENT));
@@ -1809,6 +1810,20 @@
}
}
+// Regression test for bugfix of GetPacketHeaderSize.
+TEST_P(QuicPacketCreatorTest, GetGuaranteedLargestMessagePayload) {
+ QuicTransportVersion version = GetParam().version.transport_version;
+ if (!VersionSupportsMessageFrames(version)) {
+ return;
+ }
+ QuicPacketLength expected_largest_payload = 1319;
+ if (QuicVersionHasLongHeaderLengths(version)) {
+ expected_largest_payload -= 2;
+ }
+ EXPECT_EQ(expected_largest_payload,
+ creator_.GetGuaranteedLargestMessagePayload());
+}
+
TEST_P(QuicPacketCreatorTest, PacketTransmissionType) {
creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
creator_.set_can_set_transmission_type(true);
diff --git a/quic/core/quic_packets.cc b/quic/core/quic_packets.cc
index 7abf303..c6cef78 100644
--- a/quic/core/quic_packets.cc
+++ b/quic/core/quic_packets.cc
@@ -125,13 +125,24 @@
if (VersionHasIetfInvariantHeader(version)) {
if (include_version) {
// Long header.
- return kPacketHeaderTypeSize + kConnectionIdLengthSize +
- destination_connection_id_length + source_connection_id_length +
- (version > QUIC_VERSION_44 ? packet_number_length
- : PACKET_4BYTE_PACKET_NUMBER) +
- kQuicVersionSize +
- (include_diversification_nonce ? kDiversificationNonceSize : 0) +
- retry_token_length_length + retry_token_length + length_length;
+ size_t size = kPacketHeaderTypeSize + kConnectionIdLengthSize +
+ destination_connection_id_length +
+ source_connection_id_length +
+ (version > QUIC_VERSION_44 ? packet_number_length
+ : PACKET_4BYTE_PACKET_NUMBER) +
+ kQuicVersionSize;
+ if (include_diversification_nonce) {
+ size += kDiversificationNonceSize;
+ }
+ DCHECK(QuicVersionHasLongHeaderLengths(version) ||
+ retry_token_length_length + retry_token_length + length_length ==
+ 0);
+ if (QuicVersionHasLongHeaderLengths(version) ||
+ !GetQuicReloadableFlag(quic_fix_get_packet_header_size)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_fix_get_packet_header_size, 1, 3);
+ size += retry_token_length_length + retry_token_length + length_length;
+ }
+ return size;
}
// Short header.
return kPacketHeaderTypeSize + destination_connection_id_length +
diff --git a/quic/quartc/quartc_factory.cc b/quic/quartc/quartc_factory.cc
index 53eb34b..e609d65 100644
--- a/quic/quartc/quartc_factory.cc
+++ b/quic/quartc/quartc_factory.cc
@@ -82,6 +82,9 @@
SetQuicReloadableFlag(quic_bbr_less_probe_rtt, true); // Enable BBR6,7,8.
SetQuicReloadableFlag(quic_unified_iw_options, true); // Enable IWXX opts.
SetQuicReloadableFlag(quic_bbr_flexible_app_limited, true); // Enable BBR9.
+
+ // Fix GetPacketHeaderSize
+ SetQuicReloadableFlag(quic_fix_get_packet_header_size, true);
}
QuicConfig CreateQuicConfig(const QuartcSessionConfig& quartc_session_config) {
diff --git a/quic/test_tools/quic_packet_creator_peer.cc b/quic/test_tools/quic_packet_creator_peer.cc
index e7af2d5..85c7687 100644
--- a/quic/test_tools/quic_packet_creator_peer.cc
+++ b/quic/test_tools/quic_packet_creator_peer.cc
@@ -137,5 +137,11 @@
return creator->framer_;
}
+// static
+void QuicPacketCreatorPeer::EnableGetPacketHeaderSizeBugFix(
+ QuicPacketCreator* creator) {
+ creator->fix_get_packet_header_size_ = true;
+}
+
} // namespace test
} // namespace quic
diff --git a/quic/test_tools/quic_packet_creator_peer.h b/quic/test_tools/quic_packet_creator_peer.h
index e040090..f8f3d59 100644
--- a/quic/test_tools/quic_packet_creator_peer.h
+++ b/quic/test_tools/quic_packet_creator_peer.h
@@ -57,6 +57,7 @@
static EncryptionLevel GetEncryptionLevel(QuicPacketCreator* creator);
static QuicFramer* framer(QuicPacketCreator* creator);
+ static void EnableGetPacketHeaderSizeBugFix(QuicPacketCreator* creator);
};
} // namespace test
