Skip to content

Commit edf6e90

Browse files
devinmoore-googAndroid (Google) Code Review
authored andcommitted
Merge " Remove TransferDeathRecipients when ABpBinder is deleted" into main
2 parents a559a12 + 9b927e2 commit edf6e90

5 files changed

Lines changed: 172 additions & 4 deletions

File tree

libs/binder/ndk/ibinder.cpp

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -258,11 +258,24 @@ status_t ABBinder::onTransact(transaction_code_t code, const Parcel& data, Parce
258258
}
259259
}
260260

261+
void ABBinder::addDeathRecipient(const ::android::sp<AIBinder_DeathRecipient>& /* recipient */,
262+
void* /* cookie */) {
263+
LOG_ALWAYS_FATAL("Should not reach this. Can't linkToDeath local binders.");
264+
}
265+
261266
ABpBinder::ABpBinder(const ::android::sp<::android::IBinder>& binder)
262267
: AIBinder(nullptr /*clazz*/), mRemote(binder) {
263268
LOG_ALWAYS_FATAL_IF(binder == nullptr, "binder == nullptr");
264269
}
265-
ABpBinder::~ABpBinder() {}
270+
271+
ABpBinder::~ABpBinder() {
272+
for (auto& recip : mDeathRecipients) {
273+
sp<AIBinder_DeathRecipient> strongRecip = recip.recipient.promote();
274+
if (strongRecip) {
275+
strongRecip->pruneThisTransferEntry(getBinder(), recip.cookie);
276+
}
277+
}
278+
}
266279

267280
sp<AIBinder> ABpBinder::lookupOrCreateFromBinder(const ::android::sp<::android::IBinder>& binder) {
268281
if (binder == nullptr) {
@@ -301,6 +314,11 @@ sp<AIBinder> ABpBinder::lookupOrCreateFromBinder(const ::android::sp<::android::
301314
return ret;
302315
}
303316

317+
void ABpBinder::addDeathRecipient(const ::android::sp<AIBinder_DeathRecipient>& recipient,
318+
void* cookie) {
319+
mDeathRecipients.emplace_back(recipient, cookie);
320+
}
321+
304322
struct AIBinder_Weak {
305323
wp<AIBinder> binder;
306324
};
@@ -426,6 +444,17 @@ AIBinder_DeathRecipient::AIBinder_DeathRecipient(AIBinder_DeathRecipient_onBinde
426444
LOG_ALWAYS_FATAL_IF(onDied == nullptr, "onDied == nullptr");
427445
}
428446

447+
void AIBinder_DeathRecipient::pruneThisTransferEntry(const sp<IBinder>& who, void* cookie) {
448+
std::lock_guard<std::mutex> l(mDeathRecipientsMutex);
449+
mDeathRecipients.erase(std::remove_if(mDeathRecipients.begin(), mDeathRecipients.end(),
450+
[&](const sp<TransferDeathRecipient>& tdr) {
451+
auto tdrWho = tdr->getWho();
452+
return tdrWho != nullptr && tdrWho.promote() == who &&
453+
cookie == tdr->getCookie();
454+
}),
455+
mDeathRecipients.end());
456+
}
457+
429458
void AIBinder_DeathRecipient::pruneDeadTransferEntriesLocked() {
430459
mDeathRecipients.erase(std::remove_if(mDeathRecipients.begin(), mDeathRecipients.end(),
431460
[](const sp<TransferDeathRecipient>& tdr) {
@@ -554,8 +583,11 @@ binder_status_t AIBinder_linkToDeath(AIBinder* binder, AIBinder_DeathRecipient*
554583
return STATUS_UNEXPECTED_NULL;
555584
}
556585

557-
// returns binder_status_t
558-
return recipient->linkToDeath(binder->getBinder(), cookie);
586+
binder_status_t ret = recipient->linkToDeath(binder->getBinder(), cookie);
587+
if (ret == STATUS_OK) {
588+
binder->addDeathRecipient(recipient, cookie);
589+
}
590+
return ret;
559591
}
560592

561593
binder_status_t AIBinder_unlinkToDeath(AIBinder* binder, AIBinder_DeathRecipient* recipient,

libs/binder/ndk/ibinder_internal.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ struct AIBinder : public virtual ::android::RefBase {
5151
::android::sp<::android::IBinder> binder = const_cast<AIBinder*>(this)->getBinder();
5252
return binder->remoteBinder() != nullptr;
5353
}
54+
virtual void addDeathRecipient(const ::android::sp<AIBinder_DeathRecipient>& recipient,
55+
void* cookie) = 0;
5456

5557
private:
5658
// AIBinder instance is instance of this class for a local object. In order to transact on a
@@ -78,6 +80,8 @@ struct ABBinder : public AIBinder, public ::android::BBinder {
7880
::android::status_t dump(int fd, const ::android::Vector<::android::String16>& args) override;
7981
::android::status_t onTransact(uint32_t code, const ::android::Parcel& data,
8082
::android::Parcel* reply, binder_flags_t flags) override;
83+
void addDeathRecipient(const ::android::sp<AIBinder_DeathRecipient>& /* recipient */,
84+
void* /* cookie */) override;
8185

8286
private:
8387
ABBinder(const AIBinder_Class* clazz, void* userData);
@@ -106,12 +110,19 @@ struct ABpBinder : public AIBinder {
106110

107111
bool isServiceFuzzing() const { return mServiceFuzzing; }
108112
void setServiceFuzzing() { mServiceFuzzing = true; }
113+
void addDeathRecipient(const ::android::sp<AIBinder_DeathRecipient>& recipient,
114+
void* cookie) override;
109115

110116
private:
111117
friend android::sp<ABpBinder>;
112118
explicit ABpBinder(const ::android::sp<::android::IBinder>& binder);
113119
::android::sp<::android::IBinder> mRemote;
114120
bool mServiceFuzzing = false;
121+
struct DeathRecipientInfo {
122+
android::wp<AIBinder_DeathRecipient> recipient;
123+
void* cookie;
124+
};
125+
std::vector<DeathRecipientInfo> mDeathRecipients;
115126
};
116127

117128
struct AIBinder_Class {
@@ -183,6 +194,7 @@ struct AIBinder_DeathRecipient : ::android::RefBase {
183194
binder_status_t linkToDeath(const ::android::sp<::android::IBinder>&, void* cookie);
184195
binder_status_t unlinkToDeath(const ::android::sp<::android::IBinder>& binder, void* cookie);
185196
void setOnUnlinked(AIBinder_DeathRecipient_onBinderUnlinked onUnlinked);
197+
void pruneThisTransferEntry(const ::android::sp<::android::IBinder>&, void* cookie);
186198

187199
private:
188200
// When the user of this API deletes a Bp object but not the death recipient, the

libs/binder/ndk/tests/iface.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ using ::android::wp;
2525

2626
const char* IFoo::kSomeInstanceName = "libbinder_ndk-test-IFoo";
2727
const char* IFoo::kInstanceNameToDieFor = "libbinder_ndk-test-IFoo-to-die";
28+
const char* IFoo::kInstanceNameToDieFor2 = "libbinder_ndk-test-IFoo-to-die2";
2829
const char* IFoo::kIFooDescriptor = "my-special-IFoo-class";
2930

3031
struct IFoo_Class_Data {

libs/binder/ndk/tests/include/iface/iface.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ class IFoo : public virtual ::android::RefBase {
2727
public:
2828
static const char* kSomeInstanceName;
2929
static const char* kInstanceNameToDieFor;
30+
static const char* kInstanceNameToDieFor2;
3031
static const char* kIFooDescriptor;
3132

3233
static AIBinder_Class* kClass;

libs/binder/ndk/tests/libbinder_ndk_unit_test.cpp

Lines changed: 123 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -536,6 +536,7 @@ TEST(NdkBinder, DeathRecipient) {
536536
bool deathReceived = false;
537537

538538
std::function<void(void)> onDeath = [&] {
539+
std::unique_lock<std::mutex> lockDeath(deathMutex);
539540
std::cerr << "Binder died (as requested)." << std::endl;
540541
deathReceived = true;
541542
deathCv.notify_one();
@@ -547,6 +548,7 @@ TEST(NdkBinder, DeathRecipient) {
547548
bool wasDeathReceivedFirst = false;
548549

549550
std::function<void(void)> onUnlink = [&] {
551+
std::unique_lock<std::mutex> lockUnlink(unlinkMutex);
550552
std::cerr << "Binder unlinked (as requested)." << std::endl;
551553
wasDeathReceivedFirst = deathReceived;
552554
unlinkReceived = true;
@@ -560,7 +562,6 @@ TEST(NdkBinder, DeathRecipient) {
560562

561563
EXPECT_EQ(STATUS_OK, AIBinder_linkToDeath(binder, recipient, static_cast<void*>(cookie)));
562564

563-
// the binder driver should return this if the service dies during the transaction
564565
EXPECT_EQ(STATUS_DEAD_OBJECT, foo->die());
565566

566567
foo = nullptr;
@@ -579,6 +580,123 @@ TEST(NdkBinder, DeathRecipient) {
579580
binder = nullptr;
580581
}
581582

583+
TEST(NdkBinder, DeathRecipientDropBinderNoDeath) {
584+
using namespace std::chrono_literals;
585+
586+
std::mutex deathMutex;
587+
std::condition_variable deathCv;
588+
bool deathReceived = false;
589+
590+
std::function<void(void)> onDeath = [&] {
591+
std::unique_lock<std::mutex> lockDeath(deathMutex);
592+
std::cerr << "Binder died (as requested)." << std::endl;
593+
deathReceived = true;
594+
deathCv.notify_one();
595+
};
596+
597+
std::mutex unlinkMutex;
598+
std::condition_variable unlinkCv;
599+
bool unlinkReceived = false;
600+
bool wasDeathReceivedFirst = false;
601+
602+
std::function<void(void)> onUnlink = [&] {
603+
std::unique_lock<std::mutex> lockUnlink(unlinkMutex);
604+
std::cerr << "Binder unlinked (as requested)." << std::endl;
605+
wasDeathReceivedFirst = deathReceived;
606+
unlinkReceived = true;
607+
unlinkCv.notify_one();
608+
};
609+
610+
// keep the death recipient around
611+
ndk::ScopedAIBinder_DeathRecipient recipient(AIBinder_DeathRecipient_new(LambdaOnDeath));
612+
AIBinder_DeathRecipient_setOnUnlinked(recipient.get(), LambdaOnUnlink);
613+
614+
{
615+
AIBinder* binder;
616+
sp<IFoo> foo = IFoo::getService(IFoo::kInstanceNameToDieFor2, &binder);
617+
ASSERT_NE(nullptr, foo.get());
618+
ASSERT_NE(nullptr, binder);
619+
620+
DeathRecipientCookie* cookie = new DeathRecipientCookie{&onDeath, &onUnlink};
621+
622+
EXPECT_EQ(STATUS_OK,
623+
AIBinder_linkToDeath(binder, recipient.get(), static_cast<void*>(cookie)));
624+
// let the sp<IFoo> and AIBinder fall out of scope
625+
AIBinder_decStrong(binder);
626+
binder = nullptr;
627+
}
628+
629+
{
630+
std::unique_lock<std::mutex> lockDeath(deathMutex);
631+
EXPECT_FALSE(deathCv.wait_for(lockDeath, 100ms, [&] { return deathReceived; }));
632+
EXPECT_FALSE(deathReceived);
633+
}
634+
635+
{
636+
std::unique_lock<std::mutex> lockUnlink(unlinkMutex);
637+
EXPECT_TRUE(deathCv.wait_for(lockUnlink, 1s, [&] { return unlinkReceived; }));
638+
EXPECT_TRUE(unlinkReceived);
639+
EXPECT_FALSE(wasDeathReceivedFirst);
640+
}
641+
}
642+
643+
TEST(NdkBinder, DeathRecipientDropBinderOnDied) {
644+
using namespace std::chrono_literals;
645+
646+
std::mutex deathMutex;
647+
std::condition_variable deathCv;
648+
bool deathReceived = false;
649+
650+
sp<IFoo> foo;
651+
AIBinder* binder;
652+
std::function<void(void)> onDeath = [&] {
653+
std::unique_lock<std::mutex> lockDeath(deathMutex);
654+
std::cerr << "Binder died (as requested)." << std::endl;
655+
deathReceived = true;
656+
AIBinder_decStrong(binder);
657+
binder = nullptr;
658+
deathCv.notify_one();
659+
};
660+
661+
std::mutex unlinkMutex;
662+
std::condition_variable unlinkCv;
663+
bool unlinkReceived = false;
664+
bool wasDeathReceivedFirst = false;
665+
666+
std::function<void(void)> onUnlink = [&] {
667+
std::unique_lock<std::mutex> lockUnlink(unlinkMutex);
668+
std::cerr << "Binder unlinked (as requested)." << std::endl;
669+
wasDeathReceivedFirst = deathReceived;
670+
unlinkReceived = true;
671+
unlinkCv.notify_one();
672+
};
673+
674+
ndk::ScopedAIBinder_DeathRecipient recipient(AIBinder_DeathRecipient_new(LambdaOnDeath));
675+
AIBinder_DeathRecipient_setOnUnlinked(recipient.get(), LambdaOnUnlink);
676+
677+
foo = IFoo::getService(IFoo::kInstanceNameToDieFor2, &binder);
678+
ASSERT_NE(nullptr, foo.get());
679+
ASSERT_NE(nullptr, binder);
680+
681+
DeathRecipientCookie* cookie = new DeathRecipientCookie{&onDeath, &onUnlink};
682+
EXPECT_EQ(STATUS_OK, AIBinder_linkToDeath(binder, recipient.get(), static_cast<void*>(cookie)));
683+
684+
EXPECT_EQ(STATUS_DEAD_OBJECT, foo->die());
685+
686+
{
687+
std::unique_lock<std::mutex> lockDeath(deathMutex);
688+
EXPECT_TRUE(deathCv.wait_for(lockDeath, 1s, [&] { return deathReceived; }));
689+
EXPECT_TRUE(deathReceived);
690+
}
691+
692+
{
693+
std::unique_lock<std::mutex> lockUnlink(unlinkMutex);
694+
EXPECT_TRUE(deathCv.wait_for(lockUnlink, 100ms, [&] { return unlinkReceived; }));
695+
EXPECT_TRUE(unlinkReceived);
696+
EXPECT_TRUE(wasDeathReceivedFirst);
697+
}
698+
}
699+
582700
TEST(NdkBinder, RetrieveNonNdkService) {
583701
#pragma clang diagnostic push
584702
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
@@ -956,6 +1074,10 @@ int main(int argc, char* argv[]) {
9561074
prctl(PR_SET_PDEATHSIG, SIGHUP);
9571075
return manualThreadPoolService(IFoo::kInstanceNameToDieFor);
9581076
}
1077+
if (fork() == 0) {
1078+
prctl(PR_SET_PDEATHSIG, SIGHUP);
1079+
return manualThreadPoolService(IFoo::kInstanceNameToDieFor2);
1080+
}
9591081
if (fork() == 0) {
9601082
prctl(PR_SET_PDEATHSIG, SIGHUP);
9611083
return manualPollingService(IFoo::kSomeInstanceName);

0 commit comments

Comments
 (0)