gfe-relnote: fix path probe request in IETF QUIC, padded ping with peer address change should not be treated as path probe request. Only a packet contains PATH_CHALLENGE frame is considered as a path probe request. Protected by disabled QUIC version 99.
This CL also changes UpdatePacketContent to no-op in IETF version.
PiperOrigin-RevId: 296970477
Change-Id: Ida8c7e6a686c08508b936081c812cd14869c2137
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc
index 0420637..4cfc95d 100644
--- a/quic/core/quic_connection.cc
+++ b/quic/core/quic_connection.cc
@@ -1191,13 +1191,6 @@
// response.
received_path_challenge_payloads_.push_back(frame.data_buffer);
- // For VERSION 99 we define a "Padded PATH CHALLENGE" to be the same thing
- // as a PADDED PING -- it will start a connectivity check and prevent
- // connection migration. Insofar as the connectivity check and connection
- // migration are concerned, logically the PATH CHALLENGE is the same as the
- // PING, so as a stopgap, tell the FSM that determines whether we have a
- // Padded PING or not that we received a PING.
- UpdatePacketContent(FIRST_FRAME_IS_PING);
should_last_packet_instigate_acks_ = true;
return true;
}
@@ -1214,7 +1207,6 @@
}
// Have received the matching PATH RESPONSE, saved payload no longer valid.
transmitted_connectivity_probe_payload_ = nullptr;
- UpdatePacketContent(FIRST_FRAME_IS_PING);
return true;
}
@@ -1404,6 +1396,7 @@
}
if (IsCurrentPacketConnectivityProbing()) {
+ DCHECK(!version().HasIetfQuicFrames());
++stats_.num_connectivity_probing_received;
}
@@ -1420,6 +1413,7 @@
<< IsCurrentPacketConnectivityProbing();
if (IsCurrentPacketConnectivityProbing()) {
+ DCHECK(!version().HasIetfQuicFrames());
visitor_->OnPacketReceived(last_packet_destination_address_,
last_packet_source_address_,
/*is_connectivity_probe=*/true);
@@ -1436,29 +1430,24 @@
visitor_->OnPacketReceived(last_packet_destination_address_,
last_packet_source_address_,
/*is_connectivity_probe=*/false);
- } else {
- // This node is not a client (is a server) AND the received packet was
- // NOT connectivity-probing. If the packet had PATH CHALLENGES, send
- // appropriate RESPONSE. Then deal with possible peer migration.
- if (VersionHasIetfQuicFrames(transport_version()) &&
- !received_path_challenge_payloads_.empty()) {
- // If a PATH CHALLENGE was in a "Padded PING (or PATH CHALLENGE)"
- // then it is taken care of above. This handles the case where a PATH
- // CHALLENGE appeared someplace else (eg, the peer randomly added a PATH
- // CHALLENGE frame to some other packet.
- // There was at least one PATH CHALLENGE in the received packet,
- // Generate the required PATH RESPONSE.
- SendGenericPathProbePacket(nullptr, last_packet_source_address_,
- /* is_response= */ true);
+ } else if (version().HasIetfQuicFrames() &&
+ !received_path_challenge_payloads_.empty()) {
+ if (current_effective_peer_migration_type_ != NO_CHANGE) {
+ // TODO(b/150095588): change the stats to
+ // num_valid_path_challenge_received.
+ ++stats_.num_connectivity_probing_received;
}
-
- if (last_header_.packet_number == GetLargestReceivedPacket()) {
- direct_peer_address_ = last_packet_source_address_;
- if (current_effective_peer_migration_type_ != NO_CHANGE) {
- // TODO(fayang): When multiple packet number spaces is supported, only
- // start peer migration for the application data.
- StartEffectivePeerMigration(current_effective_peer_migration_type_);
- }
+ // If the packet contains PATH CHALLENGE, send appropriate RESPONSE.
+ // There was at least one PATH CHALLENGE in the received packet,
+ // Generate the required PATH RESPONSE.
+ SendGenericPathProbePacket(nullptr, last_packet_source_address_,
+ /* is_response=*/true);
+ } else if (last_header_.packet_number == GetLargestReceivedPacket()) {
+ direct_peer_address_ = last_packet_source_address_;
+ if (current_effective_peer_migration_type_ != NO_CHANGE) {
+ // TODO(fayang): When multiple packet number spaces is supported, only
+ // start peer migration for the application data.
+ StartEffectivePeerMigration(current_effective_peer_migration_type_);
}
}
@@ -3466,39 +3455,28 @@
<< server_connection_id_;
OwningSerializedPacketPointer probing_packet;
- if (!VersionHasIetfQuicFrames(transport_version())) {
+ if (!version().HasIetfQuicFrames()) {
// Non-IETF QUIC, generate a padded ping regardless of whether this is a
// request or a response.
probing_packet = packet_creator_.SerializeConnectivityProbingPacket();
+ } else if (is_response) {
+ // IETF QUIC path response.
+ // Respond to path probe request using IETF QUIC PATH_RESPONSE frame.
+ probing_packet =
+ packet_creator_.SerializePathResponseConnectivityProbingPacket(
+ received_path_challenge_payloads_,
+ /*is_padded=*/false);
+ received_path_challenge_payloads_.clear();
} else {
- if (is_response) {
- // Respond using IETF QUIC PATH_RESPONSE frame
- if (IsCurrentPacketConnectivityProbing()) {
- // Pad the response if the request was a google connectivity probe
- // (padded).
- probing_packet =
- packet_creator_.SerializePathResponseConnectivityProbingPacket(
- received_path_challenge_payloads_, /* is_padded = */ true);
- received_path_challenge_payloads_.clear();
- } else {
- // Do not pad the response if the path challenge was not a google
- // connectivity probe.
- probing_packet =
- packet_creator_.SerializePathResponseConnectivityProbingPacket(
- received_path_challenge_payloads_,
- /* is_padded = */ false);
- received_path_challenge_payloads_.clear();
- }
- } else {
- // Request using IETF QUIC PATH_CHALLENGE frame
- transmitted_connectivity_probe_payload_ =
- std::make_unique<QuicPathFrameBuffer>();
- probing_packet =
- packet_creator_.SerializePathChallengeConnectivityProbingPacket(
- transmitted_connectivity_probe_payload_.get());
- if (!probing_packet) {
- transmitted_connectivity_probe_payload_ = nullptr;
- }
+ // IETF QUIC path challenge.
+ // Send a path probe request using IETF QUIC PATH_CHALLENGE frame.
+ transmitted_connectivity_probe_payload_ =
+ std::make_unique<QuicPathFrameBuffer>();
+ probing_packet =
+ packet_creator_.SerializePathChallengeConnectivityProbingPacket(
+ transmitted_connectivity_probe_payload_.get());
+ if (!probing_packet) {
+ transmitted_connectivity_probe_payload_ = nullptr;
}
}
@@ -3685,6 +3663,14 @@
}
void QuicConnection::UpdatePacketContent(PacketContent type) {
+ // Packet content is tracked to identify connectivity probe in non-IETF
+ // version, where a connectivity probe is defined as
+ // - a padded PING packet with peer address change received by server,
+ // - a padded PING packet on new path received by client.
+ if (version().HasIetfQuicFrames()) {
+ return;
+ }
+
if (current_packet_content_ == NOT_PADDED_PING) {
// We have already learned the current packet is not a connectivity
// probing packet. Peer migration should have already been started earlier
@@ -3703,11 +3689,9 @@
}
}
- // In Google QUIC we look for a packet with just a PING and PADDING.
- // For IETF QUIC, the packet must consist of just a PATH_CHALLENGE frame,
- // followed by PADDING. If the condition is met, mark things as
- // connectivity-probing, causing later processing to generate the correct
- // response.
+ // In Google QUIC, we look for a packet with just a PING and PADDING.
+ // If the condition is met, mark things as connectivity-probing, causing
+ // later processing to generate the correct response.
if (type == SECOND_FRAME_IS_PADDING &&
current_packet_content_ == FIRST_FRAME_IS_PING) {
current_packet_content_ = SECOND_FRAME_IS_PADDING;
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc
index c17424e..9753ba1 100644
--- a/quic/core/quic_connection_test.cc
+++ b/quic/core/quic_connection_test.cc
@@ -1149,6 +1149,12 @@
QuicSocketAddress peer_address) {
QuicFrames frames;
frames.push_back(QuicFrame(frame));
+ return ProcessFramesPacketWithAddresses(frames, self_address, peer_address);
+ }
+
+ void ProcessFramesPacketWithAddresses(QuicFrames frames,
+ QuicSocketAddress self_address,
+ QuicSocketAddress peer_address) {
QuicPacketCreatorPeer::SetSendVersionInPacket(
&peer_creator_,
QuicPacketCreatorPeer::GetEncryptionLevel(&peer_creator_) <
@@ -1984,7 +1990,7 @@
connection_.active_effective_peer_migration_type());
}
-TEST_P(QuicConnectionTest, ReceivePaddedPingAtServer) {
+TEST_P(QuicConnectionTest, ReceivePathProbeWithNoAddressChangeAtServer) {
set_perspective(Perspective::IS_SERVER);
QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false);
EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective());
@@ -2016,17 +2022,8 @@
// Process a padded PING or PATH CHALLENGE packet with no peer address change
// on server side will be ignored.
- OwningSerializedPacketPointer probing_packet;
- if (VersionHasIetfQuicFrames(version().transport_version)) {
- QuicPathFrameBuffer payload = {
- {0xde, 0xad, 0xbe, 0xef, 0xba, 0xdc, 0x0f, 0xfe}};
- probing_packet =
- QuicPacketCreatorPeer::SerializePathChallengeConnectivityProbingPacket(
- &peer_creator_, &payload);
- } else {
- probing_packet = QuicPacketCreatorPeer::SerializeConnectivityProbingPacket(
- &peer_creator_);
- }
+ OwningSerializedPacketPointer probing_packet = ConstructProbingPacket();
+
std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket(
QuicEncryptedPacket(probing_packet->encrypted_buffer,
probing_packet->encrypted_length),
@@ -2091,7 +2088,11 @@
EXPECT_EQ(0u, connection_.GetStats().packets_discarded);
}
-TEST_P(QuicConnectionTest, ReceiveConnectivityProbingAtServer) {
+// Receive a path probe request at the server side, i.e.,
+// in non-IETF version: receive a padded PING packet with a peer addess change;
+// in IETF version: receive a packet contains PATH CHALLENGE with peer address
+// change.
+TEST_P(QuicConnectionTest, ReceivePathProbingAtServer) {
set_perspective(Perspective::IS_SERVER);
QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false);
EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective());
@@ -2119,8 +2120,13 @@
EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0);
- EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1);
-
+ if (!GetParam().version.HasIetfQuicFrames()) {
+ EXPECT_CALL(visitor_,
+ OnPacketReceived(_, _, /*is_connectivity_probe=*/true))
+ .Times(1);
+ } else {
+ EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0);
+ }
// Process a padded PING packet from a new peer address on server side
// is effectively receiving a connectivity probing.
const QuicSocketAddress kNewPeerAddress =
@@ -2149,7 +2155,90 @@
EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
}
-TEST_P(QuicConnectionTest, ReceiveReorderedConnectivityProbingAtServer) {
+// Receive a padded PING packet with a port change on server side.
+TEST_P(QuicConnectionTest, ReceivePaddedPingWithPortChangeAtServer) {
+ set_perspective(Perspective::IS_SERVER);
+ QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false);
+ EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective());
+
+ // Clear direct_peer_address.
+ QuicConnectionPeer::SetDirectPeerAddress(&connection_, QuicSocketAddress());
+ // Clear effective_peer_address, it is the same as direct_peer_address for
+ // this test.
+ QuicConnectionPeer::SetEffectivePeerAddress(&connection_,
+ QuicSocketAddress());
+ EXPECT_FALSE(connection_.effective_peer_address().IsInitialized());
+
+ QuicFrame frame;
+ if (GetParam().version.UsesCryptoFrames()) {
+ frame = QuicFrame(&crypto_frame_);
+ EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber());
+ } else {
+ frame = QuicFrame(QuicStreamFrame(
+ QuicUtils::GetCryptoStreamId(connection_.transport_version()), false,
+ 0u, quiche::QuicheStringPiece()));
+ EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber());
+ }
+ ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress);
+ EXPECT_EQ(kPeerAddress, connection_.peer_address());
+ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
+
+ if (GetParam().version.HasIetfQuicFrames()) {
+ // In IETF version, a padded PING packet with port change is not taken as
+ // connectivity probe.
+ EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1);
+ EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0);
+ } else {
+ // In non-IETF version, process a padded PING packet from a new peer
+ // address on server side is effectively receiving a connectivity probing.
+ EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0);
+ EXPECT_CALL(visitor_,
+ OnPacketReceived(_, _, /*is_connectivity_probe=*/true))
+ .Times(1);
+ }
+ const QuicSocketAddress kNewPeerAddress =
+ QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456);
+
+ QuicFrames frames;
+ // Write a PING frame, which has no data payload.
+ QuicPingFrame ping_frame;
+ frames.push_back(QuicFrame(ping_frame));
+
+ // Add padding to the rest of the packet.
+ QuicPaddingFrame padding_frame;
+ frames.push_back(QuicFrame(padding_frame));
+
+ uint64_t num_probing_received =
+ connection_.GetStats().num_connectivity_probing_received;
+
+ ProcessFramesPacketWithAddresses(frames, kSelfAddress, kNewPeerAddress);
+
+ if (GetParam().version.HasIetfQuicFrames()) {
+ // Padded PING with port changen is not considered as connectivity probe but
+ // a PORT CHANGE.
+ EXPECT_EQ(num_probing_received,
+ connection_.GetStats().num_connectivity_probing_received);
+ EXPECT_EQ(kNewPeerAddress, connection_.peer_address());
+ EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address());
+ } else {
+ EXPECT_EQ(num_probing_received + 1,
+ connection_.GetStats().num_connectivity_probing_received);
+ EXPECT_EQ(kPeerAddress, connection_.peer_address());
+ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
+ }
+
+ // Process another packet with the old peer address on server side.
+ if (GetParam().version.HasIetfQuicFrames()) {
+ EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1);
+ } else {
+ EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0);
+ }
+ ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress);
+ EXPECT_EQ(kPeerAddress, connection_.peer_address());
+ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
+}
+
+TEST_P(QuicConnectionTest, ReceiveReorderedPathProbingAtServer) {
set_perspective(Perspective::IS_SERVER);
QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false);
EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective());
@@ -2181,7 +2270,13 @@
QuicPacketCreatorPeer::SetPacketNumber(&peer_creator_, 4);
EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0);
- EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1);
+ if (!GetParam().version.HasIetfQuicFrames()) {
+ EXPECT_CALL(visitor_,
+ OnPacketReceived(_, _, /*is_connectivity_probe=*/true))
+ .Times(1);
+ } else {
+ EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0);
+ }
// Process a padded PING packet from a new peer address on server side
// is effectively receiving a connectivity probing, even if a newer packet has
@@ -2233,7 +2328,13 @@
EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0);
- EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1);
+ if (!GetParam().version.HasIetfQuicFrames()) {
+ EXPECT_CALL(visitor_,
+ OnPacketReceived(_, _, /*is_connectivity_probe=*/true))
+ .Times(1);
+ } else {
+ EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0);
+ }
// Process a padded PING packet from a new peer address on server side
// is effectively receiving a connectivity probing.
@@ -2305,7 +2406,13 @@
EXPECT_EQ(kPeerAddress, connection_.effective_peer_address());
}
-TEST_P(QuicConnectionTest, ReceiveConnectivityProbingAtClient) {
+TEST_P(QuicConnectionTest, ReceiveConnectivityProbingResponseAtClient) {
+ // TODO(b/150095484): add test coverage for IETF to verify that client takes
+ // PATH RESPONSE with peer address change as correct validation on the new
+ // path.
+ if (GetParam().version.HasIetfQuicFrames()) {
+ return;
+ }
EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_));
set_perspective(Perspective::IS_CLIENT);
EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective());
@@ -2335,7 +2442,13 @@
// Process a padded PING packet with a different self address on client side
// is effectively receiving a connectivity probing.
EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0);
- EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1);
+ if (!GetParam().version.HasIetfQuicFrames()) {
+ EXPECT_CALL(visitor_,
+ OnPacketReceived(_, _, /*is_connectivity_probe=*/true))
+ .Times(1);
+ } else {
+ EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0);
+ }
const QuicSocketAddress kNewSelfAddress =
QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456);
