Use next_connection_id_sequence_number to validate retired CID number as retiring an active CID would not guarantee that a new CID is issued due to CID collision in dispatcher.
Default enabled as a bug fix.
Protected by FLAGS_quic_reloadable_flag_quic_check_retire_cid_with_next_cid_sequence_number.
PiperOrigin-RevId: 487508836
diff --git a/quiche/quic/core/quic_connection_id_manager.cc b/quiche/quic/core/quic_connection_id_manager.cc
index a2a3fbf..4776545 100644
--- a/quiche/quic/core/quic_connection_id_manager.cc
+++ b/quiche/quic/core/quic_connection_id_manager.cc
@@ -340,9 +340,19 @@
const QuicRetireConnectionIdFrame& frame, QuicTime::Delta pto_delay,
std::string* error_detail) {
QUICHE_DCHECK(!active_connection_ids_.empty());
- if (frame.sequence_number > active_connection_ids_.back().second) {
- *error_detail = "To be retired connecton ID is never issued.";
- return IETF_QUIC_PROTOCOL_VIOLATION;
+ if (GetQuicReloadableFlag(
+ quic_check_retire_cid_with_next_cid_sequence_number)) {
+ QUIC_RELOADABLE_FLAG_COUNT(
+ quic_check_retire_cid_with_next_cid_sequence_number);
+ if (frame.sequence_number >= next_connection_id_sequence_number_) {
+ *error_detail = "To be retired connecton ID is never issued.";
+ return IETF_QUIC_PROTOCOL_VIOLATION;
+ }
+ } else {
+ if (frame.sequence_number > active_connection_ids_.back().second) {
+ *error_detail = "To be retired connecton ID is never issued.";
+ return IETF_QUIC_PROTOCOL_VIOLATION;
+ }
}
auto it =
diff --git a/quiche/quic/core/quic_connection_id_manager_test.cc b/quiche/quic/core/quic_connection_id_manager_test.cc
index 9df262c..2dd35bf 100644
--- a/quiche/quic/core/quic_connection_id_manager_test.cc
+++ b/quiche/quic/core/quic_connection_id_manager_test.cc
@@ -6,8 +6,10 @@
#include <cstddef>
+#include "quiche/quic/core/frames/quic_retire_connection_id_frame.h"
#include "quiche/quic/core/quic_connection_id.h"
#include "quiche/quic/core/quic_error_codes.h"
+#include "quiche/quic/platform/api/quic_flags.h"
#include "quiche/quic/platform/api/quic_test.h"
#include "quiche/quic/test_tools/mock_clock.h"
#include "quiche/quic/test_tools/mock_connection_id_generator.h"
@@ -1028,5 +1030,45 @@
cid_manager_.MaybeSendNewConnectionIds();
}
+// Regression test for b/258450534
+TEST_F(QuicSelfIssuedConnectionIdManagerTest,
+ RetireConnectionIdAfterConnectionIdCollisionIsFine) {
+ SetQuicReloadableFlag(quic_check_cid_collision_when_issue_new_cid, true);
+ QuicConnectionId cid0 = initial_connection_id_;
+ QuicConnectionId cid1 = CheckGenerate(cid0);
+ EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid1))
+ .WillOnce(Return(true));
+ EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_))
+ .WillOnce(Return(true));
+ cid_manager_.MaybeSendNewConnectionIds();
+
+ QuicRetireConnectionIdFrame retire_cid_frame(/*control_frame_id=*/0,
+ /*sequence_number=*/1);
+ QuicConnectionId cid2 = CheckGenerate(cid1);
+ // This happens when cid2 is aleady present in the dispatcher map.
+ EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid2))
+ .WillOnce(Return(false));
+ std::string error_details;
+ EXPECT_EQ(
+ cid_manager_.OnRetireConnectionIdFrame(
+ retire_cid_frame, QuicTime::Delta::FromSeconds(1), &error_details),
+ QUIC_NO_ERROR)
+ << error_details;
+
+ if (GetQuicReloadableFlag(
+ quic_check_retire_cid_with_next_cid_sequence_number)) {
+ EXPECT_EQ(
+ cid_manager_.OnRetireConnectionIdFrame(
+ retire_cid_frame, QuicTime::Delta::FromSeconds(1), &error_details),
+ QUIC_NO_ERROR)
+ << error_details;
+ } else {
+ EXPECT_EQ(
+ cid_manager_.OnRetireConnectionIdFrame(
+ retire_cid_frame, QuicTime::Delta::FromSeconds(1), &error_details),
+ IETF_QUIC_PROTOCOL_VIOLATION);
+ }
+}
+
} // namespace
} // namespace quic::test
diff --git a/quiche/quic/core/quic_flags_list.h b/quiche/quic/core/quic_flags_list.h
index 88cd4a0..7f855cb 100644
--- a/quiche/quic/core/quic_flags_list.h
+++ b/quiche/quic/core/quic_flags_list.h
@@ -85,6 +85,8 @@
QUIC_FLAG(quic_reloadable_flag_quic_use_ping_manager2, true)
// If true, use new connection ID in connection migration.
QUIC_FLAG(quic_reloadable_flag_quic_connection_migration_use_new_cid_v2, true)
+// If true, use next_connection_id_sequence_number to validate retired cid number.
+QUIC_FLAG(quic_reloadable_flag_quic_check_retire_cid_with_next_cid_sequence_number, true)
// If true, uses conservative cwnd gain and pacing gain when cwnd gets bootstrapped.
QUIC_FLAG(quic_reloadable_flag_quic_conservative_cwnd_and_pacing_gains, false)
// Instead of assuming an incoming connection ID length for short headers, ask each time.
