From 791e466ea0b4283b53cf41ef9dfa37118b345df4 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Mon, 13 Sep 2021 15:22:58 -0700 Subject: libbinder: shutdownAndWait based on # of in conns shutdownAndWait no longer waits when it is called on a zero-thread session (the session only needs to be deleted). However, more importantly, we save a small amount of memory, and the log here is much better (we can see how many incoming threads are stuck). Bug: N/A Test: binderRpcTest Change-Id: I5924a82c33b4c3c2c970e6c6e86f1b8e2af074f5 --- libs/binder/RpcSession.cpp | 12 +++++++----- libs/binder/include/binder/RpcSession.h | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index ffa12559c5..e8f984c717 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -202,7 +202,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::fromExisting(this)); LOG_ALWAYS_FATAL_IF(!mThreads.empty(), "Shutdown failed"); } @@ -256,17 +256,19 @@ status_t RpcSession::readId() { void RpcSession::WaitForShutdownListener::onSessionAllIncomingThreadsEnded( const sp& session) { (void)session; - mShutdown = true; } void RpcSession::WaitForShutdownListener::onSessionIncomingThreadEnded() { mCv.notify_all(); } -void RpcSession::WaitForShutdownListener::waitForShutdown(std::unique_lock& lock) { - while (!mShutdown) { +void RpcSession::WaitForShutdownListener::waitForShutdown(std::unique_lock& lock, + const sp& 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/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h index 24df0e89fe..9c7c97035a 100644 --- a/libs/binder/include/binder/RpcSession.h +++ b/libs/binder/include/binder/RpcSession.h @@ -182,12 +182,12 @@ private: public: void onSessionAllIncomingThreadsEnded(const sp& session) override; void onSessionIncomingThreadEnded() override; - void waitForShutdown(std::unique_lock& lock); + void waitForShutdown(std::unique_lock& lock, const sp& session); private: std::condition_variable mCv; - volatile bool mShutdown = false; }; + friend WaitForShutdownListener; struct RpcConnection : public RefBase { std::unique_ptr rpcTransport; -- cgit v1.2.3-59-g8ed1b From 728587b2851fa5aab102a16d55048a8deb23f18e Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Mon, 13 Sep 2021 11:31:51 -0700 Subject: binder_bpBinderFuzz: use valid BpBinder object Using RPC binder to work on host. Avoid using BpBinder::create which will become private. Bug: 167966510 Test: binder_bpBinderFuzz Change-Id: Icfb454c25ceec8ca9dd47d78d0b81642dc7823f0 --- libs/binder/tests/unit_fuzzers/Android.bp | 1 - libs/binder/tests/unit_fuzzers/BpBinderFuzz.cpp | 47 +++++++++++++++++----- .../tests/unit_fuzzers/BpBinderFuzzFunctions.h | 6 ++- 3 files changed, 42 insertions(+), 12 deletions(-) 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 #include +#include #include #include +#include +#include + +#include +#include +#include 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(0, 1024); - sp bpbinder = BpBinder::create(handle); - if (bpbinder == nullptr) return 0; + std::string addr = std::string(getenv("TMPDIR") ?: "/tmp") + "/binderRpcBenchmark"; + (void)unlink(addr.c_str()); + + sp 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::make()); + server->iUnderstandThisCodeIsExperimentalAndIWillNotUseItInProduction(); + CHECK_EQ(OK, server->setupUnixDomainServer(addr.c_str())); + server->join(); + }); + + sp 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 = 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& s_recipient) -> void { // Clean up possible leftover memory. wp outRecipient(nullptr); - bpbinder->sendObituary(); + if (!bpbinder->isRpcBinder()) bpbinder->sendObituary(); bpbinder->unlinkToDeath(nullptr, reinterpret_cast(&kBpBinderCookie), 0, &outRecipient); @@ -72,7 +72,9 @@ static const std::vector& bpbinder, const sp&) -> void { bpbinder->remoteBinder(); }, [](FuzzedDataProvider*, const sp& bpbinder, - const sp&) -> void { bpbinder->sendObituary(); }, + const sp&) -> void { + if (!bpbinder->isRpcBinder()) bpbinder->sendObituary(); + }, [](FuzzedDataProvider* fdp, const sp& bpbinder, const sp&) -> void { uint32_t uid = fdp->ConsumeIntegral(); -- cgit v1.2.3-59-g8ed1b From 99157624317005011ae57e15a6482c35cea085e5 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Mon, 13 Sep 2021 16:27:34 -0700 Subject: libbinder: hide BpBinder::create(*) functions There is no need to have these around, and having them exposed potentially means they could be stuck in the ABI. They are created (instead of absorbed into the private constructors) because of the additional UID tracking logic there. Bug: 167966510 Test: binderRpcTest Change-Id: I673d7a4c591a1b004f3214e8a17b48e54e91171d --- libs/binder/Parcel.cpp | 4 ++-- libs/binder/ProcessState.cpp | 4 ++-- libs/binder/RpcState.cpp | 7 +++---- libs/binder/include/binder/BpBinder.h | 21 ++++++++++++--------- 4 files changed, 19 insertions(+), 17 deletions(-) diff --git a/libs/binder/Parcel.cpp b/libs/binder/Parcel.cpp index 9147e23363..9f95167f86 100644 --- a/libs/binder/Parcel.cpp +++ b/libs/binder/Parcel.cpp @@ -237,7 +237,7 @@ status_t Parcel::flattenBinder(const sp& 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& 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 15b8604101..4fe4fe651c 100644 --- a/libs/binder/ProcessState.cpp +++ b/libs/binder/ProcessState.cpp @@ -212,7 +212,7 @@ ssize_t ProcessState::getStrongRefCountForNode(const sp& 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 ProcessState::getStrongProxyForHandle(int32_t handle) return nullptr; } - sp b = BpBinder::create(handle); + sp b = BpBinder::PrivateAccessor::create(handle); e->binder = b.get(); if (b) e->refs = b->getWeakRefs(); result = b; 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& session, const spremoteBinder(); 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& session, const spremoteBinder()->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& 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..7f56658328 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 create(int32_t handle); - static sp create(const sp& session, uint64_t address); - /** * Return value: * true - this is associated with a socket RpcSession @@ -116,13 +113,18 @@ public: KeyedVector mObjects; }; - class PrivateAccessorForId { + class PrivateAccessor { private: friend class BpBinder; friend class ::android::Parcel; friend class ::android::ProcessState; friend class ::android::RpcState; - explicit PrivateAccessorForId(const BpBinder* binder) : mBinder(binder) {} + explicit PrivateAccessor(const BpBinder* binder) : mBinder(binder) {} + + static sp create(int32_t handle) { return BpBinder::create(handle); } + static sp create(const sp& session, uint64_t address) { + return BpBinder::create(session, address); + } // valid if !isRpcBinder int32_t binderHandle() const { return mBinder->binderHandle(); } @@ -133,14 +135,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; + static sp create(int32_t handle); + static sp create(const sp& session, uint64_t address); + struct BinderHandle { int32_t handle; }; -- cgit v1.2.3-59-g8ed1b From 4f622fec33ea45e85eabf6f138e972598863c3e4 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Mon, 13 Sep 2021 17:38:09 -0700 Subject: libbinder: RpcSession - hide RPC address format FAQ for this change. - Why does RpcSession still have sendDecStrong(uint64_t)? This is used by RpcState to send dec strongs on local (non-BpBinder) binders. This is needed because if a client has the last strong ref of a binder which is local to the corresponding server, the client needs to transfer ownership of this binder to the server using the call. - Why is RpcSession::sendDecStrong(BpBinder) public? It may be useful for testing, especially in situations where ~BpBinder would be hard to call. - Why does BpBinder have a private accessor, but RpcState is friends with some other Rpc* classes? I would like to have as few friends as possible (in regards to C++ classes!), but it takes quite a bit of boilerplate to avoid this. I want to avoid the Rpc stuff introspecting into the standard binder stuff, but I'm not currently concerned about their interactions between each other. Bug: 167966510 Test: binderRpcTest Change-Id: I89e4de2e3a042a4531e2fd41deeb9bab1c11ff94 --- libs/binder/BpBinder.cpp | 2 +- libs/binder/RpcSession.cpp | 5 +++++ libs/binder/include/binder/BpBinder.h | 1 + libs/binder/include/binder/RpcSession.h | 10 +++++++++- 4 files changed, 16 insertions(+), 2 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/RpcSession.cpp b/libs/binder/RpcSession.cpp index e8f984c717..886a2e952b 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -226,6 +227,10 @@ status_t RpcSession::transact(const sp& binder, uint32_t code, const Pa sp::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::fromExisting(this), diff --git a/libs/binder/include/binder/BpBinder.h b/libs/binder/include/binder/BpBinder.h index 7f56658328..c0454b6fbd 100644 --- a/libs/binder/include/binder/BpBinder.h +++ b/libs/binder/include/binder/BpBinder.h @@ -118,6 +118,7 @@ public: friend class BpBinder; friend class ::android::Parcel; friend class ::android::ProcessState; + friend class ::android::RpcSession; friend class ::android::RpcState; explicit PrivateAccessor(const BpBinder* binder) : mBinder(binder) {} diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h index 9c7c97035a..a841079ec4 100644 --- a/libs/binder/include/binder/RpcSession.h +++ b/libs/binder/include/binder/RpcSession.h @@ -153,7 +153,13 @@ public: [[nodiscard]] status_t transact(const sp& 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(); @@ -172,6 +178,8 @@ private: friend RpcState; explicit RpcSession(std::unique_ptr ctx); + [[nodiscard]] status_t sendDecStrong(uint64_t address); + class EventListener : public virtual RefBase { public: virtual void onSessionAllIncomingThreadsEnded(const sp& session) = 0; -- cgit v1.2.3-59-g8ed1b