From 6602c0559ba8e5a6a3c249e98e906b9b42af207b Mon Sep 17 00:00:00 2001 From: Edwin Wu Date: Tue, 30 Jun 2026 19:27:05 -0700 Subject: [PATCH] Fix Use-After-Free in Nearby Connections EndpointManager PiperOrigin-RevId: 940790364 --- .../implementation/endpoint_manager.cc | 33 ++++++++++--------- connections/implementation/endpoint_manager.h | 26 +++++++-------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/connections/implementation/endpoint_manager.cc b/connections/implementation/endpoint_manager.cc index 31b4454b..481eb6f5 100644 --- a/connections/implementation/endpoint_manager.cc +++ b/connections/implementation/endpoint_manager.cc @@ -112,7 +112,8 @@ class EndpointManager::LockedFrameProcessor { void EndpointManager::EndpointChannelLoopRunnable( const std::string& runnable_name, ClientProxy* client, const std::string& endpoint_id, - absl::AnyInvocable(EndpointChannel*)> handler) { + absl::AnyInvocable(std::shared_ptr)> + handler) { // EndpointChannelManager will not let multiple channels exist simultaneously // for the same endpoint_id; it will be closing "old" channels as new ones // come. @@ -143,7 +144,7 @@ void EndpointManager::EndpointChannelLoopRunnable( break; } - ExceptionOr keep_using_channel = handler(channel.get()); + ExceptionOr keep_using_channel = handler(channel); if (!keep_using_channel.ok()) { Exception exception = keep_using_channel.GetException(); @@ -195,7 +196,7 @@ void EndpointManager::EndpointChannelLoopRunnable( } ExceptionOr EndpointManager::TryDecryptFrame( - const ByteArray& data, EndpointChannel* endpoint_channel) { + const ByteArray& data, std::shared_ptr endpoint_channel) { auto start_time = SystemClock::ElapsedRealtime(); while (true) { ExceptionOr decrypted = endpoint_channel->TryDecrypt(data); @@ -222,7 +223,7 @@ ExceptionOr EndpointManager::TryDecryptFrame( ExceptionOr EndpointManager::HandleData( const std::string& endpoint_id, ClientProxy* client, - EndpointChannel* endpoint_channel) { + std::shared_ptr endpoint_channel) { bool try_decrypting = !endpoint_channel->IsEncrypted(); // Read as much as we can from the healthy EndpointChannel - when it is no // longer in good shape (i.e. our read from it throws an Exception), our @@ -317,7 +318,7 @@ ExceptionOr EndpointManager::HandleData( void EndpointManager::ProcessDisconnectionFrame( ClientProxy* client, const std::string& endpoint_id, - EndpointChannel* endpoint_channel, OfflineFrame& frame) { + std::shared_ptr endpoint_channel, OfflineFrame& frame) { if (!client->IsSafeToDisconnectEnabled(endpoint_id)) { LOG(INFO) << "EndpointManager received a DISCONNECTION frame from endpoint " << endpoint_id << " on channel " << endpoint_channel->GetType() @@ -371,9 +372,9 @@ void EndpointManager::ProcessDisconnectionFrame( } ExceptionOr EndpointManager::HandleKeepAlive( - EndpointChannel* endpoint_channel, absl::Duration keep_alive_interval, - absl::Duration keep_alive_timeout, Mutex* keep_alive_waiter_mutex, - ConditionVariable* keep_alive_waiter) { + std::shared_ptr endpoint_channel, + absl::Duration keep_alive_interval, absl::Duration keep_alive_timeout, + Mutex* keep_alive_waiter_mutex, ConditionVariable* keep_alive_waiter) { // Check if it has been too long since we received a frame from our endpoint. absl::Time last_read_time = endpoint_channel->GetLastReadTimestamp(); absl::Duration duration_until_timeout = @@ -578,7 +579,8 @@ void EndpointManager::RegisterEndpoint( endpoint_state.StartEndpointReader([this, client, endpoint_id]() { EndpointChannelLoopRunnable( "Read", client, endpoint_id, - [this, client, endpoint_id](EndpointChannel* channel) { + [this, client, + endpoint_id](std::shared_ptr channel) { return HandleData(endpoint_id, client, channel); }); }); @@ -605,7 +607,7 @@ void EndpointManager::RegisterEndpoint( "KeepAliveManager", client, endpoint_id, [this, keep_alive_interval, keep_alive_timeout, keep_alive_waiter_mutex, - keep_alive_waiter](EndpointChannel* channel) { + keep_alive_waiter](std::shared_ptr channel) { return HandleKeepAlive( channel, keep_alive_interval, keep_alive_timeout, keep_alive_waiter_mutex, keep_alive_waiter); @@ -745,8 +747,8 @@ void EndpointManager::RemoveEndpoint(ClientProxy* client, SafeDisconnectionResult::kSafeDisconnection; // Grab the service ID before we destroy the channel. - EndpointChannel* channel = - channel_manager_->GetChannelForEndpoint(endpoint_id).get(); + std::shared_ptr channel = + channel_manager_->GetChannelForEndpoint(endpoint_id); std::string service_id = channel ? channel->GetServiceId() : std::string(kUnknownServiceId); @@ -784,9 +786,10 @@ void EndpointManager::RemoveEndpoint(ClientProxy* client, RemoveEndpointState(endpoint_id); } -bool EndpointManager::ApplySafeToDisconnect(const std::string& endpoint_id, - EndpointChannel* endpoint_channel, - DisconnectionReason reason) { +bool EndpointManager::ApplySafeToDisconnect( + const std::string& endpoint_id, + std::shared_ptr endpoint_channel, + DisconnectionReason reason) { LOG(INFO) << "[safe-to-disconnect] ApplySafeToDisconnect reason: " << reason; // TODO(b/303544913): clean up the safe-to-disconnect logic bool is_safe_disconnection = false; diff --git a/connections/implementation/endpoint_manager.h b/connections/implementation/endpoint_manager.h index 2250a958..5820c830 100644 --- a/connections/implementation/endpoint_manager.h +++ b/connections/implementation/endpoint_manager.h @@ -229,15 +229,14 @@ class EndpointManager { LockedFrameProcessor GetFrameProcessor( location::nearby::connections::V1Frame::FrameType frame_type); - ExceptionOr HandleData(const std::string& endpoint_id, - ClientProxy* client_proxy, - EndpointChannel* endpoint_channel); + ExceptionOr HandleData( + const std::string& endpoint_id, ClientProxy* client_proxy, + std::shared_ptr endpoint_channel); - ExceptionOr HandleKeepAlive(EndpointChannel* endpoint_channel, - absl::Duration keep_alive_interval, - absl::Duration keep_alive_timeout, - Mutex* keep_alive_waiter_mutex, - ConditionVariable* keep_alive_waiter); + ExceptionOr HandleKeepAlive( + std::shared_ptr endpoint_channel, + absl::Duration keep_alive_interval, absl::Duration keep_alive_timeout, + Mutex* keep_alive_waiter_mutex, ConditionVariable* keep_alive_waiter); // Waits for a given endpoint EndpointChannelLoopRunnable() workers to // terminate. @@ -249,7 +248,8 @@ class EndpointManager { void EndpointChannelLoopRunnable( const std::string& runnable_name, ClientProxy* client_proxy, const std::string& endpoint_id, - absl::AnyInvocable(EndpointChannel*)> handler); + absl::AnyInvocable(std::shared_ptr)> + handler); static void WaitForLatch(const std::string& method_name, CountDownLatch* latch); @@ -265,7 +265,7 @@ class EndpointManager { void RemoveEndpoint(ClientProxy* client, const std::string& endpoint_id, bool notify, DisconnectionReason reason); bool ApplySafeToDisconnect(const std::string& endpoint_id, - EndpointChannel* endpoint_channel, + std::shared_ptr endpoint_channel, DisconnectionReason reason); void WaitForEndpointDisconnectionProcessing(ClientProxy* client, const std::string& service_id, @@ -273,7 +273,7 @@ class EndpointManager { DisconnectionReason reason); void ProcessDisconnectionFrame( ClientProxy* client, const std::string& endpoint_id, - EndpointChannel* endpoint_channel, + std::shared_ptr endpoint_channel, location::nearby::connections::OfflineFrame& frame); CountDownLatch NotifyFrameProcessorsOnEndpointDisconnect( ClientProxy* client, const std::string& service_id, @@ -287,8 +287,8 @@ class EndpointManager { // Executes all jobs sequentially, on a serial_executor_. void RunOnEndpointManagerThread(const std::string& name, Runnable runnable); - ExceptionOr TryDecryptFrame(const ByteArray& data, - EndpointChannel* endpoint_channel); + ExceptionOr TryDecryptFrame( + const ByteArray& data, std::shared_ptr endpoint_channel); EndpointChannelManager* channel_manager_; RecursiveMutex frame_processors_lock_;