Merge changes I89e4de2e,I673d7a4c,Icfb454c2,I5924a82c am: a54c861843 am: d612489ad4

Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/1825953

Change-Id: Iefab3593ba5f4b4b4e7b3ba8de3490af7f369acf

10 files changed, 86 insertions, 38 deletions

diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 687ee257fd..92df874b1f 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -510,7 +510,7 @@ void BpBinder::onLastStrongRef(const void* /*id*/)
 {
     ALOGV("onLastStrongRef BpBinder %p handle %d\n", this, binderHandle());
     if (CC_UNLIKELY(isRpcBinder())) {
-        (void)rpcSession()->sendDecStrong(rpcAddress());
+        (void)rpcSession()->sendDecStrong(this);
         return;
     }
     IF_ALOGV() {
diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp
index dce6f3bf70..75752525c6 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -237,7 +237,7 @@ status_t Parcel::flattenBinder(const sp<IBinder>& binder) {
                     return INVALID_OPERATION;
                 }
             }
-            const int32_t handle = proxy ? proxy->getPrivateAccessorForId().binderHandle() : 0;
+            const int32_t handle = proxy ? proxy->getPrivateAccessor().binderHandle() : 0;
             obj.hdr.type = BINDER_TYPE_HANDLE;
             obj.binder = 0; /* Don't pass uninitialized stack data to a remote process */
             obj.handle = handle;
@@ -572,7 +572,7 @@ void Parcel::markForBinder(const sp<IBinder>& binder) {
     LOG_ALWAYS_FATAL_IF(mData != nullptr, "format must be set before data is written");
 
     if (binder && binder->remoteBinder() && binder->remoteBinder()->isRpcBinder()) {
-        markForRpc(binder->remoteBinder()->getPrivateAccessorForId().rpcSession());
+        markForRpc(binder->remoteBinder()->getPrivateAccessor().rpcSession());
     }
 }
 
diff --git a/libs/binder/ProcessState.cpp b/libs/binder/ProcessState.cpp
index 8ab0e88c13..94b28063ab 100644
--- a/libs/binder/ProcessState.cpp
+++ b/libs/binder/ProcessState.cpp
@@ -212,7 +212,7 @@ ssize_t ProcessState::getStrongRefCountForNode(const sp<BpBinder>& binder) {
     binder_node_info_for_ref info;
     memset(&info, 0, sizeof(binder_node_info_for_ref));
 
-    info.handle = binder->getPrivateAccessorForId().binderHandle();
+    info.handle = binder->getPrivateAccessor().binderHandle();
 
     status_t result = ioctl(mDriverFD, BINDER_GET_NODE_INFO_FOR_REF, &info);
 
@@ -301,7 +301,7 @@ sp<IBinder> ProcessState::getStrongProxyForHandle(int32_t handle)
                    return nullptr;
             }
 
-            sp<BpBinder> b = BpBinder::create(handle);
+            sp<BpBinder> b = BpBinder::PrivateAccessor::create(handle);
             e->binder = b.get();
             if (b) e->refs = b->getWeakRefs();
             result = b;
diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp
index 9395b505e1..38958c93cb 100644
--- a/libs/binder/RpcSession.cpp
+++ b/libs/binder/RpcSession.cpp
@@ -29,6 +29,7 @@
 #include <android-base/hex.h>
 #include <android-base/macros.h>
 #include <android_runtime/vm.h>
+#include <binder/BpBinder.h>
 #include <binder/Parcel.h>
 #include <binder/RpcServer.h>
 #include <binder/RpcTransportRaw.h>
@@ -191,7 +192,7 @@ bool RpcSession::shutdownAndWait(bool wait) {
 
     if (wait) {
         LOG_ALWAYS_FATAL_IF(mShutdownListener == nullptr, "Shutdown listener not installed");
-        mShutdownListener->waitForShutdown(_l);
+        mShutdownListener->waitForShutdown(_l, sp<RpcSession>::fromExisting(this));
 
         LOG_ALWAYS_FATAL_IF(!mThreads.empty(), "Shutdown failed");
     }
@@ -215,6 +216,10 @@ status_t RpcSession::transact(const sp<IBinder>& binder, uint32_t code, const Pa
                              sp<RpcSession>::fromExisting(this), reply, flags);
 }
 
+status_t RpcSession::sendDecStrong(const BpBinder* binder) {
+    return sendDecStrong(binder->getPrivateAccessor().rpcAddress());
+}
+
 status_t RpcSession::sendDecStrong(uint64_t address) {
     ExclusiveConnection connection;
     status_t status = ExclusiveConnection::find(sp<RpcSession>::fromExisting(this),
@@ -245,17 +250,19 @@ status_t RpcSession::readId() {
 void RpcSession::WaitForShutdownListener::onSessionAllIncomingThreadsEnded(
         const sp<RpcSession>& session) {
     (void)session;
-    mShutdown = true;
 }
 
 void RpcSession::WaitForShutdownListener::onSessionIncomingThreadEnded() {
     mCv.notify_all();
 }
 
-void RpcSession::WaitForShutdownListener::waitForShutdown(std::unique_lock<std::mutex>& lock) {
-    while (!mShutdown) {
+void RpcSession::WaitForShutdownListener::waitForShutdown(std::unique_lock<std::mutex>& lock,
+                                                          const sp<RpcSession>& session) {
+    while (session->mIncomingConnections.size() > 0) {
         if (std::cv_status::timeout == mCv.wait_for(lock, std::chrono::seconds(1))) {
-            ALOGE("Waiting for RpcSession to shut down (1s w/o progress).");
+            ALOGE("Waiting for RpcSession to shut down (1s w/o progress): %zu incoming connections "
+                  "still.",
+                  session->mIncomingConnections.size());
         }
     }
 }
diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp
index 59643ba4ee..11a083ac11 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -56,7 +56,7 @@ status_t RpcState::onBinderLeaving(const sp<RpcSession>& session, const sp<IBind
     bool isRemote = binder->remoteBinder();
     bool isRpc = isRemote && binder->remoteBinder()->isRpcBinder();
 
-    if (isRpc && binder->remoteBinder()->getPrivateAccessorForId().rpcSession() != session) {
+    if (isRpc && binder->remoteBinder()->getPrivateAccessor().rpcSession() != session) {
         // We need to be able to send instructions over the socket for how to
         // connect to a different server, and we also need to let the host
         // process know that this is happening.
@@ -85,8 +85,7 @@ status_t RpcState::onBinderLeaving(const sp<RpcSession>& session, const sp<IBind
         if (binder == node.binder) {
             if (isRpc) {
                 // check integrity of data structure
-                uint64_t actualAddr =
-                        binder->remoteBinder()->getPrivateAccessorForId().rpcAddress();
+                uint64_t actualAddr = binder->remoteBinder()->getPrivateAccessor().rpcAddress();
                 LOG_ALWAYS_FATAL_IF(addr != actualAddr, "Address mismatch %" PRIu64 " vs %" PRIu64,
                                     addr, actualAddr);
             }
@@ -185,7 +184,7 @@ status_t RpcState::onBinderEntering(const sp<RpcSession>& session, uint64_t addr
 
     // Currently, all binders are assumed to be part of the same session (no
     // device global binders in the RPC world).
-    it->second.binder = *out = BpBinder::create(session, it->first);
+    it->second.binder = *out = BpBinder::PrivateAccessor::create(session, it->first);
     it->second.timesRecd = 1;
     return OK;
 }
diff --git a/libs/binder/include/binder/BpBinder.h b/libs/binder/include/binder/BpBinder.h
index 9f2ce1efba..c0454b6fbd 100644
--- a/libs/binder/include/binder/BpBinder.h
+++ b/libs/binder/include/binder/BpBinder.h
@@ -39,9 +39,6 @@ using binder_proxy_limit_callback = void(*)(int);
 class BpBinder : public IBinder
 {
 public:
-    static sp<BpBinder> create(int32_t handle);
-    static sp<BpBinder> create(const sp<RpcSession>& session, uint64_t address);
-
     /**
      * Return value:
      * true - this is associated with a socket RpcSession
@@ -116,13 +113,19 @@ public:
         KeyedVector<const void*, entry_t> mObjects;
     };
 
-    class PrivateAccessorForId {
+    class PrivateAccessor {
     private:
         friend class BpBinder;
         friend class ::android::Parcel;
         friend class ::android::ProcessState;
+        friend class ::android::RpcSession;
         friend class ::android::RpcState;
-        explicit PrivateAccessorForId(const BpBinder* binder) : mBinder(binder) {}
+        explicit PrivateAccessor(const BpBinder* binder) : mBinder(binder) {}
+
+        static sp<BpBinder> create(int32_t handle) { return BpBinder::create(handle); }
+        static sp<BpBinder> create(const sp<RpcSession>& session, uint64_t address) {
+            return BpBinder::create(session, address);
+        }
 
         // valid if !isRpcBinder
         int32_t binderHandle() const { return mBinder->binderHandle(); }
@@ -133,14 +136,15 @@ public:
 
         const BpBinder* mBinder;
     };
-    const PrivateAccessorForId getPrivateAccessorForId() const {
-        return PrivateAccessorForId(this);
-    }
+    const PrivateAccessor getPrivateAccessor() const { return PrivateAccessor(this); }
 
 private:
-    friend PrivateAccessorForId;
+    friend PrivateAccessor;
     friend class sp<BpBinder>;
 
+    static sp<BpBinder> create(int32_t handle);
+    static sp<BpBinder> create(const sp<RpcSession>& session, uint64_t address);
+
     struct BinderHandle {
         int32_t handle;
     };
diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h
index 71eb223a07..6a29c05e36 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -151,7 +151,13 @@ public:
 
     [[nodiscard]] status_t transact(const sp<IBinder>& binder, uint32_t code, const Parcel& data,
                                     Parcel* reply, uint32_t flags);
-    [[nodiscard]] status_t sendDecStrong(uint64_t address);
+
+    /**
+     * Generally, you should not call this, unless you are testing error
+     * conditions, as this is called automatically by BpBinders when they are
+     * deleted (this is also why a raw pointer is used here)
+     */
+    [[nodiscard]] status_t sendDecStrong(const BpBinder* binder);
 
     ~RpcSession();
 
@@ -170,6 +176,8 @@ private:
     friend RpcState;
     explicit RpcSession(std::unique_ptr<RpcTransportCtx> ctx);
 
+    [[nodiscard]] status_t sendDecStrong(uint64_t address);
+
     class EventListener : public virtual RefBase {
     public:
         virtual void onSessionAllIncomingThreadsEnded(const sp<RpcSession>& session) = 0;
@@ -180,12 +188,12 @@ private:
     public:
         void onSessionAllIncomingThreadsEnded(const sp<RpcSession>& session) override;
         void onSessionIncomingThreadEnded() override;
-        void waitForShutdown(std::unique_lock<std::mutex>& lock);
+        void waitForShutdown(std::unique_lock<std::mutex>& lock, const sp<RpcSession>& session);
 
     private:
         std::condition_variable mCv;
-        volatile bool mShutdown = false;
     };
+    friend WaitForShutdownListener;
 
     struct RpcConnection : public RefBase {
         std::unique_ptr<RpcTransport> rpcTransport;
diff --git a/libs/binder/tests/unit_fuzzers/Android.bp b/libs/binder/tests/unit_fuzzers/Android.bp
index b1263e8d8e..6f054d2284 100644
--- a/libs/binder/tests/unit_fuzzers/Android.bp
+++ b/libs/binder/tests/unit_fuzzers/Android.bp
@@ -51,7 +51,6 @@ cc_fuzz {
 cc_fuzz {
     name: "binder_bpBinderFuzz",
     defaults: ["binder_fuzz_defaults"],
-    host_supported: false,
     srcs: ["BpBinderFuzz.cpp"],
 }
 
diff --git a/libs/binder/tests/unit_fuzzers/BpBinderFuzz.cpp b/libs/binder/tests/unit_fuzzers/BpBinderFuzz.cpp
index c50279b13d..95582bf0d4 100644
--- a/libs/binder/tests/unit_fuzzers/BpBinderFuzz.cpp
+++ b/libs/binder/tests/unit_fuzzers/BpBinderFuzz.cpp
@@ -19,8 +19,15 @@
 #include <commonFuzzHelpers.h>
 #include <fuzzer/FuzzedDataProvider.h>
 
+#include <android-base/logging.h>
 #include <binder/BpBinder.h>
 #include <binder/IServiceManager.h>
+#include <binder/RpcServer.h>
+#include <binder/RpcSession.h>
+
+#include <signal.h>
+#include <sys/prctl.h>
+#include <thread>
 
 namespace android {
 
@@ -28,13 +35,31 @@ namespace android {
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     FuzzedDataProvider fdp(data, size);
 
-    // TODO: In the future it would be more effective to fork a new process and then pass a BBinder
-    // to your process. Right now this is not implemented because it would involved fuzzing IPC on a
-    // forked process, and libfuzzer will not be able to handle code coverage. This would lead to
-    // crashes that are not easy to diagnose.
-    int32_t handle = fdp.ConsumeIntegralInRange<int32_t>(0, 1024);
-    sp<BpBinder> bpbinder = BpBinder::create(handle);
-    if (bpbinder == nullptr) return 0;
+    std::string addr = std::string(getenv("TMPDIR") ?: "/tmp") + "/binderRpcBenchmark";
+    (void)unlink(addr.c_str());
+
+    sp<RpcServer> server = RpcServer::make();
+
+    // use RPC binder because fuzzer can't get coverage from another process.
+    auto thread = std::thread([&]() {
+        prctl(PR_SET_PDEATHSIG, SIGHUP); // racey, okay
+        server->setRootObject(sp<BBinder>::make());
+        server->iUnderstandThisCodeIsExperimentalAndIWillNotUseItInProduction();
+        CHECK_EQ(OK, server->setupUnixDomainServer(addr.c_str()));
+        server->join();
+    });
+
+    sp<RpcSession> session = RpcSession::make();
+    status_t status;
+    for (size_t tries = 0; tries < 5; tries++) {
+        usleep(10000);
+        status = session->setupUnixDomainClient(addr.c_str());
+        if (status == OK) goto success;
+    }
+    LOG(FATAL) << "Unable to connect";
+success:
+
+    sp<BpBinder> bpBinder = session->getRootObject()->remoteBinder();
 
     // To prevent memory from running out from calling too many add item operations.
     const uint32_t MAX_RUNS = 2048;
@@ -43,12 +68,16 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
 
     while (fdp.remaining_bytes() > 0 && count++ < MAX_RUNS) {
         if (fdp.ConsumeBool()) {
-            callArbitraryFunction(&fdp, gBPBinderOperations, bpbinder, s_recipient);
+            callArbitraryFunction(&fdp, gBPBinderOperations, bpBinder, s_recipient);
         } else {
-            callArbitraryFunction(&fdp, gIBinderOperations, bpbinder.get());
+            callArbitraryFunction(&fdp, gIBinderOperations, bpBinder.get());
         }
     }
 
+    CHECK(session->shutdownAndWait(true)) << "couldn't shutdown session";
+    CHECK(server->shutdown()) << "couldn't shutdown server";
+    thread.join();
+
     return 0;
 }
 } // namespace android
diff --git a/libs/binder/tests/unit_fuzzers/BpBinderFuzzFunctions.h b/libs/binder/tests/unit_fuzzers/BpBinderFuzzFunctions.h
index 6ca0e2f075..741987f754 100644
--- a/libs/binder/tests/unit_fuzzers/BpBinderFuzzFunctions.h
+++ b/libs/binder/tests/unit_fuzzers/BpBinderFuzzFunctions.h
@@ -52,7 +52,7 @@ static const std::vector<std::function<void(FuzzedDataProvider*, const sp<BpBind
                     const sp<IBinder::DeathRecipient>& s_recipient) -> void {
                      // Clean up possible leftover memory.
                      wp<IBinder::DeathRecipient> outRecipient(nullptr);
-                     bpbinder->sendObituary();
+                     if (!bpbinder->isRpcBinder()) bpbinder->sendObituary();
                      bpbinder->unlinkToDeath(nullptr, reinterpret_cast<void*>(&kBpBinderCookie), 0,
                                              &outRecipient);
 
@@ -72,7 +72,9 @@ static const std::vector<std::function<void(FuzzedDataProvider*, const sp<BpBind
                  [](FuzzedDataProvider*, const sp<BpBinder>& bpbinder,
                     const sp<IBinder::DeathRecipient>&) -> void { bpbinder->remoteBinder(); },
                  [](FuzzedDataProvider*, const sp<BpBinder>& bpbinder,
-                    const sp<IBinder::DeathRecipient>&) -> void { bpbinder->sendObituary(); },
+                    const sp<IBinder::DeathRecipient>&) -> void {
+                     if (!bpbinder->isRpcBinder()) bpbinder->sendObituary();
+                 },
                  [](FuzzedDataProvider* fdp, const sp<BpBinder>& bpbinder,
                     const sp<IBinder::DeathRecipient>&) -> void {
                      uint32_t uid = fdp->ConsumeIntegral<uint32_t>();