Fix QUIC MTU discovery enablement while `SoftMaxPacketLength` is set. If SoftMaxPacketLength is set to a lower value than current max packet length, `QuicConnection::SetMtuDiscoveryTarget` will incorrectly enable MTU discovery with the first probe packet size being `(SoftMaxPacketLength + MaxPacketLength)/2`, since this is smaller than MaxPacketLength, a successful probe will not be able to increase the max packet length. See the full bug report at https://github.com/google/quiche/issues/107. Protected by FLAGS_quic_reloadable_flag_quic_fix_mtu_discovery. PiperOrigin-RevId: 926136428
diff --git a/quiche/common/quiche_feature_flags_list.h b/quiche/common/quiche_feature_flags_list.h index 4673c26..49825df 100755 --- a/quiche/common/quiche_feature_flags_list.h +++ b/quiche/common/quiche_feature_flags_list.h
@@ -42,6 +42,7 @@ QUICHE_FLAG(bool, quiche_reloadable_flag_quic_enforce_immediate_goaway, false, true, "If true, QUIC will support sending immediate GOAWAYS and will refuse streams above the limit.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_enobufs_blocked, true, true, "If true, ENOBUFS socket errors are reported as socket blocked instead of socket failure.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_fin_before_completed_http_headers, false, true, "If true, close the connection with error if FIN is received before finish receiving the whole HTTP headers.") +QUICHE_FLAG(bool, quiche_reloadable_flag_quic_fix_mtu_discovery, false, true, "If true, fix a externally reported bug in QUIC MTU discovery.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_fix_timeouts, true, true, "If true, postpone setting handshake timeout to infinite to handshake complete.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_include_datagrams_in_willing_to_write, false, false, "If true, checks for queued datagrams when determining if a connection is willing to write.") QUICHE_FLAG(bool, quiche_reloadable_flag_quic_maybe_copy_datagram_frames, false, true, "If true, maybe copy datagram frames in QuicUnackedPacketMap.")
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index 11f2088..26e3eb4 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -5286,7 +5286,13 @@ void QuicConnection::SetMtuDiscoveryTarget(QuicByteCount target) { QUIC_DVLOG(2) << ENDPOINT << "SetMtuDiscoveryTarget: " << target; mtu_discoverer_.Disable(); - mtu_discoverer_.Enable(max_packet_length(), GetLimitedMaxPacketSize(target)); + if (fix_mtu_discovery_) { + QUIC_RELOADABLE_FLAG_COUNT(quic_fix_mtu_discovery); + mtu_discoverer_.Enable(long_term_mtu_, GetLimitedMaxPacketSize(target)); + } else { + mtu_discoverer_.Enable(max_packet_length(), + GetLimitedMaxPacketSize(target)); + } } QuicByteCount QuicConnection::GetLimitedMaxPacketSize(
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index b01c79a..c0b27ad 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -2600,6 +2600,7 @@ GetQuicReloadableFlag(quic_close_on_idle_timeout); // True if spin bit is enabled for this connection. bool spin_bit_enabled_ : 1 = ShouldEnableSpinBit(); + bool fix_mtu_discovery_ : 1 = GetQuicReloadableFlag(quic_fix_mtu_discovery); }; } // namespace quic
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index 162ef31..0d66c4f 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -5338,6 +5338,87 @@ IsError(QUIC_PACKET_WRITE_ERROR)); } +// Repro test for https://github.com/google/quiche/issues/107. +TEST_P(QuicConnectionTest, MtuDiscoveryEnabledWithSoftMaxPacketLength) { + set_perspective(Perspective::IS_CLIENT); + QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); + if (version().IsIetfQuic()) { + QuicConnectionPeer::SetAddressValidated(&connection_); + } + connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); + peer_creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE); + EXPECT_CALL(visitor_, GetHandshakeState()) + .WillRepeatedly(Return(HANDSHAKE_CONFIRMED)); + EXPECT_TRUE(connection_.connected()); + + SetQuicReloadableFlag(quic_enable_mtu_discovery_at_server, false); + + const QuicPacketCount packets_between_probes_base = 5; + set_packets_between_probes_base(packets_between_probes_base); + + QuicConfig config; + QuicTagVector connection_options; + connection_options.push_back(kMTUH); + config.SetClientConnectionOptions(connection_options); + + EXPECT_CALL(*send_algorithm_, SetFromConfig(_, _)); + EXPECT_CALL(*send_algorithm_, EnableECT1()).WillRepeatedly(Return(false)); + EXPECT_CALL(*send_algorithm_, EnableECT0()).WillRepeatedly(Return(false)); + EXPECT_CALL(*send_algorithm_, PacingRate(_)) + .WillRepeatedly(Return(QuicBandwidth::Infinite())); + + const QuicByteCount kOriginalMaxPacketSize = connection_.max_packet_length(); + + // Simulate packet coalescing active by setting soft max packet length to + // 1015, which is lower than kOriginalMaxPacketSize. + ASSERT_LT(1015, kOriginalMaxPacketSize); + creator_->SetSoftMaxPacketLength(1015); + + // This enables MTU discovery. + connection_.SetFromConfig(config); + + QuicPacketCreatorPeer::RemoveSoftMaxPacketLength(creator_); + + // Send enough packets so that the next one triggers path MTU discovery. + for (QuicPacketCount i = 0; i < packets_between_probes_base - 1; i++) { + SendStreamDataToPeer(3, ".", i, NO_FIN, nullptr); + ASSERT_FALSE(connection_.GetMtuDiscoveryAlarm()->IsSet()); + } + + // Trigger the probe. + SendStreamDataToPeer(3, "!", packets_between_probes_base - 1, NO_FIN, + nullptr); + ASSERT_TRUE(connection_.GetMtuDiscoveryAlarm()->IsSet()); + QuicByteCount probe_size; + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)) + .WillOnce(SaveArg<3>(&probe_size)); + connection_.GetMtuDiscoveryAlarm()->Fire(); + + if (GetQuicReloadableFlag(quic_fix_mtu_discovery)) { + // Probe size should be larger than the original max packet length. + EXPECT_GT(probe_size, kOriginalMaxPacketSize); + } else { + EXPECT_LT(probe_size, kOriginalMaxPacketSize); + } + + const QuicPacketNumber probe_packet_number = + FirstSendingPacketNumber() + packets_between_probes_base; + ASSERT_EQ(probe_packet_number, creator_->packet_number()); + + // Acknowledge the probe packet, to trigger OnPathMtuIncreased(). + QuicAckFrame probe_ack = InitAckFrame(probe_packet_number); + EXPECT_CALL(*send_algorithm_, OnCongestionEvent(true, _, _, _, _, _, _)) + .Times(AnyNumber()); + ProcessAckPacket(&probe_ack); + + if (GetQuicReloadableFlag(quic_fix_mtu_discovery)) { + // The max packet length should be increased. + EXPECT_GT(connection_.max_packet_length(), kOriginalMaxPacketSize); + } else { + EXPECT_EQ(connection_.max_packet_length(), kOriginalMaxPacketSize); + } +} + // After a successful MTU probe, one and only one write error should be ignored // if it happened in QuicConnection::FlushPacket. TEST_P(QuicConnectionTest,
diff --git a/quiche/quic/test_tools/quic_packet_creator_peer.cc b/quiche/quic/test_tools/quic_packet_creator_peer.cc index d96c633..40599b1 100644 --- a/quiche/quic/test_tools/quic_packet_creator_peer.cc +++ b/quiche/quic/test_tools/quic_packet_creator_peer.cc
@@ -154,5 +154,11 @@ return creator.append_scone_indicator_; } +// static +bool QuicPacketCreatorPeer::RemoveSoftMaxPacketLength( + QuicPacketCreator* creator) { + return creator->RemoveSoftMaxPacketLength(); +} + } // namespace test } // namespace quic
diff --git a/quiche/quic/test_tools/quic_packet_creator_peer.h b/quiche/quic/test_tools/quic_packet_creator_peer.h index 31b0070..8fa9226 100644 --- a/quiche/quic/test_tools/quic_packet_creator_peer.h +++ b/quiche/quic/test_tools/quic_packet_creator_peer.h
@@ -56,6 +56,7 @@ static QuicFrames& QueuedFrames(QuicPacketCreator* creator); static void SetRandom(QuicPacketCreator* creator, QuicRandom* random); static bool WillAttachSconeIndicator(const QuicPacketCreator& creator); + static bool RemoveSoftMaxPacketLength(QuicPacketCreator* creator); }; } // namespace test