From 7227c8a7b5e95ae47f2c2c19c22e3004848f8129 Mon Sep 17 00:00:00 2001
From: Steven Moreland <smoreland@google.com>
Date: Wed, 2 Jun 2021 00:24:32 +0000
Subject: 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
---
 libs/binder/Parcel.cpp   |  7 ++++---
 libs/binder/RpcState.cpp | 20 ++++++++++++--------
 libs/binder/RpcState.h   |  3 ++-
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index 9795348a0c..d19b4d83fb 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -283,9 +283,10 @@ status_t Parcel::unflattenBinder(sp<IBinder>* out) const
 
         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 2f6b1b3ac4..e18179e4a8 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -61,6 +61,7 @@ status_t RpcState::onBinderLeaving(const sp<RpcSession>& session, const sp<IBind
     }
 
     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 @@ status_t RpcState::onBinderLeaving(const sp<RpcSession>& session, const sp<IBind
     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 @@ sp<IBinder> RpcState::onBinderEntering(const sp<RpcSession>& session, const RpcA
         // 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 @@ sp<IBinder> RpcState::onBinderEntering(const sp<RpcSession>& session, const RpcA
 
     // 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 @@ status_t RpcState::processTransactInternal(const base::unique_fd& fd, const sp<R
     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 aacb5307e1..d63c1c36ba 100644
--- a/libs/binder/RpcState.h
+++ b/libs/binder/RpcState.h
@@ -81,7 +81,8 @@ public:
      * 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();
-- 
cgit v1.2.3-59-g8ed1b