Fix 2 QUIC reverse path validation related GFE_BUG.
Flush queued frames when PathValidator::retry_timer_ fires.
Cancel retransmission alarm after resetting congestion controller.
Protected by quic_reloadable_flag_quic_server_reverse_validate_new_path2.
PiperOrigin-RevId: 362972561
Change-Id: I49cd49615a1afb4f9b0f4ceb04d9c41ef2338a35
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 76bc891..30387ba 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -375,7 +375,7 @@
validate_client_addresses_(
framer_.version().HasIetfQuicFrames() && use_path_validator_ &&
count_bytes_on_alternative_path_separately_ &&
- GetQuicReloadableFlag(quic_server_reverse_validate_new_path)) {
+ GetQuicReloadableFlag(quic_server_reverse_validate_new_path2)) {
QUIC_BUG_IF_V2(quic_bug_12714_1,
!start_peer_migration_earlier_ && send_path_response_);
@@ -1677,6 +1677,7 @@
if (!validate_client_addresses_) {
return OnPathChallengeFrameInternal(frame);
}
+ QUIC_CODE_COUNT_N(quic_server_reverse_validate_new_path2, 1, 5);
{
// UpdatePacketStateAndReplyPathChallenge() may start reverse path
// validation, if so bundle the PATH_CHALLENGE together with the
@@ -4961,6 +4962,7 @@
if (!validate_client_addresses_) {
return;
}
+ QUIC_CODE_COUNT_N(quic_server_reverse_validate_new_path2, 2, 5);
if (debug_visitor_ != nullptr) {
const QuicTime now = clock_->ApproximateNow();
if (now >= stats_.handshake_completion_time) {
@@ -5005,6 +5007,7 @@
return;
}
+ QUIC_CODE_COUNT_N(quic_server_reverse_validate_new_path2, 3, 5);
if (type == NO_CHANGE) {
UpdatePeerAddress(last_packet_source_address_);
QUIC_BUG_V2(quic_bug_10511_36)
@@ -5052,14 +5055,7 @@
// congestion controller to initial state first and then change to the one
// on alternative path.
// TODO(danzh) combine these two steps into one after deprecating gQUIC.
- previous_default_path.send_algorithm =
- sent_packet_manager_.OnConnectionMigration(
- /*reset_send_algorithm=*/true);
- // OnConnectionMigration() might have marked in-flight packets to be
- // retransmitted if there is any.
- QUICHE_DCHECK(!sent_packet_manager_.HasInFlightPackets());
- // Stop detections in quiecense.
- blackhole_detector_.StopDetection();
+ previous_default_path.send_algorithm = OnPeerIpAddressChanged();
if (alternative_path_.peer_address.host() ==
current_effective_peer_address.host() &&
@@ -5290,6 +5286,7 @@
alternative_path_ = PathState(last_packet_destination_address_,
current_effective_peer_address);
} else if (!default_path_.validated) {
+ QUIC_CODE_COUNT_N(quic_server_reverse_validate_new_path2, 4, 5);
// Skip reverse path validation because either handshake hasn't
// completed or the connection is validating the default path. Using
// PATH_CHALLENGE to validate alternative client address before
@@ -5303,6 +5300,7 @@
IsHandshakeConfirmed() && !alternative_path_.validated)
<< "No validated peer address to send after handshake comfirmed.";
} else if (!IsReceivedPeerAddressValidated()) {
+ QUIC_CODE_COUNT_N(quic_server_reverse_validate_new_path2, 5, 5);
// Only override alternative path state upon receiving a PATH_CHALLENGE
// from an unvalidated peer address, and the connection isn't validating
// a recent peer migration.
@@ -6192,6 +6190,7 @@
const QuicSocketAddress& /*effective_peer_address*/,
QuicPacketWriter* writer) {
if (writer == writer_) {
+ ScopedPacketFlusher flusher(this);
{
// It's on current path, add the PATH_CHALLENGE the same way as other
// frames.
@@ -6541,10 +6540,7 @@
}
// Revert congestion control context to old state.
- sent_packet_manager_.OnConnectionMigration(true);
- QUICHE_DCHECK(!sent_packet_manager_.HasInFlightPackets());
- // Stop detections in quiecense.
- blackhole_detector_.StopDetection();
+ OnPeerIpAddressChanged();
if (alternative_path_.send_algorithm != nullptr) {
sent_packet_manager_.SetSendAlgorithm(
@@ -6565,5 +6561,22 @@
WriteIfNotBlocked();
}
+std::unique_ptr<SendAlgorithmInterface>
+QuicConnection::OnPeerIpAddressChanged() {
+ QUICHE_DCHECK(validate_client_addresses_);
+ std::unique_ptr<SendAlgorithmInterface> old_send_algorithm =
+ sent_packet_manager_.OnConnectionMigration(
+ /*reset_send_algorithm=*/true);
+ // OnConnectionMigration() should have marked in-flight packets to be
+ // retransmitted if there is any.
+ QUICHE_DCHECK(!sent_packet_manager_.HasInFlightPackets());
+ // OnConnectionMigration() may have changed the retransmission timer, so
+ // re-arm it.
+ SetRetransmissionAlarm();
+ // Stop detections in quiecense.
+ blackhole_detector_.StopDetection();
+ return old_send_algorithm;
+}
+
#undef ENDPOINT // undef for jumbo builds
} // namespace quic
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h
index ec866a6..65338d5 100644
--- a/quic/core/quic_connection.h
+++ b/quic/core/quic_connection.h
@@ -1722,6 +1722,11 @@
virtual std::unique_ptr<QuicSelfIssuedConnectionIdManager>
MakeSelfIssuedConnectionIdManager();
+ // Called on peer IP change or restoring to previous address to reset
+ // congestion window, RTT stats, retransmission timer, etc. Only used in IETF
+ // QUIC.
+ std::unique_ptr<SendAlgorithmInterface> OnPeerIpAddressChanged();
+
// Process NewConnectionIdFrame either sent from peer or synsthesized from
// preferred_address transport parameter.
bool OnNewConnectionIdFrameInner(const QuicNewConnectionIdFrame& frame);
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index d2f7499..aff4029 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1726,6 +1726,7 @@
EXPECT_CALL(visitor_, GetHandshakeState())
.WillRepeatedly(Return(HANDSHAKE_CONFIRMED));
QuicConnectionPeer::SetAddressValidated(&connection_);
+ connection_.OnHandshakeComplete();
// Enable 5 RTO
QuicConfig config;
@@ -1771,6 +1772,7 @@
connection_.SendStreamData3();
EXPECT_EQ(1u, writer_->packets_write_attempts());
EXPECT_TRUE(connection_.BlackholeDetectionInProgress());
+ EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet());
// Process another packet with a different peer address on server side will
// start connection migration.
@@ -1792,6 +1794,7 @@
EXPECT_EQ(IPV6_TO_IPV4_CHANGE,
connection_.active_effective_peer_migration_type());
EXPECT_FALSE(connection_.BlackholeDetectionInProgress());
+ EXPECT_FALSE(connection_.GetRetransmissionAlarm()->IsSet());
EXPECT_EQ(2u, writer_->packets_write_attempts());
EXPECT_FALSE(writer_->path_challenge_frames().empty());
@@ -1979,6 +1982,7 @@
EXPECT_CALL(visitor_, GetHandshakeState())
.WillRepeatedly(Return(HANDSHAKE_CONFIRMED));
QuicConnectionPeer::SetAddressValidated(&connection_);
+ connection_.OnHandshakeComplete();
// Clear direct_peer_address.
QuicConnectionPeer::SetDirectPeerAddress(&connection_, QuicSocketAddress());
@@ -2016,14 +2020,16 @@
QuicFrames frames2;
frames2.push_back(QuicFrame(frame2_));
+ QuicPaddingFrame padding;
+ frames2.push_back(QuicFrame(padding));
ProcessFramesPacketWithAddresses(frames2, kSelfAddress, kNewPeerAddress,
ENCRYPTION_FORWARD_SECURE);
EXPECT_EQ(kNewPeerAddress, connection_.peer_address());
EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address());
EXPECT_EQ(IPV6_TO_IPV4_CHANGE,
connection_.active_effective_peer_migration_type());
- EXPECT_EQ(1u, writer_->packets_write_attempts());
- EXPECT_FALSE(writer_->path_challenge_frames().empty());
+ EXPECT_LT(0u, writer_->packets_write_attempts());
+ EXPECT_TRUE(connection_.HasPendingPathValidation());
EXPECT_NE(connection_.sent_packet_manager().GetSendAlgorithm(),
send_algorithm_);
EXPECT_EQ(kNewPeerAddress, writer_->last_write_peer_address());
@@ -2040,6 +2046,9 @@
EXPECT_EQ(IPV6_TO_IPV4_CHANGE,
connection_.active_effective_peer_migration_type());
+ SendStreamDataToPeer(1, "foo", 0, NO_FIN, nullptr);
+ EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet());
+
// Advance the time so that the reverse path validation times out.
clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(3 * kInitialRttMs));
static_cast<TestAlarmFactory::TestAlarm*>(
@@ -2051,6 +2060,7 @@
EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
EXPECT_EQ(connection_.sent_packet_manager().GetSendAlgorithm(),
send_algorithm_);
+ EXPECT_TRUE(connection_.GetRetransmissionAlarm()->IsSet());
}
TEST_P(QuicConnectionTest, ReceivePathProbeWithNoAddressChangeAtServer) {
@@ -11780,6 +11790,40 @@
EXPECT_TRUE(connection_.HasPendingPathValidation());
}
+// Regression test for b/182571515.
+TEST_P(QuicConnectionTest, PathValidationRetry) {
+ if (!VersionHasIetfQuicFrames(connection_.version().transport_version) ||
+ !connection_.use_path_validator()) {
+ return;
+ }
+ PathProbeTestInit(Perspective::IS_CLIENT);
+
+ EXPECT_CALL(*send_algorithm_, OnPacketSent(_, _, _, _, _))
+ .Times(2u)
+ .WillRepeatedly(Invoke([&]() {
+ EXPECT_EQ(1u, writer_->path_challenge_frames().size());
+ EXPECT_EQ(1u, writer_->padding_frames().size());
+ }));
+ bool success = true;
+ connection_.ValidatePath(
+ std::make_unique<TestQuicPathValidationContext>(
+ connection_.self_address(), connection_.peer_address(),
+ writer_.get()),
+ std::make_unique<TestValidationResultDelegate>(
+ connection_.self_address(), connection_.peer_address(), &success));
+ EXPECT_EQ(1u, writer_->packets_write_attempts());
+ EXPECT_TRUE(connection_.HasPendingPathValidation());
+
+ // Retry after time out.
+ clock_.AdvanceTime(QuicTime::Delta::FromMilliseconds(3 * kInitialRttMs));
+ static_cast<test::MockRandom*>(helper_->GetRandomGenerator())->ChangeValue();
+ static_cast<TestAlarmFactory::TestAlarm*>(
+ QuicPathValidatorPeer::retry_timer(
+ QuicConnectionPeer::path_validator(&connection_)))
+ ->Fire();
+ EXPECT_EQ(2u, writer_->packets_write_attempts());
+}
+
TEST_P(QuicConnectionTest, PathValidationReceivesStatelessReset) {
if (!VersionHasIetfQuicFrames(connection_.version().transport_version) ||
!connection_.use_path_validator()) {
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 1894b24..4dd8816 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -55,7 +55,7 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_path_response2, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_timestamps, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_send_tls_crypto_error_code, true)
-QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_server_reverse_validate_new_path, false)
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_server_reverse_validate_new_path2, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_single_ack_in_packet2, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_start_peer_migration_earlier, true)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_stateless_reset, true)
diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc
index 8b8bfce..9b476b1 100644
--- a/quic/core/quic_packet_creator.cc
+++ b/quic/core/quic_packet_creator.cc
@@ -2055,6 +2055,12 @@
void QuicPacketCreator::AddPathChallengeFrame(
const QuicPathFrameBuffer& payload) {
+ // TODO(danzh) Unify similar checks at several entry points into one in
+ // AddFrame(). Sort out test helper functions and peer class that don't
+ // enforce this check.
+ QUIC_BUG_IF_V2(quic_bug_10752_39, !flusher_attached_)
+ << "Packet flusher is not attached when "
+ "generator tries to write stream data.";
// Write a PATH_CHALLENGE frame, which has a random 8-byte payload.
auto path_challenge_frame = new QuicPathChallengeFrame(0, payload);
QuicFrame frame(path_challenge_frame);
