libbinder_ndk: unlinkToDeath dead proxy
This CL automatically unlinks to death when a proxy dies (to clean
up additional data) and documents the behavior.
Bug: N/A
Test: ./runtests.sh
Change-Id: Ib092b25273a51f5afb12904bfe491927ad404f8d
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index 14edcbe..0630963 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -143,6 +143,9 @@
* dies. The @a cookie is optional. If non-NULL, you can
* supply a NULL @a recipient, and the recipient previously
* added with that cookie will be unlinked.
+ *
+ * If the binder is dead, this will return DEAD_OBJECT. Deleting
+ * the object will also unlink all death recipients.
*/
// NOLINTNEXTLINE(google-default-arguments)
virtual status_t unlinkToDeath( const wp<DeathRecipient>& recipient,
diff --git a/libs/binder/ndk/ibinder.cpp b/libs/binder/ndk/ibinder.cpp
index bdbc0d3..bd6886d 100644
--- a/libs/binder/ndk/ibinder.cpp
+++ b/libs/binder/ndk/ibinder.cpp
@@ -269,6 +269,18 @@
CHECK(who == mWho);
mOnDied(mCookie);
+
+ sp<AIBinder_DeathRecipient> recipient = mParentRecipient.promote();
+ sp<IBinder> strongWho = who.promote();
+
+ // otherwise this will be cleaned up later with pruneDeadTransferEntriesLocked
+ if (recipient != nullptr && strongWho != nullptr) {
+ status_t result = recipient->unlinkToDeath(strongWho, mCookie);
+ if (result != ::android::DEAD_OBJECT) {
+ LOG(WARNING) << "Unlinking to dead binder resulted in: " << result;
+ }
+ }
+
mWho = nullptr;
}
@@ -277,24 +289,34 @@
CHECK(onDied != nullptr);
}
-binder_status_t AIBinder_DeathRecipient::linkToDeath(AIBinder* binder, void* cookie) {
+void AIBinder_DeathRecipient::pruneDeadTransferEntriesLocked() {
+ mDeathRecipients.erase(std::remove_if(mDeathRecipients.begin(), mDeathRecipients.end(),
+ [](const sp<TransferDeathRecipient>& tdr) {
+ return tdr->getWho() == nullptr;
+ }),
+ mDeathRecipients.end());
+}
+
+binder_status_t AIBinder_DeathRecipient::linkToDeath(sp<IBinder> binder, void* cookie) {
CHECK(binder != nullptr);
std::lock_guard<std::mutex> l(mDeathRecipientsMutex);
sp<TransferDeathRecipient> recipient =
- new TransferDeathRecipient(binder->getBinder(), cookie, mOnDied);
+ new TransferDeathRecipient(binder, cookie, this, mOnDied);
- status_t status = binder->getBinder()->linkToDeath(recipient, cookie, 0 /*flags*/);
+ status_t status = binder->linkToDeath(recipient, cookie, 0 /*flags*/);
if (status != STATUS_OK) {
return PruneStatusT(status);
}
mDeathRecipients.push_back(recipient);
+
+ pruneDeadTransferEntriesLocked();
return STATUS_OK;
}
-binder_status_t AIBinder_DeathRecipient::unlinkToDeath(AIBinder* binder, void* cookie) {
+binder_status_t AIBinder_DeathRecipient::unlinkToDeath(sp<IBinder> binder, void* cookie) {
CHECK(binder != nullptr);
std::lock_guard<std::mutex> l(mDeathRecipientsMutex);
@@ -302,10 +324,10 @@
for (auto it = mDeathRecipients.rbegin(); it != mDeathRecipients.rend(); ++it) {
sp<TransferDeathRecipient> recipient = *it;
- if (recipient->getCookie() == cookie && recipient->getWho() == binder->getBinder()) {
+ if (recipient->getCookie() == cookie && recipient->getWho() == binder) {
mDeathRecipients.erase(it.base() - 1);
- status_t status = binder->getBinder()->unlinkToDeath(recipient, cookie, 0 /*flags*/);
+ status_t status = binder->unlinkToDeath(recipient, cookie, 0 /*flags*/);
if (status != ::android::OK) {
LOG(ERROR) << __func__
<< ": removed reference to death recipient but unlink failed.";
@@ -390,7 +412,7 @@
}
// returns binder_status_t
- return recipient->linkToDeath(binder, cookie);
+ return recipient->linkToDeath(binder->getBinder(), cookie);
}
binder_status_t AIBinder_unlinkToDeath(AIBinder* binder, AIBinder_DeathRecipient* recipient,
@@ -401,7 +423,7 @@
}
// returns binder_status_t
- return recipient->unlinkToDeath(binder, cookie);
+ return recipient->unlinkToDeath(binder->getBinder(), cookie);
}
uid_t AIBinder_getCallingUid() {
@@ -555,9 +577,15 @@
LOG(ERROR) << __func__ << ": requires non-null onBinderDied parameter.";
return nullptr;
}
- return new AIBinder_DeathRecipient(onBinderDied);
+ auto ret = new AIBinder_DeathRecipient(onBinderDied);
+ ret->incStrong(nullptr);
+ return ret;
}
void AIBinder_DeathRecipient_delete(AIBinder_DeathRecipient* recipient) {
- delete recipient;
+ if (recipient == nullptr) {
+ return;
+ }
+
+ recipient->decStrong(nullptr);
}
diff --git a/libs/binder/ndk/ibinder_internal.h b/libs/binder/ndk/ibinder_internal.h
index 202d6d2..5cb68c2 100644
--- a/libs/binder/ndk/ibinder_internal.h
+++ b/libs/binder/ndk/ibinder_internal.h
@@ -128,13 +128,14 @@
//
// When the AIBinder_DeathRecipient is dropped, so are the actual underlying death recipients. When
// the IBinder dies, only a wp to it is kept.
-struct AIBinder_DeathRecipient {
+struct AIBinder_DeathRecipient : ::android::RefBase {
// One of these is created for every linkToDeath. This is to be able to recover data when a
// binderDied receipt only gives us information about the IBinder.
struct TransferDeathRecipient : ::android::IBinder::DeathRecipient {
TransferDeathRecipient(const ::android::wp<::android::IBinder>& who, void* cookie,
+ const ::android::wp<AIBinder_DeathRecipient>& parentRecipient,
const AIBinder_DeathRecipient_onBinderDied onDied)
- : mWho(who), mCookie(cookie), mOnDied(onDied) {}
+ : mWho(who), mCookie(cookie), mParentRecipient(parentRecipient), mOnDied(onDied) {}
void binderDied(const ::android::wp<::android::IBinder>& who) override;
@@ -144,14 +145,24 @@
private:
::android::wp<::android::IBinder> mWho;
void* mCookie;
+
+ ::android::wp<AIBinder_DeathRecipient> mParentRecipient;
+
+ // This is kept separately from AIBinder_DeathRecipient in case the death recipient is
+ // deleted while the death notification is fired
const AIBinder_DeathRecipient_onBinderDied mOnDied;
};
explicit AIBinder_DeathRecipient(AIBinder_DeathRecipient_onBinderDied onDied);
- binder_status_t linkToDeath(AIBinder* binder, void* cookie);
- binder_status_t unlinkToDeath(AIBinder* binder, void* cookie);
+ binder_status_t linkToDeath(::android::sp<::android::IBinder>, void* cookie);
+ binder_status_t unlinkToDeath(::android::sp<::android::IBinder> binder, void* cookie);
private:
+ // When the user of this API deletes a Bp object but not the death recipient, the
+ // TransferDeathRecipient object can't be cleaned up. This is called whenever a new
+ // TransferDeathRecipient is linked, and it ensures that mDeathRecipients can't grow unbounded.
+ void pruneDeadTransferEntriesLocked();
+
std::mutex mDeathRecipientsMutex;
std::vector<::android::sp<TransferDeathRecipient>> mDeathRecipients;
AIBinder_DeathRecipient_onBinderDied mOnDied;
diff --git a/libs/binder/ndk/include_ndk/android/binder_ibinder.h b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
index bddc10d..80d1254 100644
--- a/libs/binder/ndk/include_ndk/android/binder_ibinder.h
+++ b/libs/binder/ndk/include_ndk/android/binder_ibinder.h
@@ -301,6 +301,11 @@
* may return a binder transaction failure and in case the death recipient cannot be found, it
* returns STATUS_NAME_NOT_FOUND.
*
+ * This only ever needs to be called when the AIBinder_DeathRecipient remains for use with other
+ * AIBinder objects. If the death recipient is deleted, all binders will automatically be unlinked.
+ * If the binder dies, it will automatically unlink. If the binder is deleted, it will be
+ * automatically unlinked.
+ *
* \param binder the binder object to remove a previously linked death recipient from.
* \param recipient the callback to remove.
* \param cookie the cookie used to link to death.
diff --git a/libs/binder/ndk/test/main_client.cpp b/libs/binder/ndk/test/main_client.cpp
index bff601e..8467734 100644
--- a/libs/binder/ndk/test/main_client.cpp
+++ b/libs/binder/ndk/test/main_client.cpp
@@ -86,12 +86,15 @@
// the binder driver should return this if the service dies during the transaction
EXPECT_EQ(STATUS_DEAD_OBJECT, foo->die());
+ foo = nullptr;
+ AIBinder_decStrong(binder);
+ binder = nullptr;
+
std::unique_lock<std::mutex> lock(deathMutex);
EXPECT_TRUE(deathCv.wait_for(lock, 1s, [&] { return deathRecieved; }));
EXPECT_TRUE(deathRecieved);
AIBinder_DeathRecipient_delete(recipient);
- AIBinder_decStrong(binder);
}
TEST(NdkBinder, RetrieveNonNdkService) {