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
diff --git a/libs/binder/BpBinder.cpp b/libs/binder/BpBinder.cpp
index 687ee25..92df874 100644
--- a/libs/binder/BpBinder.cpp
+++ b/libs/binder/BpBinder.cpp
@@ -510,7 +510,7 @@
{
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 dce6f3b..7575252 100644
--- a/libs/binder/Parcel.cpp
+++ b/libs/binder/Parcel.cpp
@@ -237,7 +237,7 @@
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 @@
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 8ab0e88..94b2806 100644
--- a/libs/binder/ProcessState.cpp
+++ b/libs/binder/ProcessState.cpp
@@ -212,7 +212,7 @@
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 @@
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 9395b50..38958c9 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 @@
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 @@
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 @@
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 59643ba..11a083a 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -56,7 +56,7 @@
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 @@
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 @@
// 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 9f2ce1e..c0454b6 100644
--- a/libs/binder/include/binder/BpBinder.h
+++ b/libs/binder/include/binder/BpBinder.h
@@ -39,9 +39,6 @@
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 @@
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 @@
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 71eb223..6a29c05 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -151,7 +151,13 @@
[[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 @@
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 @@
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 b1263e8..6f054d2 100644
--- a/libs/binder/tests/unit_fuzzers/Android.bp
+++ b/libs/binder/tests/unit_fuzzers/Android.bp
@@ -51,7 +51,6 @@
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 c50279b..95582bf 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 @@
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 @@
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 6ca0e2f..741987f 100644
--- a/libs/binder/tests/unit_fuzzers/BpBinderFuzzFunctions.h
+++ b/libs/binder/tests/unit_fuzzers/BpBinderFuzzFunctions.h
@@ -52,7 +52,7 @@
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 @@
[](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>();