Use absl::optional<StatelessResetToken> in place of a separate boolean and token on QuicConnection::PathState. This will fix UndefinedBehaviorSanitizer crash in b/204901337 due to QuicConnection::FindMatchingOrNewClientConnectionIdOrToken not always set the value of *stateless_reset_token_received. PiperOrigin-RevId: 407201586
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index fdecddc..7c4f004 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -242,8 +242,7 @@ default_path_(initial_self_address, QuicSocketAddress(), /*client_connection_id=*/EmptyQuicConnectionId(), server_connection_id, - /*stateless_reset_token_received=*/false, - /*stateless_reset_token=*/{}), + /*stateless_reset_token=*/absl::nullopt), active_effective_peer_migration_type_(NO_CHANGE), support_key_update_for_connection_(false), last_packet_decrypted_(false), @@ -615,7 +614,6 @@ no_stop_waiting_frames_ = true; } if (config.HasReceivedStatelessResetToken()) { - default_path_.stateless_reset_token_received = true; default_path_.stateless_reset_token = config.ReceivedStatelessResetToken(); } if (config.HasReceivedAckDelayExponent()) { @@ -1942,7 +1940,6 @@ QUIC_DVLOG(1) << ENDPOINT << "Patch connection ID " << unused_cid_data->connection_id << " to default path"; default_path_.client_connection_id = unused_cid_data->connection_id; - default_path_.stateless_reset_token_received = true; default_path_.stateless_reset_token = unused_cid_data->stateless_reset_token; QUICHE_DCHECK(!packet_creator_.HasPendingFrames()); @@ -1960,7 +1957,6 @@ QUIC_DVLOG(1) << ENDPOINT << "Patch connection ID " << unused_cid_data->connection_id << " to alternative path"; alternative_path_.client_connection_id = unused_cid_data->connection_id; - alternative_path_.stateless_reset_token_received = true; alternative_path_.stateless_reset_token = unused_cid_data->stateless_reset_token; } @@ -2297,9 +2293,9 @@ bool QuicConnection::IsValidStatelessResetToken( const StatelessResetToken& token) const { QUICHE_DCHECK_EQ(perspective_, Perspective::IS_CLIENT); - return default_path_.stateless_reset_token_received && - QuicUtils::AreStatelessResetTokensEqual( - token, default_path_.stateless_reset_token); + return default_path_.stateless_reset_token.has_value() && + QuicUtils::AreStatelessResetTokensEqual( + token, *default_path_.stateless_reset_token); } void QuicConnection::OnAuthenticatedIetfStatelessResetPacket( @@ -2968,25 +2964,19 @@ } void QuicConnection::FindMatchingOrNewClientConnectionIdOrToken( - const PathState& default_path, - const PathState& alternative_path, + const PathState& default_path, const PathState& alternative_path, const QuicConnectionId& server_connection_id, QuicConnectionId* client_connection_id, - bool* stateless_reset_token_received, - StatelessResetToken* stateless_reset_token) { + absl::optional<StatelessResetToken>* stateless_reset_token) { QUICHE_DCHECK(perspective_ == Perspective::IS_SERVER); if (peer_issued_cid_manager_ == nullptr || server_connection_id == default_path.server_connection_id) { *client_connection_id = default_path.client_connection_id; - *stateless_reset_token_received = - default_path.stateless_reset_token_received; *stateless_reset_token = default_path.stateless_reset_token; return; } if (server_connection_id == alternative_path_.server_connection_id) { *client_connection_id = alternative_path.client_connection_id; - *stateless_reset_token_received = - alternative_path.stateless_reset_token_received; *stateless_reset_token = alternative_path.stateless_reset_token; return; } @@ -3001,7 +2991,6 @@ } *client_connection_id = connection_id_data->connection_id; *stateless_reset_token = connection_id_data->stateless_reset_token; - *stateless_reset_token_received = true; } bool QuicConnection::FindOnPathConnectionIds( @@ -5359,17 +5348,15 @@ SetDefaultPathState(std::move(alternative_path_)); } else { QuicConnectionId client_connection_id; - bool stateless_reset_token_received = false; - StatelessResetToken stateless_reset_token; + absl::optional<StatelessResetToken> stateless_reset_token; FindMatchingOrNewClientConnectionIdOrToken( previous_default_path, alternative_path_, last_packet_destination_connection_id_, &client_connection_id, - &stateless_reset_token_received, &stateless_reset_token); - SetDefaultPathState( - PathState(last_received_packet_info_.destination_address, - current_effective_peer_address, client_connection_id, - last_packet_destination_connection_id_, - stateless_reset_token_received, stateless_reset_token)); + &stateless_reset_token); + SetDefaultPathState(PathState( + last_received_packet_info_.destination_address, + current_effective_peer_address, client_connection_id, + last_packet_destination_connection_id_, stateless_reset_token)); // The path is considered validated if its peer IP address matches any // validated path's peer IP address. default_path_.validated = @@ -5577,17 +5564,15 @@ << last_received_packet_info_.destination_address; if (!validate_client_addresses_) { QuicConnectionId client_cid; - bool stateless_reset_token_received = false; - StatelessResetToken stateless_reset_token; + absl::optional<StatelessResetToken> stateless_reset_token; FindMatchingOrNewClientConnectionIdOrToken( default_path_, alternative_path_, last_packet_destination_connection_id_, &client_cid, - &stateless_reset_token_received, &stateless_reset_token); - alternative_path_ = - PathState(last_received_packet_info_.destination_address, - current_effective_peer_address, client_cid, - last_packet_destination_connection_id_, - stateless_reset_token_received, stateless_reset_token); + &stateless_reset_token); + alternative_path_ = PathState( + last_received_packet_info_.destination_address, + current_effective_peer_address, client_cid, + last_packet_destination_connection_id_, stateless_reset_token); } else if (!default_path_.validated) { QUIC_CODE_COUNT_N(quic_server_reverse_validate_new_path3, 4, 6); // Skip reverse path validation because either handshake hasn't @@ -5605,20 +5590,18 @@ } else if (!IsReceivedPeerAddressValidated()) { QUIC_CODE_COUNT_N(quic_server_reverse_validate_new_path3, 5, 6); QuicConnectionId client_connection_id; - bool stateless_reset_token_received; - StatelessResetToken stateless_reset_token; + absl::optional<StatelessResetToken> stateless_reset_token; FindMatchingOrNewClientConnectionIdOrToken( default_path_, alternative_path_, last_packet_destination_connection_id_, &client_connection_id, - &stateless_reset_token_received, &stateless_reset_token); + &stateless_reset_token); // Only override alternative path state upon receiving a PATH_CHALLENGE // from an unvalidated peer address, and the connection isn't validating // a recent peer migration. - alternative_path_ = - PathState(last_received_packet_info_.destination_address, - current_effective_peer_address, client_connection_id, - last_packet_destination_connection_id_, - stateless_reset_token_received, stateless_reset_token); + alternative_path_ = PathState( + last_received_packet_info_.destination_address, + current_effective_peer_address, client_connection_id, + last_packet_destination_connection_id_, stateless_reset_token); should_proactively_validate_peer_address_on_path_challenge_ = true; } } @@ -6376,7 +6359,6 @@ *default_path_cid = unused_connection_id_data->connection_id; default_path_.stateless_reset_token = unused_connection_id_data->stateless_reset_token; - default_path_.stateless_reset_token_received = true; if (perspective_ == Perspective::IS_CLIENT) { packet_creator_.SetServerConnectionId( unused_connection_id_data->connection_id); @@ -6388,8 +6370,6 @@ } if (default_path_and_alternative_path_use_the_same_peer_connection_id) { *alternative_path_cid = *default_path_cid; - alternative_path_.stateless_reset_token_received = - default_path_.stateless_reset_token_received; alternative_path_.stateless_reset_token = default_path_.stateless_reset_token; } else if (!alternative_path_cid->IsEmpty() && @@ -6402,7 +6382,6 @@ *alternative_path_cid = unused_connection_id_data->connection_id; alternative_path_.stateless_reset_token = unused_connection_id_data->stateless_reset_token; - alternative_path_.stateless_reset_token_received = true; } } @@ -6585,7 +6564,6 @@ alternative_path_ = PathState( context->self_address(), context->peer_address(), default_path_.client_connection_id, default_path_.server_connection_id, - default_path_.stateless_reset_token_received, default_path_.stateless_reset_token); } if (path_validator_.HasPendingPathValidation()) { @@ -6611,8 +6589,7 @@ return; } QuicConnectionId client_connection_id, server_connection_id; - StatelessResetToken stateless_reset_token; - bool stateless_reset_token_received = false; + absl::optional<StatelessResetToken> stateless_reset_token; if (self_issued_cid_manager_ != nullptr) { client_connection_id = *self_issued_cid_manager_->ConsumeOneConnectionId(); @@ -6621,13 +6598,11 @@ const auto* connection_id_data = peer_issued_cid_manager_->ConsumeOneUnusedConnectionId(); server_connection_id = connection_id_data->connection_id; - stateless_reset_token_received = true; stateless_reset_token = connection_id_data->stateless_reset_token; } - alternative_path_ = - PathState(context->self_address(), context->peer_address(), - client_connection_id, server_connection_id, - stateless_reset_token_received, stateless_reset_token); + alternative_path_ = PathState(context->self_address(), + context->peer_address(), client_connection_id, + server_connection_id, stateless_reset_token); } path_validator_.StartPathValidation(std::move(context), std::move(result_delegate)); @@ -6723,8 +6698,6 @@ default_path_.server_connection_id = alternative_path_.server_connection_id; default_path_.stateless_reset_token = alternative_path_.stateless_reset_token; - default_path_.stateless_reset_token_received = - alternative_path_.stateless_reset_token_received; return true; } // Client migration is without path validation. @@ -6748,7 +6721,6 @@ const auto* connection_id_data = peer_issued_cid_manager_->ConsumeOneUnusedConnectionId(); default_path_.server_connection_id = connection_id_data->connection_id; - default_path_.stateless_reset_token_received = true; default_path_.stateless_reset_token = connection_id_data->stateless_reset_token; } @@ -6961,12 +6933,12 @@ peer_address = QuicSocketAddress(); client_connection_id = {}; server_connection_id = {}; - stateless_reset_token_received = false; validated = false; bytes_received_before_address_validation = 0; bytes_sent_before_address_validation = 0; send_algorithm = nullptr; rtt_stats = absl::nullopt; + stateless_reset_token.reset(); } QuicConnection::PathState::PathState(PathState&& other) { @@ -6980,7 +6952,6 @@ peer_address = other.peer_address; client_connection_id = other.client_connection_id; server_connection_id = other.server_connection_id; - stateless_reset_token_received = other.stateless_reset_token_received; stateless_reset_token = other.stateless_reset_token; validated = other.validated; bytes_received_before_address_validation =
diff --git a/quic/core/quic_connection.h b/quic/core/quic_connection.h index e27b6cf..1d9bf95 100644 --- a/quic/core/quic_connection.h +++ b/quic/core/quic_connection.h
@@ -1365,14 +1365,12 @@ const QuicSocketAddress& alternative_peer_address, const QuicConnectionId& client_connection_id, const QuicConnectionId& server_connection_id, - bool stateless_reset_token_received, - StatelessResetToken stateless_reset_token) + absl::optional<StatelessResetToken> stateless_reset_token) : self_address(alternative_self_address), peer_address(alternative_peer_address), client_connection_id(client_connection_id), server_connection_id(server_connection_id), - stateless_reset_token(stateless_reset_token), - stateless_reset_token_received(stateless_reset_token_received) {} + stateless_reset_token(stateless_reset_token) {} PathState(PathState&& other); @@ -1386,8 +1384,7 @@ QuicSocketAddress peer_address; QuicConnectionId client_connection_id; QuicConnectionId server_connection_id; - StatelessResetToken stateless_reset_token; - bool stateless_reset_token_received = false; + absl::optional<StatelessResetToken> stateless_reset_token; // True if the peer address has been validated. Address is considered // validated when 1) an address token of the peer address is received and // validated, or 2) a HANDSHAKE packet has been successfully processed on @@ -1524,12 +1521,10 @@ // client connection ID used on default/alternative path. If not, find if // there is an unused connection ID. void FindMatchingOrNewClientConnectionIdOrToken( - const PathState& default_path, - const PathState& alternative_path, + const PathState& default_path, const PathState& alternative_path, const QuicConnectionId& server_connection_id, QuicConnectionId* client_connection_id, - bool* stateless_reset_token_received, - StatelessResetToken* stateless_reset_token); + absl::optional<StatelessResetToken>* stateless_reset_token); // Returns true and sets connection IDs if (self_address, peer_address) // corresponds to either the default path or alternative path. Returns false
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index 946bda9..1e3b70d 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -2318,7 +2318,7 @@ EXPECT_EQ(default_path->client_connection_id, client_cid0); EXPECT_EQ(default_path->server_connection_id, server_cid0); EXPECT_TRUE(alternative_path->server_connection_id.IsEmpty()); - EXPECT_FALSE(alternative_path->stateless_reset_token_received); + EXPECT_FALSE(alternative_path->stateless_reset_token.has_value()); auto* retire_peer_issued_cid_alarm = connection_.GetRetirePeerIssuedConnectionIdAlarm(); ASSERT_TRUE(retire_peer_issued_cid_alarm->IsSet()); @@ -14232,7 +14232,7 @@ // Verify that alternative_path_ is cleared and the peer CID is retired. EXPECT_TRUE(alternative_path->client_connection_id.IsEmpty()); EXPECT_TRUE(alternative_path->server_connection_id.IsEmpty()); - EXPECT_FALSE(alternative_path->stateless_reset_token_received); + EXPECT_FALSE(alternative_path->stateless_reset_token.has_value()); auto* retire_peer_issued_cid_alarm = connection_.GetRetirePeerIssuedConnectionIdAlarm(); ASSERT_TRUE(retire_peer_issued_cid_alarm->IsSet()); @@ -14341,7 +14341,7 @@ EXPECT_EQ(packet_creator->GetSourceConnectionId(), server_cid1); // Verify that alternative_path_ is cleared. EXPECT_TRUE(alternative_path->server_connection_id.IsEmpty()); - EXPECT_FALSE(alternative_path->stateless_reset_token_received); + EXPECT_FALSE(alternative_path->stateless_reset_token.has_value()); // Switch to use the mock send algorithm. send_algorithm_ = new StrictMock<MockSendAlgorithm>(); @@ -14511,12 +14511,11 @@ EXPECT_EQ(default_path->client_connection_id, client_cid1); EXPECT_EQ(default_path->server_connection_id, server_cid1); EXPECT_EQ(default_path->stateless_reset_token, frame1.stateless_reset_token); - EXPECT_TRUE(default_path->stateless_reset_token_received); const auto* alternative_path = QuicConnectionPeer::GetAlternativePath(&connection_); EXPECT_TRUE(alternative_path->client_connection_id.IsEmpty()); EXPECT_TRUE(alternative_path->server_connection_id.IsEmpty()); - EXPECT_FALSE(alternative_path->stateless_reset_token_received); + EXPECT_FALSE(alternative_path->stateless_reset_token.has_value()); ASSERT_EQ(packet_creator->GetDestinationConnectionId(), server_cid1); // Client will retire server connection ID on old default_path. @@ -14587,7 +14586,7 @@ QuicConnectionPeer::GetAlternativePath(&connection_); EXPECT_TRUE(alternative_path->client_connection_id.IsEmpty()); EXPECT_TRUE(alternative_path->server_connection_id.IsEmpty()); - EXPECT_FALSE(alternative_path->stateless_reset_token_received); + EXPECT_FALSE(alternative_path->stateless_reset_token.has_value()); // Client will retire server connection ID on alternative_path. auto* retire_peer_issued_cid_alarm = @@ -14664,7 +14663,6 @@ EXPECT_EQ(default_path->client_connection_id, new_client_connection_id); EXPECT_EQ(default_path->server_connection_id, frame1.connection_id); EXPECT_EQ(default_path->stateless_reset_token, frame1.stateless_reset_token); - EXPECT_TRUE(default_path->stateless_reset_token_received); // Client will retire server connection ID on old default_path. auto* retire_peer_issued_cid_alarm = @@ -15060,7 +15058,7 @@ alternative_path->peer_address = QuicSocketAddress(new_host, 12345); alternative_path->server_connection_id = TestConnectionId(3); ASSERT_TRUE(alternative_path->client_connection_id.IsEmpty()); - ASSERT_FALSE(alternative_path->stateless_reset_token_received); + ASSERT_FALSE(alternative_path->stateless_reset_token.has_value()); QuicNewConnectionIdFrame frame; frame.sequence_number = 1u; @@ -15075,7 +15073,6 @@ ASSERT_EQ(alternative_path->client_connection_id, frame.connection_id); ASSERT_EQ(alternative_path->stateless_reset_token, frame.stateless_reset_token); - ASSERT_TRUE(alternative_path->stateless_reset_token_received); } TEST_P(QuicConnectionTest, PatchMissingClientConnectionIdOntoDefaultPath) { @@ -15103,7 +15100,7 @@ ASSERT_FALSE(default_path->validated); ASSERT_TRUE(default_path->client_connection_id.IsEmpty()); - ASSERT_FALSE(default_path->stateless_reset_token_received); + ASSERT_FALSE(default_path->stateless_reset_token.has_value()); QuicNewConnectionIdFrame frame; frame.sequence_number = 1u; @@ -15117,7 +15114,6 @@ ASSERT_EQ(default_path->client_connection_id, frame.connection_id); ASSERT_EQ(default_path->stateless_reset_token, frame.stateless_reset_token); - ASSERT_TRUE(default_path->stateless_reset_token_received); ASSERT_EQ(packet_creator->GetDestinationConnectionId(), frame.connection_id); }