[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.