Do not issue a new connection ID if it has been taken by another connection in the session map. Protected by FLAGS_quic_reloadable_flag_quic_check_cid_collision_when_issue_new_cid. PiperOrigin-RevId: 456851394
diff --git a/quiche/quic/core/quic_connection.cc b/quiche/quic/core/quic_connection.cc index ad7a103..f5bf7be 100644 --- a/quiche/quic/core/quic_connection.cc +++ b/quiche/quic/core/quic_connection.cc
@@ -6398,11 +6398,12 @@ return connected_; } -void QuicConnection::OnNewConnectionIdIssued( +bool QuicConnection::MaybeReserveConnectionId( const QuicConnectionId& connection_id) { if (perspective_ == Perspective::IS_SERVER) { - visitor_->OnServerConnectionIdIssued(connection_id); + return visitor_->MaybeReserveConnectionId(connection_id); } + return true; } void QuicConnection::OnSelfIssuedConnectionIdRetired(
diff --git a/quiche/quic/core/quic_connection.h b/quiche/quic/core/quic_connection.h index 87f9c14..db2d320 100644 --- a/quiche/quic/core/quic_connection.h +++ b/quiche/quic/core/quic_connection.h
@@ -164,8 +164,9 @@ // Called to send a RETIRE_CONNECTION_ID frame. virtual void SendRetireConnectionId(uint64_t sequence_number) = 0; - // Called when server starts to use a server issued connection ID. - virtual void OnServerConnectionIdIssued( + // Called when server starts to use a server issued connection ID. Returns + // true if this connection ID hasn't been used by another connection. + virtual bool MaybeReserveConnectionId( const QuicConnectionId& server_connection_id) = 0; // Called when server stops to use a server issued connection ID. @@ -718,7 +719,7 @@ // QuicConnectionIdManagerVisitorInterface void OnPeerIssuedConnectionIdRetired() override; bool SendNewConnectionId(const QuicNewConnectionIdFrame& frame) override; - void OnNewConnectionIdIssued(const QuicConnectionId& connection_id) override; + bool MaybeReserveConnectionId(const QuicConnectionId& connection_id) override; void OnSelfIssuedConnectionIdRetired( const QuicConnectionId& connection_id) override;
diff --git a/quiche/quic/core/quic_connection_id_manager.cc b/quiche/quic/core/quic_connection_id_manager.cc index 46ff85f..17f7a72 100644 --- a/quiche/quic/core/quic_connection_id_manager.cc +++ b/quiche/quic/core/quic_connection_id_manager.cc
@@ -10,6 +10,7 @@ #include "quiche/quic/core/quic_connection_id.h" #include "quiche/quic/core/quic_error_codes.h" #include "quiche/quic/core/quic_utils.h" +#include "quiche/quic/platform/api/quic_flag_utils.h" #include "quiche/common/platform/api/quiche_logging.h" namespace quic { @@ -298,14 +299,28 @@ return QuicUtils::CreateReplacementConnectionId(old_connection_id); } -QuicNewConnectionIdFrame -QuicSelfIssuedConnectionIdManager::IssueNewConnectionId() { +absl::optional<QuicNewConnectionIdFrame> +QuicSelfIssuedConnectionIdManager::MaybeIssueNewConnectionId() { + const bool check_cid_collision_when_issue_new_cid = + GetQuicReloadableFlag(quic_check_cid_collision_when_issue_new_cid); + QuicConnectionId new_cid = GenerateNewConnectionId(last_connection_id_); + if (check_cid_collision_when_issue_new_cid) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_check_cid_collision_when_issue_new_cid, 1, + 2); + if (!visitor_->MaybeReserveConnectionId(new_cid)) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_check_cid_collision_when_issue_new_cid, + 2, 2); + return {}; + } + } QuicNewConnectionIdFrame frame; - frame.connection_id = GenerateNewConnectionId(last_connection_id_); + frame.connection_id = new_cid; frame.sequence_number = next_connection_id_sequence_number_++; frame.stateless_reset_token = QuicUtils::GenerateStatelessResetToken(frame.connection_id); - visitor_->OnNewConnectionIdIssued(frame.connection_id); + if (!check_cid_collision_when_issue_new_cid) { + visitor_->MaybeReserveConnectionId(frame.connection_id); + } active_connection_ids_.emplace_back(frame.connection_id, frame.sequence_number); frame.retire_prior_to = active_connection_ids_.front().second; @@ -313,10 +328,10 @@ return frame; } -QuicNewConnectionIdFrame -QuicSelfIssuedConnectionIdManager::IssueNewConnectionIdForPreferredAddress() { - QuicNewConnectionIdFrame frame = IssueNewConnectionId(); - QUICHE_DCHECK_EQ(frame.sequence_number, 1u); +absl::optional<QuicNewConnectionIdFrame> QuicSelfIssuedConnectionIdManager:: + MaybeIssueNewConnectionIdForPreferredAddress() { + absl::optional<QuicNewConnectionIdFrame> frame = MaybeIssueNewConnectionId(); + QUICHE_DCHECK(!frame.has_value() || (frame->sequence_number == 1u)); return frame; } @@ -406,8 +421,12 @@ void QuicSelfIssuedConnectionIdManager::MaybeSendNewConnectionIds() { while (active_connection_ids_.size() < active_connection_id_limit_) { - QuicNewConnectionIdFrame frame = IssueNewConnectionId(); - if (!visitor_->SendNewConnectionId(frame)) { + absl::optional<QuicNewConnectionIdFrame> frame = + MaybeIssueNewConnectionId(); + if (!frame.has_value()) { + break; + } + if (!visitor_->SendNewConnectionId(*frame)) { break; } }
diff --git a/quiche/quic/core/quic_connection_id_manager.h b/quiche/quic/core/quic_connection_id_manager.h index 501310b..7f271e2 100644 --- a/quiche/quic/core/quic_connection_id_manager.h +++ b/quiche/quic/core/quic_connection_id_manager.h
@@ -47,7 +47,7 @@ virtual ~QuicConnectionIdManagerVisitorInterface() = default; virtual void OnPeerIssuedConnectionIdRetired() = 0; virtual bool SendNewConnectionId(const QuicNewConnectionIdFrame& frame) = 0; - virtual void OnNewConnectionIdIssued( + virtual bool MaybeReserveConnectionId( const QuicConnectionId& connection_id) = 0; virtual void OnSelfIssuedConnectionIdRetired( const QuicConnectionId& connection_id) = 0; @@ -130,7 +130,8 @@ virtual ~QuicSelfIssuedConnectionIdManager(); - QuicNewConnectionIdFrame IssueNewConnectionIdForPreferredAddress(); + absl::optional<QuicNewConnectionIdFrame> + MaybeIssueNewConnectionIdForPreferredAddress(); QuicErrorCode OnRetireConnectionIdFrame( const QuicRetireConnectionIdFrame& frame, QuicTime::Delta pto_delay, @@ -164,7 +165,8 @@ private: friend class test::QuicConnectionIdManagerPeer; - QuicNewConnectionIdFrame IssueNewConnectionId(); + // Issue a new connection ID. Can return nullopt. + absl::optional<QuicNewConnectionIdFrame> MaybeIssueNewConnectionId(); // This should be set to the min of: // (1) # of active connection IDs that peer can maintain.
diff --git a/quiche/quic/core/quic_connection_id_manager_test.cc b/quiche/quic/core/quic_connection_id_manager_test.cc index 1c2657d..dbd239f 100644 --- a/quiche/quic/core/quic_connection_id_manager_test.cc +++ b/quiche/quic/core/quic_connection_id_manager_test.cc
@@ -64,8 +64,10 @@ bool SendNewConnectionId(const QuicNewConnectionIdFrame& /*frame*/) override { return false; } - void OnNewConnectionIdIssued( - const QuicConnectionId& /*connection_id*/) override {} + bool MaybeReserveConnectionId(const QuicConnectionId&) override { + return false; + } + void OnSelfIssuedConnectionIdRetired( const QuicConnectionId& /*connection_id*/) override {} @@ -522,7 +524,7 @@ MOCK_METHOD(bool, SendNewConnectionId, (const QuicNewConnectionIdFrame& frame), (override)); - MOCK_METHOD(void, OnNewConnectionIdIssued, + MOCK_METHOD(bool, MaybeReserveConnectionId, (const QuicConnectionId& connection_id), (override)); MOCK_METHOD(void, OnSelfIssuedConnectionIdRetired, (const QuicConnectionId& connection_id), (override)); @@ -568,7 +570,8 @@ QuicConnectionId cid5 = cid_manager_.GenerateNewConnectionId(cid4); // Sends CID #1 to peer. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid1)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid1)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid1, 1u, 0u))) .WillOnce(Return(true)); @@ -578,7 +581,8 @@ // Peer retires CID #0; // Sends CID #2 and asks peer to retire CIDs prior to #1. // Outcome: (#1, #2) are active. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid2)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid2)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid2, 2u, 1u))) .WillOnce(Return(true)); @@ -593,7 +597,8 @@ // Peer retires CID #1; // Sends CID #3 and asks peer to retire CIDs prior to #2. // Outcome: (#2, #3) are active. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid3)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid3)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid3, 3u, 2u))) .WillOnce(Return(true)); @@ -608,7 +613,8 @@ // Peer retires CID #2; // Sends CID #4 and asks peer to retire CIDs prior to #3. // Outcome: (#3, #4) are active. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid4)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid4)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid4, 4u, 3u))) .WillOnce(Return(true)); @@ -623,7 +629,8 @@ // Peer retires CID #3; // Sends CID #5 and asks peer to retire CIDs prior to #4. // Outcome: (#4, #5) are active. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid5)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid5)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid5, 5u, 4u))) .WillOnce(Return(true)); @@ -644,7 +651,8 @@ QuicConnectionId cid4 = cid_manager_.GenerateNewConnectionId(cid3); // Sends CID #1 to peer. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid1)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid1)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid1, 1u, 0u))) .WillOnce(Return(true)); @@ -654,7 +662,8 @@ // Peer retires CID #1; // Sends CID #2 and asks peer to retire CIDs prior to #0. // Outcome: (#0, #2) are active. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid2)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid2)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid2, 2u, 0u))) .WillOnce(Return(true)); @@ -678,7 +687,8 @@ // Peer retires CID #0; // Sends CID #3 and asks peer to retire CIDs prior to #2. // Outcome: (#2, #3) are active. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid3)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid3)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid3, 3u, 2u))) .WillOnce(Return(true)); @@ -693,7 +703,8 @@ // Peer retires CID #3; // Sends CID #4 and asks peer to retire CIDs prior to #2. // Outcome: (#2, #4) are active. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid4)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid4)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(ExpectedNewConnectionIdFrame(cid4, 4u, 2u))) .WillOnce(Return(true)); @@ -720,7 +731,9 @@ QuicConnectionId cid1 = cid_manager_.GenerateNewConnectionId(cid0); QuicConnectionId cid2 = cid_manager_.GenerateNewConnectionId(cid1); QuicConnectionId cid3 = cid_manager_.GenerateNewConnectionId(cid2); - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(_)).Times(3); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(_)) + .Times(3) + .WillRepeatedly(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)) .Times(3) .WillRepeatedly(Return(true)); @@ -777,7 +790,9 @@ QuicConnectionId cid1 = cid_manager_.GenerateNewConnectionId(cid0); QuicConnectionId cid2 = cid_manager_.GenerateNewConnectionId(cid1); QuicConnectionId cid3 = cid_manager_.GenerateNewConnectionId(cid2); - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(_)).Times(3); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(_)) + .Times(3) + .WillRepeatedly(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)) .Times(3) .WillRepeatedly(Return(true)); @@ -834,7 +849,9 @@ QuicConnectionId cid2 = cid_manager_.GenerateNewConnectionId(cid1); QuicConnectionId cid3 = cid_manager_.GenerateNewConnectionId(cid2); QuicConnectionId cid; - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(_)).Times(3); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(_)) + .Times(3) + .WillRepeatedly(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)) .Times(3) .WillRepeatedly(Return(true)); @@ -893,7 +910,8 @@ QuicConnectionId cid1 = cid_manager_.GenerateNewConnectionId(cid0); // CID #1 is sent to peer. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(_)).Times(1); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)) .WillOnce(Return(true)); cid_manager_.MaybeSendNewConnectionIds(); @@ -909,7 +927,8 @@ TEST_F(QuicSelfIssuedConnectionIdManagerTest, ErrorWhenTooManyConnectionIdWaitingToBeRetired) { // CID #0 & #1 are issued. - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(_)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)) .WillOnce(Return(true)); cid_manager_.MaybeSendNewConnectionIds(); @@ -918,7 +937,8 @@ QuicConnectionId last_connection_id = cid_manager_.GenerateNewConnectionId(initial_connection_id_); for (int i = 0; i < 8; ++i) { - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(_)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Return(true)); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)); QuicRetireConnectionIdFrame retire_cid_frame; retire_cid_frame.sequence_number = i; @@ -937,17 +957,60 @@ IsError(QUIC_TOO_MANY_CONNECTION_ID_WAITING_TO_RETIRE)); } +TEST_F(QuicSelfIssuedConnectionIdManagerTest, CannotIssueNewCidDueToVisitor) { + QuicConnectionId cid0 = initial_connection_id_; + QuicConnectionId cid1 = cid_manager_.GenerateNewConnectionId(cid0); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid1)) + .WillOnce(Return(false)); + if (GetQuicReloadableFlag(quic_check_cid_collision_when_issue_new_cid)) { + EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)).Times(0); + } else { + EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)).Times(1); + } + cid_manager_.MaybeSendNewConnectionIds(); +} + +TEST_F(QuicSelfIssuedConnectionIdManagerTest, + CannotIssueNewCidUponRetireConnectionIdDueToVisitor) { + QuicConnectionId cid0 = initial_connection_id_; + QuicConnectionId cid1 = cid_manager_.GenerateNewConnectionId(cid0); + QuicConnectionId cid2 = cid_manager_.GenerateNewConnectionId(cid1); + // CID #0 & #1 are issued. + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid1)) + .WillOnce(Return(true)); + EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)) + .WillOnce(Return(true)); + cid_manager_.MaybeSendNewConnectionIds(); + + // CID #2 is not issued. + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid2)) + .WillOnce(Return(false)); + if (GetQuicReloadableFlag(quic_check_cid_collision_when_issue_new_cid)) { + EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)).Times(0); + } else { + EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)).Times(1); + } + QuicRetireConnectionIdFrame retire_cid_frame; + retire_cid_frame.sequence_number = 1; + ASSERT_THAT(cid_manager_.OnRetireConnectionIdFrame( + retire_cid_frame, pto_delay_, &error_details_), + IsQuicNoError()); +} + TEST_F(QuicSelfIssuedConnectionIdManagerTest, DoNotIssueConnectionIdVoluntarilyIfOneHasIssuedForPerferredAddress) { QuicConnectionId cid0 = initial_connection_id_; QuicConnectionId cid1 = cid_manager_.GenerateNewConnectionId(cid0); - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(cid1)); - ASSERT_THAT(cid_manager_.IssueNewConnectionIdForPreferredAddress(), - ExpectedNewConnectionIdFrame(cid1, 1u, 0u)); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(cid1)) + .WillOnce(Return(true)); + absl::optional<QuicNewConnectionIdFrame> new_cid_frame = + cid_manager_.MaybeIssueNewConnectionIdForPreferredAddress(); + ASSERT_TRUE(new_cid_frame.has_value()); + ASSERT_THAT(*new_cid_frame, ExpectedNewConnectionIdFrame(cid1, 1u, 0u)); EXPECT_THAT(cid_manager_.GetUnretiredConnectionIds(), ElementsAre(cid0, cid1)); - EXPECT_CALL(cid_manager_visitor_, OnNewConnectionIdIssued(_)).Times(0); + EXPECT_CALL(cid_manager_visitor_, MaybeReserveConnectionId(_)).Times(0); EXPECT_CALL(cid_manager_visitor_, SendNewConnectionId(_)).Times(0); cid_manager_.MaybeSendNewConnectionIds(); }
diff --git a/quiche/quic/core/quic_connection_test.cc b/quiche/quic/core/quic_connection_test.cc index 35c30f4..d33a090 100644 --- a/quiche/quic/core/quic_connection_test.cc +++ b/quiche/quic/core/quic_connection_test.cc
@@ -1866,9 +1866,11 @@ QuicConnectionPeer::SetAddressValidated(&connection_); // Sends new server CID to client. - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) - .WillOnce( - Invoke([&](const QuicConnectionId& cid) { server_cid1 = cid; })); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { + server_cid1 = cid; + return true; + })); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.OnHandshakeComplete(); @@ -2057,8 +2059,11 @@ // Sends new server CID to client. QuicConnectionId new_cid; - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) - .WillOnce(Invoke([&](const QuicConnectionId& cid) { new_cid = cid; })); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { + new_cid = cid; + return true; + })); EXPECT_CALL(visitor_, SendNewConnectionId(_)); // Discard INITIAL key. connection_.RemoveEncrypter(ENCRYPTION_INITIAL); @@ -2111,8 +2116,11 @@ // Sends new server CID to client. QuicConnectionId new_cid; - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) - .WillOnce(Invoke([&](const QuicConnectionId& cid) { new_cid = cid; })); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { + new_cid = cid; + return true; + })); EXPECT_CALL(visitor_, SendNewConnectionId(_)); // Discard INITIAL key. connection_.RemoveEncrypter(ENCRYPTION_INITIAL); @@ -2176,9 +2184,11 @@ QuicConnectionId server_cid0 = connection_.connection_id(); QuicConnectionId server_cid1; // Sends new server CID to client. - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) - .WillOnce( - Invoke([&](const QuicConnectionId& cid) { server_cid1 = cid; })); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { + server_cid1 = cid; + return true; + })); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.OnHandshakeComplete(); // Receives new client CID from client. @@ -13315,9 +13325,11 @@ QuicConnectionId client_cid1 = TestConnectionId(2); QuicConnectionId server_cid1; // Sends new server CID to client. - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) - .WillOnce( - Invoke([&](const QuicConnectionId& cid) { server_cid1 = cid; })); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { + server_cid1 = cid; + return true; + })); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.MaybeSendConnectionIdToClient(); // Receives new client CID from client. @@ -13461,9 +13473,11 @@ QuicConnectionId server_cid0 = connection_.connection_id(); QuicConnectionId server_cid1; // Sends new server CID to client. - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) - .WillOnce( - Invoke([&](const QuicConnectionId& cid) { server_cid1 = cid; })); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { + server_cid1 = cid; + return true; + })); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.MaybeSendConnectionIdToClient(); auto* packet_creator = QuicConnectionPeer::GetPacketCreator(&connection_); @@ -13580,9 +13594,11 @@ QuicConnectionId client_cid1 = TestConnectionId(2); QuicConnectionId server_cid1; // Sends new server CID to client. - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) - .WillOnce( - Invoke([&](const QuicConnectionId& cid) { server_cid1 = cid; })); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .WillOnce(Invoke([&](const QuicConnectionId& cid) { + server_cid1 = cid; + return true; + })); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.MaybeSendConnectionIdToClient(); // Receives new client CID from client. @@ -13682,6 +13698,27 @@ } } +TEST_P(QuicConnectionTest, DoNotIssueNewCidIfVisitorSaysNo) { + set_perspective(Perspective::IS_SERVER); + if (!connection_.connection_migration_use_new_cid()) { + return; + } + + connection_.CreateConnectionIdManager(); + + QuicConnectionId server_cid0 = connection_.connection_id(); + QuicConnectionId client_cid1 = TestConnectionId(2); + QuicConnectionId server_cid1; + // Sends new server CID to client. + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)).WillOnce(Return(false)); + if (GetQuicReloadableFlag(quic_check_cid_collision_when_issue_new_cid)) { + EXPECT_CALL(visitor_, SendNewConnectionId(_)).Times(0); + } else { + EXPECT_CALL(visitor_, SendNewConnectionId(_)).Times(1); + } + connection_.MaybeSendConnectionIdToClient(); +} + TEST_P(QuicConnectionTest, ProbedOnAnotherPathAfterPeerIpAddressChangeAtServer) { PathProbeTestInit(Perspective::IS_SERVER); @@ -14217,7 +14254,7 @@ set_perspective(Perspective::IS_SERVER); connection_.CreateConnectionIdManager(); - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.MaybeSendConnectionIdToClient(); @@ -14250,7 +14287,9 @@ QuicRetireConnectionIdFrame frame; frame.sequence_number = 0u; if (connection_.connection_migration_use_new_cid()) { - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)).Times(2); + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) + .Times(2) + .WillRepeatedly(Return(true)); EXPECT_CALL(visitor_, SendNewConnectionId(_)).Times(2); } EXPECT_TRUE(connection_.OnRetireConnectionIdFrame(frame)); @@ -14270,8 +14309,9 @@ set_perspective(Perspective::IS_SERVER); connection_.CreateConnectionIdManager(); QuicConnectionId recorded_cid; - auto cid_recorder = [&recorded_cid](const QuicConnectionId& cid) { + auto cid_recorder = [&recorded_cid](const QuicConnectionId& cid) -> bool { recorded_cid = cid; + return true; }; QuicConnectionId cid0 = connection_id_; QuicConnectionId cid1; @@ -14280,7 +14320,7 @@ EXPECT_EQ(connection_.GetOneActiveServerConnectionId(), cid0); connection_.SetDefaultEncryptionLevel(ENCRYPTION_FORWARD_SECURE); - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) .WillOnce(Invoke(cid_recorder)); EXPECT_CALL(visitor_, SendNewConnectionId(_)); connection_.MaybeSendConnectionIdToClient(); @@ -14311,7 +14351,7 @@ // Packet2 with RetireConnectionId frame trigers sending NewConnectionId // immediately. - EXPECT_CALL(visitor_, OnServerConnectionIdIssued(_)) + EXPECT_CALL(visitor_, MaybeReserveConnectionId(_)) .WillOnce(Invoke(cid_recorder)); EXPECT_CALL(visitor_, SendNewConnectionId(_)); peer_creator_.SetServerConnectionId(cid1);
diff --git a/quiche/quic/core/quic_dispatcher.cc b/quiche/quic/core/quic_dispatcher.cc index 30a93df..883a59d 100644 --- a/quiche/quic/core/quic_dispatcher.cc +++ b/quiche/quic/core/quic_dispatcher.cc
@@ -1099,7 +1099,7 @@ void QuicDispatcher::OnStopSendingReceived( const QuicStopSendingFrame& /*frame*/) {} -void QuicDispatcher::OnNewConnectionIdSent( +bool QuicDispatcher::TryAddNewConnectionId( const QuicConnectionId& server_connection_id, const QuicConnectionId& new_connection_id) { auto it = reference_counted_session_map_.find(server_connection_id); @@ -1108,13 +1108,16 @@ << "Couldn't locate the session that issues the connection ID in " "reference_counted_session_map_. server_connection_id:" << server_connection_id << " new_connection_id: " << new_connection_id; - return; + return false; } // Count new connection ID added to the dispatcher map. QUIC_RELOADABLE_FLAG_COUNT_N(quic_connection_migration_use_new_cid_v2, 6, 6); auto insertion_result = reference_counted_session_map_.insert( std::make_pair(new_connection_id, it->second)); - QUICHE_DCHECK(insertion_result.second); + if (!insertion_result.second) { + QUIC_CODE_COUNT(quic_cid_already_in_session_map); + } + return insertion_result.second; } void QuicDispatcher::OnConnectionIdRetired(
diff --git a/quiche/quic/core/quic_dispatcher.h b/quiche/quic/core/quic_dispatcher.h index 0a103cd..878f6ac 100644 --- a/quiche/quic/core/quic_dispatcher.h +++ b/quiche/quic/core/quic_dispatcher.h
@@ -102,8 +102,9 @@ // QuicSession::Visitor interface implementation (via inheritance of // QuicTimeWaitListManager::Visitor): - // Add the newly issued connection ID to the session map. - void OnNewConnectionIdSent( + // Try to add the new connection ID to the session map. Returns true on + // success. + bool TryAddNewConnectionId( const QuicConnectionId& server_connection_id, const QuicConnectionId& new_connection_id) override;
diff --git a/quiche/quic/core/quic_dispatcher_test.cc b/quiche/quic/core/quic_dispatcher_test.cc index f8c169c..b8b8b44 100644 --- a/quiche/quic/core/quic_dispatcher_test.cc +++ b/quiche/quic/core/quic_dispatcher_test.cc
@@ -170,7 +170,10 @@ active_connection_ids_({connection_id}) {} void AddNewConnectionId(QuicConnectionId id) { - dispatcher_->OnNewConnectionIdSent(active_connection_ids_.back(), id); + if (!dispatcher_->TryAddNewConnectionId(active_connection_ids_.back(), + id)) { + return; + } QuicConnectionPeer::SetServerConnectionId(this, id); active_connection_ids_.push_back(id); } @@ -2243,7 +2246,14 @@ ::testing::PrintToStringParamName()); TEST_P(QuicDispatcherSupportMultipleConnectionIdPerConnectionTest, - OnNewConnectionIdSent) { + FailToAddExistingConnectionId) { + AddConnection1(); + EXPECT_FALSE(dispatcher_->TryAddNewConnectionId(TestConnectionId(1), + TestConnectionId(1))); +} + +TEST_P(QuicDispatcherSupportMultipleConnectionIdPerConnectionTest, + TryAddNewConnectionId) { AddConnection1(); ASSERT_EQ(dispatcher_->NumSessions(), 1u); ASSERT_THAT(session1_, testing::NotNull());
diff --git a/quiche/quic/core/quic_session.cc b/quiche/quic/core/quic_session.cc index d297600..22b05ef 100644 --- a/quiche/quic/core/quic_session.cc +++ b/quiche/quic/core/quic_session.cc
@@ -2098,12 +2098,13 @@ control_frame_manager_.WriteOrBufferRetireConnectionId(sequence_number); } -void QuicSession::OnServerConnectionIdIssued( +bool QuicSession::MaybeReserveConnectionId( const QuicConnectionId& server_connection_id) { if (visitor_) { - visitor_->OnNewConnectionIdSent( + return visitor_->TryAddNewConnectionId( connection_->GetOneActiveServerConnectionId(), server_connection_id); } + return true; } void QuicSession::OnServerConnectionIdRetired(
diff --git a/quiche/quic/core/quic_session.h b/quiche/quic/core/quic_session.h index 7bdcaa9..0da3b6c 100644 --- a/quiche/quic/core/quic_session.h +++ b/quiche/quic/core/quic_session.h
@@ -89,8 +89,8 @@ // peer. virtual void OnStopSendingReceived(const QuicStopSendingFrame& frame) = 0; - // Called when a NewConnectionId frame has been sent. - virtual void OnNewConnectionIdSent( + // Called when on whether a NewConnectionId frame can been sent. + virtual bool TryAddNewConnectionId( const QuicConnectionId& server_connection_id, const QuicConnectionId& new_connection_id) = 0; @@ -145,7 +145,12 @@ void SendAckFrequency(const QuicAckFrequencyFrame& frame) override; void SendNewConnectionId(const QuicNewConnectionIdFrame& frame) override; void SendRetireConnectionId(uint64_t sequence_number) override; - void OnServerConnectionIdIssued( + // Returns true if server_connection_id can be issued. If returns true, + // |visitor_| may establish a mapping from |server_connection_id| to this + // session, if that's not desired, + // OnServerConnectionIdRetired(server_connection_id) can be used to remove the + // mapping. + bool MaybeReserveConnectionId( const QuicConnectionId& server_connection_id) override; void OnServerConnectionIdRetired( const QuicConnectionId& server_connection_id) override;
diff --git a/quiche/quic/test_tools/mock_quic_session_visitor.h b/quiche/quic/test_tools/mock_quic_session_visitor.h index 53c09d8..61dc5d5 100644 --- a/quiche/quic/test_tools/mock_quic_session_visitor.h +++ b/quiche/quic/test_tools/mock_quic_session_visitor.h
@@ -27,7 +27,7 @@ (override)); MOCK_METHOD(void, OnStopSendingReceived, (const QuicStopSendingFrame& frame), (override)); - MOCK_METHOD(void, OnNewConnectionIdSent, + MOCK_METHOD(bool, TryAddNewConnectionId, (const QuicConnectionId& server_connection_id, const QuicConnectionId& new_connection_id), (override));
diff --git a/quiche/quic/test_tools/quic_test_utils.h b/quiche/quic/test_tools/quic_test_utils.h index 0e2c20c..5e4b52e 100644 --- a/quiche/quic/test_tools/quic_test_utils.h +++ b/quiche/quic/test_tools/quic_test_utils.h
@@ -475,7 +475,7 @@ (const QuicNewConnectionIdFrame& frame), (override)); MOCK_METHOD(void, SendRetireConnectionId, (uint64_t sequence_number), (override)); - MOCK_METHOD(void, OnServerConnectionIdIssued, + MOCK_METHOD(bool, MaybeReserveConnectionId, (const QuicConnectionId& server_connection_id), (override)); MOCK_METHOD(void, OnServerConnectionIdRetired, (const QuicConnectionId& server_connection_id), (override));
diff --git a/quiche/quic/test_tools/simulator/quic_endpoint.h b/quiche/quic/test_tools/simulator/quic_endpoint.h index d3741c5..7654d89 100644 --- a/quiche/quic/test_tools/simulator/quic_endpoint.h +++ b/quiche/quic/test_tools/simulator/quic_endpoint.h
@@ -74,8 +74,10 @@ void SendNewConnectionId(const QuicNewConnectionIdFrame& /*frame*/) override { } void SendRetireConnectionId(uint64_t /*sequence_number*/) override {} - void OnServerConnectionIdIssued( - const QuicConnectionId& /*server_connection_id*/) override {} + bool MaybeReserveConnectionId( + const QuicConnectionId& /*server_connection_id*/) override { + return true; + } void OnServerConnectionIdRetired( const QuicConnectionId& /*server_connection_id*/) override {} bool AllowSelfAddressChange() const override;