libbinder: onBinder* respect termination
Previously, these would leak binder objects if transactions occurred on
already terminated RpcState objects.
Fixes: 189345133
Test: binder_parcel_fuzzer (w/ leak repro), binderRpcTest
Change-Id: I68f86bf0656dd316691634d4fc411e6cac361449
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 9795348..d19b4d8 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -283,9 +283,10 @@
if (isNull & 1) {
auto addr = RpcAddress::zero();
- status_t status = addr.readFromParcel(*this);
- if (status != OK) return status;
- binder = mSession->state()->onBinderEntering(mSession, addr);
+ if (status_t status = addr.readFromParcel(*this); status != OK) return status;
+ if (status_t status = mSession->state()->onBinderEntering(mSession, addr, &binder);
+ status != OK)
+ return status;
}
return finishUnflattenBinder(binder, out);
diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp
index 2f6b1b3..e18179e 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -61,6 +61,7 @@
}
std::lock_guard<std::mutex> _l(mNodeMutex);
+ if (mTerminated) return DEAD_OBJECT;
// TODO(b/182939933): maybe move address out of BpBinder, and keep binder->address map
// in RpcState
@@ -95,11 +96,13 @@
return OK;
}
-sp<IBinder> RpcState::onBinderEntering(const sp<RpcSession>& session, const RpcAddress& address) {
+status_t RpcState::onBinderEntering(const sp<RpcSession>& session, const RpcAddress& address,
+ sp<IBinder>* out) {
std::unique_lock<std::mutex> _l(mNodeMutex);
+ if (mTerminated) return DEAD_OBJECT;
if (auto it = mNodeForAddress.find(address); it != mNodeForAddress.end()) {
- sp<IBinder> binder = it->second.binder.promote();
+ *out = it->second.binder.promote();
// implicitly have strong RPC refcount, since we received this binder
it->second.timesRecd++;
@@ -111,7 +114,7 @@
// immediately, we wait to send the last one in BpBinder::onLastDecStrong.
(void)session->sendDecStrong(address);
- return binder;
+ return OK;
}
auto&& [it, inserted] = mNodeForAddress.insert({address, BinderNode{}});
@@ -119,10 +122,9 @@
// Currently, all binders are assumed to be part of the same session (no
// device global binders in the RPC world).
- sp<IBinder> binder = BpBinder::create(session, it->first);
- it->second.binder = binder;
+ it->second.binder = *out = BpBinder::create(session, it->first);
it->second.timesRecd = 1;
- return binder;
+ return OK;
}
size_t RpcState::countBinders() {
@@ -556,12 +558,14 @@
sp<IBinder> target;
if (!addr.isZero()) {
if (!targetRef) {
- target = onBinderEntering(session, addr);
+ replyStatus = onBinderEntering(session, addr, &target);
} else {
target = targetRef;
}
- if (target == nullptr) {
+ if (replyStatus != OK) {
+ // do nothing
+ } else if (target == nullptr) {
// This can happen if the binder is remote in this process, and
// another thread has called the last decStrong on this binder.
// However, for local binders, it indicates a misbehaving client
diff --git a/libs/binder/RpcState.h b/libs/binder/RpcState.h
index aacb530..d63c1c3 100644
--- a/libs/binder/RpcState.h
+++ b/libs/binder/RpcState.h
@@ -81,7 +81,8 @@
* to the process, if this process already has one, or it takes ownership of
* that refcount
*/
- sp<IBinder> onBinderEntering(const sp<RpcSession>& session, const RpcAddress& address);
+ [[nodiscard]] status_t onBinderEntering(const sp<RpcSession>& session,
+ const RpcAddress& address, sp<IBinder>* out);
size_t countBinders();
void dump();