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