[QUIC]Ignore duplicate NEW_CID frame.
Protected by quic_reloadable_flag_quic_ignore_duplicate_new_cid_frame.
PiperOrigin-RevId: 558807416
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc
index cee8ef1..693da98 100644
--- a/quiche/quic/core/quic_connection.cc
+++ b/quiche/quic/core/quic_connection.cc
@@ -394,6 +394,9 @@
AddKnownServerAddress(initial_peer_address);
}
packet_creator_.SetDefaultPeerAddress(initial_peer_address);
+ if (ignore_duplicate_new_cid_frame_) {
+ QUIC_RELOADABLE_FLAG_COUNT(quic_ignore_duplicate_new_cid_frame);
+ }
}
void QuicConnection::InstallInitialCrypters(QuicConnectionId connection_id) {
@@ -1917,28 +1920,32 @@
}
}
-bool QuicConnection::OnNewConnectionIdFrameInner(
+NewConnectionIdResult QuicConnection::OnNewConnectionIdFrameInner(
const QuicNewConnectionIdFrame& frame) {
if (peer_issued_cid_manager_ == nullptr) {
CloseConnection(
IETF_QUIC_PROTOCOL_VIOLATION,
"Receives NEW_CONNECTION_ID while peer uses zero length connection ID",
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
- return false;
+ return NewConnectionIdResult::kProtocolViolation;
}
std::string error_detail;
- QuicErrorCode error =
- peer_issued_cid_manager_->OnNewConnectionIdFrame(frame, &error_detail);
+ bool duplicate_new_connection_id = false;
+ QuicErrorCode error = peer_issued_cid_manager_->OnNewConnectionIdFrame(
+ frame, &error_detail, &duplicate_new_connection_id);
if (error != QUIC_NO_ERROR) {
CloseConnection(error, error_detail,
ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
- return false;
+ return NewConnectionIdResult::kProtocolViolation;
+ }
+ if (duplicate_new_connection_id && ignore_duplicate_new_cid_frame_) {
+ return NewConnectionIdResult::kDuplicateFrame;
}
if (perspective_ == Perspective::IS_SERVER) {
OnClientConnectionIdAvailable();
}
MaybeUpdateAckTimeout();
- return true;
+ return NewConnectionIdResult::kOk;
}
bool QuicConnection::OnNewConnectionIdFrame(
@@ -1956,11 +1963,17 @@
debug_visitor_->OnNewConnectionIdFrame(frame);
}
- if (!OnNewConnectionIdFrameInner(frame)) {
- return false;
- }
- if (multi_port_stats_ != nullptr) {
- MaybeCreateMultiPortPath();
+ NewConnectionIdResult result = OnNewConnectionIdFrameInner(frame);
+ switch (result) {
+ case NewConnectionIdResult::kOk:
+ if (multi_port_stats_ != nullptr) {
+ MaybeCreateMultiPortPath();
+ }
+ break;
+ case NewConnectionIdResult::kProtocolViolation:
+ return false;
+ case NewConnectionIdResult::kDuplicateFrame:
+ break;
}
return true;
}
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h
index 01b9049..053017e 100644
--- a/quiche/quic/core/quic_connection.h
+++ b/quiche/quic/core/quic_connection.h
@@ -1980,7 +1980,8 @@
// Process NewConnectionIdFrame either sent from peer or synsthesized from
// preferred_address transport parameter.
- bool OnNewConnectionIdFrameInner(const QuicNewConnectionIdFrame& frame);
+ NewConnectionIdResult OnNewConnectionIdFrameInner(
+ const QuicNewConnectionIdFrame& frame);
// Called to patch missing client connection ID on default/alternative paths
// when a new client connection ID is received.
@@ -2367,6 +2368,9 @@
const bool ignore_gquic_probing_ =
GetQuicReloadableFlag(quic_ignore_gquic_probing);
+ const bool ignore_duplicate_new_cid_frame_ =
+ GetQuicReloadableFlag(quic_ignore_duplicate_new_cid_frame);
+
RetransmittableOnWireBehavior retransmittable_on_wire_behavior_ = DEFAULT;
// Server addresses that are known to the client.
diff --git a/quiche/quic/core/quic_connection_id_manager.cc b/quiche/quic/core/quic_connection_id_manager.cc
index c4bf3ee..b8524d1 100644
--- a/quiche/quic/core/quic_connection_id_manager.cc
+++ b/quiche/quic/core/quic_connection_id_manager.cc
@@ -125,10 +125,12 @@
}
QuicErrorCode QuicPeerIssuedConnectionIdManager::OnNewConnectionIdFrame(
- const QuicNewConnectionIdFrame& frame, std::string* error_detail) {
+ const QuicNewConnectionIdFrame& frame, std::string* error_detail,
+ bool* is_duplicate_frame) {
if (recent_new_connection_id_sequence_numbers_.Contains(
frame.sequence_number)) {
// This frame has a recently seen sequence number. Ignore.
+ *is_duplicate_frame = true;
return QUIC_NO_ERROR;
}
if (!IsConnectionIdNew(frame)) {
diff --git a/quiche/quic/core/quic_connection_id_manager.h b/quiche/quic/core/quic_connection_id_manager.h
index b8454af..dfc7e11 100644
--- a/quiche/quic/core/quic_connection_id_manager.h
+++ b/quiche/quic/core/quic_connection_id_manager.h
@@ -68,7 +68,8 @@
~QuicPeerIssuedConnectionIdManager();
QuicErrorCode OnNewConnectionIdFrame(const QuicNewConnectionIdFrame& frame,
- std::string* error_detail);
+ std::string* error_detail,
+ bool* is_duplicate_frame);
bool HasUnusedConnectionId() const {
return !unused_connection_id_data_.empty();
diff --git a/quiche/quic/core/quic_connection_id_manager_test.cc b/quiche/quic/core/quic_connection_id_manager_test.cc
index f694cb0..466ba27 100644
--- a/quiche/quic/core/quic_connection_id_manager_test.cc
+++ b/quiche/quic/core/quic_connection_id_manager_test.cc
@@ -104,6 +104,7 @@
QuicPeerIssuedConnectionIdManager peer_issued_cid_manager_;
QuicAlarm* retire_peer_issued_cid_alarm_ = nullptr;
std::string error_details_;
+ bool duplicate_frame_ = false;
};
TEST_F(QuicPeerIssuedConnectionIdManagerTest,
@@ -116,9 +117,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #1 for alternative path.
const QuicConnectionIdData* aternative_connection_id_data =
@@ -148,9 +149,9 @@
frame.retire_prior_to = 1u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #2 for alternative path.
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId();
// Connection migration succeed. Prepares to retire CID #1.
@@ -172,9 +173,9 @@
frame.retire_prior_to = 2u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #3 for alternative path.
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId();
// Connection migration succeed. Prepares to retire CID #2.
@@ -196,9 +197,9 @@
frame.retire_prior_to = 3u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
}
}
@@ -212,9 +213,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #1 for alternative path.
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId();
// Connection migration fails. Prepares to retire CID #1.
@@ -236,9 +237,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #2 for alternative path.
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId();
// Connection migration fails again. Prepares to retire CID #2.
@@ -260,9 +261,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #3 for alternative path.
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId();
// Connection migration succeed. Prepares to retire CID #0.
@@ -286,9 +287,9 @@
frame.retire_prior_to = 3u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- EXPECT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ EXPECT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
EXPECT_FALSE(retire_peer_issued_cid_alarm_->IsSet());
}
}
@@ -304,9 +305,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #1 for alternative path.
// Outcome: (active: #0 #1 unused: None)
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId();
@@ -321,9 +322,9 @@
frame.retire_prior_to = 2u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
}
{
@@ -335,9 +336,9 @@
frame.retire_prior_to = 1u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
}
{
@@ -371,9 +372,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
// Start to use CID #1 for alternative path.
// Outcome: (active: #0 #1 unused: None)
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId();
@@ -387,9 +388,10 @@
cid_manager_visitor_.most_recent_retired_connection_id_sequence_numbers(),
ElementsAre(1u));
// Receives the same frame again. Should be a no-op.
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
+ EXPECT_EQ(true, duplicate_frame_);
EXPECT_THAT(peer_issued_cid_manager_.ConsumeOneUnusedConnectionId(),
testing::IsNull());
}
@@ -405,9 +407,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
}
{
@@ -417,9 +419,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsError(QUIC_CONNECTION_ID_LIMIT_ERROR));
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsError(QUIC_CONNECTION_ID_LIMIT_ERROR));
}
}
@@ -432,9 +434,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
}
{
@@ -444,9 +446,9 @@
frame.retire_prior_to = 1u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(TestConnectionId(2));
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsError(IETF_QUIC_PROTOCOL_VIOLATION));
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsError(IETF_QUIC_PROTOCOL_VIOLATION));
}
}
@@ -459,9 +461,9 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
}
{
@@ -471,9 +473,10 @@
frame.retire_prior_to = 0u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(TestConnectionId(2));
- EXPECT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ EXPECT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
+ EXPECT_EQ(true, duplicate_frame_);
EXPECT_EQ(
peer_issued_cid_manager_.ConsumeOneUnusedConnectionId()->connection_id,
TestConnectionId(1));
@@ -492,9 +495,9 @@
frame.retire_prior_to = i;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsQuicNoError());
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsQuicNoError());
}
// Interval [40, 41) goes over the limit.
@@ -504,9 +507,9 @@
frame.retire_prior_to = 40u;
frame.stateless_reset_token =
QuicUtils::GenerateStatelessResetToken(frame.connection_id);
- ASSERT_THAT(
- peer_issued_cid_manager_.OnNewConnectionIdFrame(frame, &error_details_),
- IsError(IETF_QUIC_PROTOCOL_VIOLATION));
+ ASSERT_THAT(peer_issued_cid_manager_.OnNewConnectionIdFrame(
+ frame, &error_details_, &duplicate_frame_),
+ IsError(IETF_QUIC_PROTOCOL_VIOLATION));
}
TEST_F(QuicPeerIssuedConnectionIdManagerTest, ReplaceConnectionId) {
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc
index ccb65bf..0e213fd 100644
--- a/quiche/quic/core/quic_connection_test.cc
+++ b/quiche/quic/core/quic_connection_test.cc
@@ -13136,7 +13136,6 @@
EXPECT_TRUE(QuicConnectionPeer::IsAlternativePath(
&connection_, kNewSelfAddress, connection_.peer_address()));
EXPECT_TRUE(alt_path->validated);
-
auto stats = connection_.multi_port_stats();
EXPECT_EQ(1, connection_.GetStats().num_path_degrading);
EXPECT_EQ(0, stats->num_multi_port_probe_failures_when_path_degrading);
@@ -13144,6 +13143,12 @@
EXPECT_EQ(kTestRTT,
stats->rtt_stats_when_default_path_degrading.latest_rtt());
+ // Receiving the retransmitted NEW_CID frame now should still have no effect.
+ if (GetQuicReloadableFlag(quic_ignore_duplicate_new_cid_frame)) {
+ EXPECT_CALL(visitor_, CreateContextForMultiPortPath).Times(0);
+ connection_.OnNewConnectionIdFrame(frame);
+ }
+
// When there's no active request, the probing shouldn't happen. But the
// probing context should be saved.
EXPECT_CALL(visitor_, ShouldKeepConnectionAlive()).WillOnce(Return(false));
@@ -15346,6 +15351,7 @@
QuicWindowUpdateFrame window_update_frame;
QuicPathChallengeFrame path_challenge_frame;
QuicNewConnectionIdFrame new_connection_id_frame;
+ new_connection_id_frame.sequence_number = 1u;
QuicRetireConnectionIdFrame retire_connection_id_frame;
retire_connection_id_frame.sequence_number = 1u;
QuicStopSendingFrame stop_sending_frame;
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h
index b586bdc..ce9eed3 100644
--- a/quiche/quic/core/quic_flags_list.h
+++ b/quiche/quic/core/quic_flags_list.h
@@ -31,6 +31,8 @@
QUIC_FLAG(quic_reloadable_flag_quic_enable_mtu_discovery_at_server, false)
// If true, QuicGsoBatchWriter will support release time if it is available and the process has the permission to do so.
QUIC_FLAG(quic_restart_flag_quic_support_release_time_for_gso, false)
+// If true, a duplicate NEW_CID frame will be ignore during QUIC packet processing.
+QUIC_FLAG(quic_reloadable_flag_quic_ignore_duplicate_new_cid_frame, true)
// If true, ack frequency frame can be sent from server to client.
QUIC_FLAG(quic_reloadable_flag_quic_can_send_ack_frequency, true)
// If true, allow client to enable BBRv2 on server via connection option \'B2ON\'.
diff --git a/quiche/quic/core/quic_types.h b/quiche/quic/core/quic_types.h
index c17bf22..55183bb 100644
--- a/quiche/quic/core/quic_types.h
+++ b/quiche/quic/core/quic_types.h
@@ -715,6 +715,13 @@
PACKETS_ACKED_IN_WRONG_PACKET_NUMBER_SPACE,
};
+// Used to return the result of processing a received NEW_CID frame.
+enum class NewConnectionIdResult : uint8_t {
+ kOk,
+ kDuplicateFrame, // Not an error.
+ kProtocolViolation,
+};
+
// Indicates the fate of a serialized packet in WritePacket().
enum SerializedPacketFate : uint8_t {
DISCARD, // Discard the packet.
