From 0be7045809118b637660615875af67a2775ef4e8 Mon Sep 17 00:00:00 2001 From: Anay Wadhera Date: Thu, 25 Jul 2024 15:51:46 -0700 Subject: [PATCH] Fix recursive lock acquiring in OnPayloadTransferUpdate() PiperOrigin-RevId: 656123375 --- sharing/nearby_connections_manager_impl.cc | 58 +++++++++++++++------- sharing/nearby_connections_manager_impl.h | 12 ++++- 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/sharing/nearby_connections_manager_impl.cc b/sharing/nearby_connections_manager_impl.cc index 15435030..f40f37d7 100644 --- a/sharing/nearby_connections_manager_impl.cc +++ b/sharing/nearby_connections_manager_impl.cc @@ -803,9 +803,34 @@ void NearbyConnectionsManagerImpl::ProcessUnknownFilePathsToDelete( } } +std::optional< + std::weak_ptr> +NearbyConnectionsManagerImpl::GetStatusListenerForId(int64_t payload_id) const { + MutexLock lock(&mutex_); + auto listener_it = payload_status_listeners_.find(payload_id); + if (listener_it == payload_status_listeners_.end()) { + return std::nullopt; + } + + return listener_it->second; +} + +NearbyConnectionImpl* NearbyConnectionsManagerImpl::GetConnectionForId( + absl::string_view endpoint_id) const { + MutexLock lock(&mutex_); + auto connection_it = connections_.find(endpoint_id); + if (connection_it == connections_.end()) return nullptr; + return connection_it->second.get(); +} + +void NearbyConnectionsManagerImpl::RemoveStatusListenerForPayloadId( + int64_t payload_id) { + MutexLock lock(&mutex_); + payload_status_listeners_.erase(payload_id); +} + void NearbyConnectionsManagerImpl::OnPayloadTransferUpdate( absl::string_view endpoint_id, const PayloadTransferUpdate& update) { - MutexLock lock(&mutex_); NL_LOG(INFO) << "Received payload transfer update id=" << update.payload_id << ",status=" << PayloadStatusToString(update.status) << ",total=" << update.total_bytes @@ -814,21 +839,22 @@ void NearbyConnectionsManagerImpl::OnPayloadTransferUpdate( // If this is a payload we've registered for, then forward its status to // the PayloadStatusListener if it still exists. We don't need to do // anything more with the payload. - auto listener_it = payload_status_listeners_.find(update.payload_id); - if (listener_it != payload_status_listeners_.end()) { - std::weak_ptr listener = listener_it->second; + std::optional> listener = + GetStatusListenerForId(update.payload_id); + if (listener.has_value()) { switch (update.status) { case PayloadStatus::kInProgress: break; case PayloadStatus::kSuccess: case PayloadStatus::kCanceled: case PayloadStatus::kFailure: - payload_status_listeners_.erase(update.payload_id); + RemoveStatusListenerForPayloadId(update.payload_id); break; } + // Note: The listener might be invalidated, for example, if it is shared // with another payload in the same transfer. - if (auto status_listener = listener.lock()) { + if (auto status_listener = listener->lock()) { status_listener->OnStatusUpdate( std::make_unique(update), GetUpgradedMedium(endpoint_id)); @@ -839,27 +865,25 @@ void NearbyConnectionsManagerImpl::OnPayloadTransferUpdate( // If this is an incoming payload that we have not registered for, then // we'll treat it as a control frame (e.g. IntroductionFrame) and // forward it to the associated NearbyConnection. - auto payload_it = incoming_payloads_.find(update.payload_id); - if (payload_it == incoming_payloads_.end()) return; + auto payload = GetIncomingPayload(update.payload_id); + if (payload == nullptr) return; - if (payload_it->second.content.type != PayloadContent::Type::kBytes) { + if (payload->content.type != PayloadContent::Type::kBytes) { NL_LOG(WARNING) << "Received unknown payload of file type. Cancelling."; - nearby_connections_service_->CancelPayload(kServiceId, payload_it->first, + nearby_connections_service_->CancelPayload(kServiceId, payload->id, [](Status status) {}); - ProcessUnknownFilePathsToDelete( - update.status, payload_it->second.content.type, - payload_it->second.content.file_payload.file.path); + ProcessUnknownFilePathsToDelete(update.status, payload->content.type, + payload->content.file_payload.file.path); return; } if (update.status != PayloadStatus::kSuccess) return; - auto connections_it = connections_.find(endpoint_id); - if (connections_it == connections_.end()) return; + NearbyConnectionImpl* connection = GetConnectionForId(endpoint_id); + if (connection == nullptr) return; NL_LOG(INFO) << "Writing incoming byte message to NearbyConnection."; - connections_it->second->WriteMessage( - payload_it->second.content.bytes_payload.bytes); + connection->WriteMessage(payload->content.bytes_payload.bytes); } void NearbyConnectionsManagerImpl::Reset() { diff --git a/sharing/nearby_connections_manager_impl.h b/sharing/nearby_connections_manager_impl.h index 5fbce33f..31366b53 100644 --- a/sharing/nearby_connections_manager_impl.h +++ b/sharing/nearby_connections_manager_impl.h @@ -78,7 +78,8 @@ class NearbyConnectionsManagerImpl : public NearbyConnectionsManager { void RegisterPayloadStatusListener( int64_t payload_id, std::weak_ptr listener) override; - const Payload* GetIncomingPayload(int64_t payload_id) const override; + const Payload* GetIncomingPayload(int64_t payload_id) const override + ABSL_LOCKS_EXCLUDED(mutex_); void Cancel(int64_t payload_id) override; void ClearIncomingPayloads() override; std::optional> GetRawAuthenticationToken( @@ -128,6 +129,15 @@ class NearbyConnectionsManagerImpl : public NearbyConnectionsManager { const std::filesystem::path& path); absl::flat_hash_set GetUnknownFilePathsToDelete(); + std::optional> GetStatusListenerForId( + int64_t payload_id) const ABSL_LOCKS_EXCLUDED(mutex_); + + NearbyConnectionImpl* GetConnectionForId(absl::string_view endpoint_id) const + ABSL_LOCKS_EXCLUDED(mutex_); + + void RemoveStatusListenerForPayloadId(int64_t payload_id) + ABSL_LOCKS_EXCLUDED(mutex_); + void Reset(); std::optional GetUpgradedMedium(absl::string_view endpoint_id) const;