From 92f705cee51938f485b3a8ca1e09910761f7eac0 Mon Sep 17 00:00:00 2001 From: Devin Moore Date: Fri, 19 Jan 2024 22:22:01 +0000 Subject: [PATCH] Use onUnlinked in health HAL It's possible to get an onBinderDied callback after a call to AIBinder_unlinkToDeath() so we can't delete the objects in callbacks_ until we are done using the void* cookie. Handling the cleanup in onBinderUnlinked will handle the case where we manually unlink it as well as the case where it's unlinked due to death. Test: atest VtsHalHealthTargetTest Bug: 319210610 Change-Id: Iee4783217cc88134af6de0fe66128684ca984dba --- health/aidl/default/Health.cpp | 34 ++++++++++++++----- health/aidl/default/LinkedCallback.cpp | 26 +++++--------- health/aidl/default/LinkedCallback.h | 17 +++------- .../aidl/default/include/health-impl/Health.h | 3 +- 4 files changed, 41 insertions(+), 39 deletions(-) diff --git a/health/aidl/default/Health.cpp b/health/aidl/default/Health.cpp index 8174bc8ea4..37662eacf3 100644 --- a/health/aidl/default/Health.cpp +++ b/health/aidl/default/Health.cpp @@ -36,6 +36,11 @@ void OnCallbackDiedWrapped(void* cookie) { LinkedCallback* linked = reinterpret_cast(cookie); linked->OnCallbackDied(); } +// Delete the owned cookie. +void onCallbackUnlinked(void* cookie) { + LinkedCallback* linked = reinterpret_cast(cookie); + delete linked; +} } // namespace /* @@ -57,6 +62,7 @@ Health::Health(std::string_view instance_name, std::unique_ptr lock(callbacks_lock_); - auto matches = [callback](const auto& linked) { - return linked->callback()->asBinder() == callback->asBinder(); // compares binder object + auto matches = [callback](const auto& cb) { + return cb->asBinder() == callback->asBinder(); // compares binder object }; - auto it = std::remove_if(callbacks_.begin(), callbacks_.end(), matches); - bool removed = (it != callbacks_.end()); - callbacks_.erase(it, callbacks_.end()); // calls unlinkToDeath on deleted callbacks. + bool removed = false; + for (auto it = callbacks_.begin(); it != callbacks_.end();) { + if (it->second->asBinder() == callback->asBinder()) { + auto status = AIBinder_unlinkToDeath(callback->asBinder().get(), death_recipient_.get(), + reinterpret_cast(it->first)); + if (status != STATUS_OK && status != STATUS_DEAD_OBJECT) { + LOG(WARNING) << __func__ + << "Cannot unlink to death: " << ::android::statusToString(status); + } + it = callbacks_.erase(it); + removed = true; + } else { + it++; + } + } return removed ? ndk::ScopedAStatus::ok() : ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } @@ -347,8 +365,8 @@ ndk::ScopedAStatus Health::update() { void Health::OnHealthInfoChanged(const HealthInfo& health_info) { // Notify all callbacks std::unique_lock lock(callbacks_lock_); - for (const auto& linked : callbacks_) { - auto res = linked->callback()->healthInfoChanged(health_info); + for (const auto& [_, callback] : callbacks_) { + auto res = callback->healthInfoChanged(health_info); if (!res.isOk()) { LOG(DEBUG) << "Cannot call healthInfoChanged:" << res.getDescription() << ". Do nothing here if callback is dead as it will be cleaned up later."; diff --git a/health/aidl/default/LinkedCallback.cpp b/health/aidl/default/LinkedCallback.cpp index 26e99f95d3..df471a37bd 100644 --- a/health/aidl/default/LinkedCallback.cpp +++ b/health/aidl/default/LinkedCallback.cpp @@ -24,35 +24,24 @@ namespace aidl::android::hardware::health { -::android::base::Result> LinkedCallback::Make( +::android::base::Result LinkedCallback::Make( std::shared_ptr service, std::shared_ptr callback) { - std::unique_ptr ret(new LinkedCallback()); + LinkedCallback* ret(new LinkedCallback()); + // pass ownership of this object to the death recipient binder_status_t linkRet = AIBinder_linkToDeath(callback->asBinder().get(), service->death_recipient_.get(), - reinterpret_cast(ret.get())); + reinterpret_cast(ret)); if (linkRet != ::STATUS_OK) { LOG(WARNING) << __func__ << "Cannot link to death: " << linkRet; return ::android::base::Error(-linkRet); } ret->service_ = service; - ret->callback_ = std::move(callback); + ret->callback_ = callback; return ret; } LinkedCallback::LinkedCallback() = default; -LinkedCallback::~LinkedCallback() { - if (callback_ == nullptr) { - return; - } - auto status = - AIBinder_unlinkToDeath(callback_->asBinder().get(), service()->death_recipient_.get(), - reinterpret_cast(this)); - if (status != STATUS_OK && status != STATUS_DEAD_OBJECT) { - LOG(WARNING) << __func__ << "Cannot unlink to death: " << ::android::statusToString(status); - } -} - std::shared_ptr LinkedCallback::service() { auto service_sp = service_.lock(); CHECK_NE(nullptr, service_sp); @@ -60,7 +49,10 @@ std::shared_ptr LinkedCallback::service() { } void LinkedCallback::OnCallbackDied() { - service()->unregisterCallback(callback_); + auto sCb = callback_.lock(); + if (sCb) { + service()->unregisterCallback(sCb); + } } } // namespace aidl::android::hardware::health diff --git a/health/aidl/default/LinkedCallback.h b/health/aidl/default/LinkedCallback.h index da494c9191..8c9c997302 100644 --- a/health/aidl/default/LinkedCallback.h +++ b/health/aidl/default/LinkedCallback.h @@ -32,19 +32,10 @@ namespace aidl::android::hardware::health { class LinkedCallback { public: // Automatically linkToDeath upon construction with the returned object as the cookie. - // service->death_reciepient() should be from CreateDeathRecipient(). - // Not using a strong reference to |service| to avoid circular reference. The lifetime - // of |service| must be longer than this LinkedCallback object. - static ::android::base::Result> Make( + // The deathRecipient owns the LinkedCallback object and will delete it with + // cookie when it's unlinked. + static ::android::base::Result Make( std::shared_ptr service, std::shared_ptr callback); - - // Automatically unlinkToDeath upon destruction. So, it is always safe to reinterpret_cast - // the cookie back to the LinkedCallback object. - ~LinkedCallback(); - - // The wrapped IHealthInfoCallback object. - const std::shared_ptr& callback() const { return callback_; } - // On callback died, unreigster it from the service. void OnCallbackDied(); @@ -55,7 +46,7 @@ class LinkedCallback { std::shared_ptr service(); std::weak_ptr service_; - std::shared_ptr callback_; + std::weak_ptr callback_; }; } // namespace aidl::android::hardware::health diff --git a/health/aidl/default/include/health-impl/Health.h b/health/aidl/default/include/health-impl/Health.h index dc3a0ef8fe..429ae2ab96 100644 --- a/health/aidl/default/include/health-impl/Health.h +++ b/health/aidl/default/include/health-impl/Health.h @@ -16,6 +16,7 @@ #pragma once +#include #include #include @@ -112,7 +113,7 @@ class Health : public BnHealth, public HalHealthLoopCallback { ndk::ScopedAIBinder_DeathRecipient death_recipient_; int binder_fd_ = -1; std::mutex callbacks_lock_; - std::vector> callbacks_; + std::map> callbacks_; }; } // namespace aidl::android::hardware::health