While sending PATH_CHALLENGE, make sure QuicConnection flushes probing packet on alternative path before bundling ACK. Protected by quic_reloadable_flag_quic_not_bundle_ack_on_alternative_path. PiperOrigin-RevId: 442816394
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index c5edb1d..44a7830 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -34,6 +34,7 @@ #include "quiche/quic/core/quic_legacy_version_encapsulator.h" #include "quiche/quic/core/quic_packet_creator.h" #include "quiche/quic/core/quic_packet_writer.h" +#include "quiche/quic/core/quic_packets.h" #include "quiche/quic/core/quic_path_validator.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/core/quic_utils.h" @@ -210,6 +211,20 @@ return kCubicBytes; } +bool ContainsNonProbingFrame(const SerializedPacket& packet) { + for (const QuicFrame& frame : packet.nonretransmittable_frames) { + if (!QuicUtils::IsProbingFrame(frame.type)) { + return true; + } + } + for (const QuicFrame& frame : packet.retransmittable_frames) { + if (!QuicUtils::IsProbingFrame(frame.type)) { + return true; + } + } + return false; +} + } // namespace #define ENDPOINT \ @@ -3360,7 +3375,18 @@ WriteResult result(WRITE_STATUS_OK, encrypted_length); QuicSocketAddress send_to_address = packet->peer_address; // Self address is always the default self address on this code path. - bool send_on_current_path = send_to_address == peer_address(); + const bool send_on_current_path = send_to_address == peer_address(); + if (!send_on_current_path && only_send_probing_frames_on_alternative_path_) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_not_bundle_ack_on_alternative_path, 2, 2); + QUIC_BUG_IF(quic_send_non_probing_frames_on_alternative_path, + ContainsNonProbingFrame(*packet)) + << "Packet " << packet->packet_number + << " with non-probing frames was sent on alternative path: " + "nonretransmittable_frames: " + << QuicFramesToString(packet->nonretransmittable_frames) + << " retransmittable_frames: " + << QuicFramesToString(packet->retransmittable_frames); + } switch (fate) { case DISCARD: ++stats_.packets_discarded; @@ -6417,28 +6443,60 @@ return connected_; } if (connection_migration_use_new_cid_) { - { - QuicConnectionId client_cid, server_cid; - FindOnPathConnectionIds(self_address, effective_peer_address, &client_cid, - &server_cid); + if (!only_send_probing_frames_on_alternative_path_) { + { + QuicConnectionId client_cid, server_cid; + FindOnPathConnectionIds(self_address, effective_peer_address, + &client_cid, &server_cid); + QuicPacketCreator::ScopedPeerAddressContext context( + &packet_creator_, peer_address, client_cid, server_cid, + connection_migration_use_new_cid_); + if (writer == writer_) { + ScopedPacketFlusher flusher(this); + // It's on current path, add the PATH_CHALLENGE the same way as other + // frames. This may cause connection to be closed. + packet_creator_.AddPathChallengeFrame(data_buffer); + } else { + std::unique_ptr<SerializedPacket> probing_packet = + packet_creator_.SerializePathChallengeConnectivityProbingPacket( + data_buffer); + QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet), + NO_RETRANSMITTABLE_DATA); + QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address); + WritePacketUsingWriter(std::move(probing_packet), writer, + self_address, peer_address, + /*measure_rtt=*/false); + } + } + return connected_; + } + QUIC_RELOADABLE_FLAG_COUNT_N(quic_not_bundle_ack_on_alternative_path, 1, 2); + QuicConnectionId client_cid, server_cid; + FindOnPathConnectionIds(self_address, effective_peer_address, &client_cid, + &server_cid); + if (writer == writer_) { + ScopedPacketFlusher flusher(this); + { + QuicPacketCreator::ScopedPeerAddressContext context( + &packet_creator_, peer_address, client_cid, server_cid, + connection_migration_use_new_cid_); + // It's using the default writer, add the PATH_CHALLENGE the same way as + // other frames. This may cause connection to be closed. + packet_creator_.AddPathChallengeFrame(data_buffer); + } + } else { + // Switch to the right CID and source/peer addresses. QuicPacketCreator::ScopedPeerAddressContext context( &packet_creator_, peer_address, client_cid, server_cid, connection_migration_use_new_cid_); - if (writer == writer_) { - ScopedPacketFlusher flusher(this); - // It's on current path, add the PATH_CHALLENGE the same way as other - // frames. This may cause connection to be closed. - packet_creator_.AddPathChallengeFrame(data_buffer); - } else { - std::unique_ptr<SerializedPacket> probing_packet = - packet_creator_.SerializePathChallengeConnectivityProbingPacket( - data_buffer); - QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet), - NO_RETRANSMITTABLE_DATA); - QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address); - WritePacketUsingWriter(std::move(probing_packet), writer, self_address, - peer_address, /*measure_rtt=*/false); - } + std::unique_ptr<SerializedPacket> probing_packet = + packet_creator_.SerializePathChallengeConnectivityProbingPacket( + data_buffer); + QUICHE_DCHECK_EQ(IsRetransmittable(*probing_packet), + NO_RETRANSMITTABLE_DATA); + QUICHE_DCHECK_EQ(self_address, alternative_path_.self_address); + WritePacketUsingWriter(std::move(probing_packet), writer, self_address, + peer_address, /*measure_rtt=*/false); } return connected_; }
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index 72b3fdd..6aca00b 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -2251,6 +2251,9 @@ // fixed. absl::optional<QuicWallTime> quic_bug_10511_43_timestamp_; std::string quic_bug_10511_43_error_detail_; + + bool only_send_probing_frames_on_alternative_path_ = + GetQuicReloadableFlag(quic_not_bundle_ack_on_alternative_path); }; } // namespace quic
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index 522c78e..ac4ad7c 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -14037,6 +14037,123 @@ EXPECT_EQ(1u, connection_.GetStats().num_validated_peer_migration); } +// Regression test of b/228645208. +TEST_P(QuicConnectionTest, NoNonProbingFrameOnAlternativePath) { + if (!connection_.connection_migration_use_new_cid()) { + return; + } + + PathProbeTestInit(Perspective::IS_SERVER); + SetClientConnectionId(TestConnectionId(1)); + connection_.CreateConnectionIdManager(); + + QuicConnectionId server_cid0 = connection_.connection_id(); + QuicConnectionId client_cid0 = connection_.client_connection_id(); + QuicConnectionId client_cid1 = TestConnectionId(2); + QuicConnectionId server_cid1; + // Sends new server CID to client. + EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) + .WillOnce( + Invoke([&](const QuicConnectionId& cid) { server_cid1 = cid; })); + EXPECT_CALL(visitor_, SendNewConnectionId(_)); + connection_.MaybeSendConnectionIdToClient(); + // Receives new client CID from client. + QuicNewConnectionIdFrame new_cid_frame; + new_cid_frame.connection_id = client_cid1; + new_cid_frame.sequence_number = 1u; + new_cid_frame.retire_prior_to = 0u; + connection_.OnNewConnectionIdFrame(new_cid_frame); + auto* packet_creator = QuicConnectionPeer::GetPacketCreator(&connection_); + ASSERT_EQ(packet_creator->GetDestinationConnectionId(), client_cid0); + ASSERT_EQ(packet_creator->GetSourceConnectionId(), server_cid0); + + peer_creator_.SetServerConnectionId(server_cid1); + const QuicSocketAddress kNewPeerAddress = + QuicSocketAddress(QuicIpAddress::Loopback4(), /*port=*/23456); + QuicPathFrameBuffer path_challenge_payload{0, 1, 2, 3, 4, 5, 6, 7}; + QuicFrames frames1; + frames1.push_back( + QuicFrame(QuicPathChallengeFrame(0, path_challenge_payload))); + QuicPathFrameBuffer payload; + EXPECT_CALL(*send_algorithm_, + OnPacketSent(_, _, _, _, NO_RETRANSMITTABLE_DATA)) + .Times(AtLeast(1)) + .WillOnce(Invoke([&]() { + EXPECT_EQ(kNewPeerAddress, writer_->last_write_peer_address()); + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); + EXPECT_FALSE(writer_->path_response_frames().empty()); + EXPECT_FALSE(writer_->path_challenge_frames().empty()); + payload = writer_->path_challenge_frames().front().data_buffer; + })); + ProcessFramesPacketWithAddresses(frames1, kSelfAddress, kNewPeerAddress, + ENCRYPTION_FORWARD_SECURE); + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); + EXPECT_TRUE(connection_.HasPendingPathValidation()); + const auto* default_path = QuicConnectionPeer::GetDefaultPath(&connection_); + const auto* alternative_path = + QuicConnectionPeer::GetAlternativePath(&connection_); + EXPECT_EQ(default_path->client_connection_id, client_cid0); + EXPECT_EQ(default_path->server_connection_id, server_cid0); + EXPECT_EQ(alternative_path->client_connection_id, client_cid1); + EXPECT_EQ(alternative_path->server_connection_id, server_cid1); + EXPECT_EQ(packet_creator->GetDestinationConnectionId(), client_cid0); + EXPECT_EQ(packet_creator->GetSourceConnectionId(), server_cid0); + + // Process non-probing packets on the default path. + peer_creator_.SetServerConnectionId(server_cid0); + EXPECT_CALL(visitor_, OnStreamFrame(_)).WillRepeatedly(Invoke([=]() { + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + })); + // Receives packets 3 - 39 to send 19 ACK-only packets, which will force the + // connection to reach |kMaxConsecutiveNonRetransmittablePackets| while + // sending the next ACK. + for (size_t i = 3; i <= 39; ++i) { + ProcessDataPacket(i); + } + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); + + EXPECT_TRUE(connection_.HasPendingAcks()); + QuicTime ack_time = connection_.GetAckAlarm()->deadline(); + QuicTime path_validation_retry_time = + connection_.GetRetryTimeout(kNewPeerAddress, writer_.get()); + // Advance time to simultaneously fire path validation retry and ACK alarms. + clock_.AdvanceTime(std::max(ack_time, path_validation_retry_time) - + clock_.ApproximateNow()); + + // The 20th ACK should bundle with a WINDOW_UPDATE frame. + EXPECT_CALL(visitor_, OnAckNeedsRetransmittableFrame()) + .WillOnce(Invoke([this]() { + connection_.SendControlFrame(QuicFrame(QuicWindowUpdateFrame(1, 0, 0))); + })); + if (GetQuicReloadableFlag(quic_not_bundle_ack_on_alternative_path)) { + EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _)) + .WillOnce(Invoke([&]() { + EXPECT_EQ(kNewPeerAddress, writer_->last_write_peer_address()); + EXPECT_FALSE(writer_->path_challenge_frames().empty()); + // Retry path validation shouldn't bundle ACK. + EXPECT_TRUE(writer_->ack_frames().empty()); + })) + .WillOnce(Invoke([&]() { + EXPECT_EQ(kPeerAddress, writer_->last_write_peer_address()); + EXPECT_FALSE(writer_->ack_frames().empty()); + EXPECT_FALSE(writer_->window_update_frames().empty()); + })); + static_cast<TestAlarmFactory::TestAlarm*>( + QuicPathValidatorPeer::retry_timer( + QuicConnectionPeer::path_validator(&connection_))) + ->Fire(); + } else { + EXPECT_QUIC_BUG(static_cast<TestAlarmFactory::TestAlarm*>( + QuicPathValidatorPeer::retry_timer( + QuicConnectionPeer::path_validator(&connection_))) + ->Fire(), + "quic_bug_12645_2"); + } +} + TEST_P(QuicConnectionTest, ProbedOnAnotherPathAfterPeerIpAddressChangeAtServer) { PathProbeTestInit(Perspective::IS_SERVER);
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h index 794e4f7..092ce91 100644 --- a/quiche/quic/core/quic_flags_list.h +++ b/quiche/quic/core/quic_flags_list.h
@@ -57,6 +57,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_disable_server_blackhole_detection, false) // If true, discard INITIAL packet if the key has been dropped. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_discard_initial_packet_with_key_dropped, true) +// If true, do not bundle ACK while sending PATH_CHALLENGE on alternative path. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_not_bundle_ack_on_alternative_path, true) // If true, do not count bytes sent/received on the alternative path into the bytes sent/received on the default path. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_count_bytes_on_alternative_path_seperately, true) // If true, enable server retransmittable on wire PING.