From f51742734f476e7f9478f7cb56c18b9215ecad43 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Tue, 25 May 2021 00:39:28 +0000 Subject: libbinder: transaction includes refcount to binder Prevents case where one thread is making a transaction and another thread clears the ref to this transaction (mainly this is a problem with oneway transactions). This is something which the binder driver also does implicitly, but it was missing from the RPC binder implementation. Bug: 183140903 Test: binderRpcTest Change-Id: I4f59ad6094f90e5c95af5febea2780bed29d4c88 --- libs/binder/RpcSession.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'libs/binder/RpcSession.cpp') diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index 7c458c123a..d5ce324de1 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -100,12 +100,12 @@ status_t RpcSession::getRemoteMaxThreads(size_t* maxThreads) { return state()->getMaxThreads(connection.fd(), sp::fromExisting(this), maxThreads); } -status_t RpcSession::transact(const RpcAddress& address, uint32_t code, const Parcel& data, +status_t RpcSession::transact(const sp& binder, uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags) { ExclusiveConnection connection(sp::fromExisting(this), (flags & IBinder::FLAG_ONEWAY) ? ConnectionUse::CLIENT_ASYNC : ConnectionUse::CLIENT); - return state()->transact(connection.fd(), address, code, data, + return state()->transact(connection.fd(), binder, code, data, sp::fromExisting(this), reply, flags); } -- cgit v1.2.3-59-g8ed1b From af4ca715bd1820616e04fcc63ad4e64d64df4024 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Mon, 24 May 2021 23:22:08 +0000 Subject: binderRpcTest: use waitpid Actually reap child processes. This gives us stronger guarantees (that everything can shut down) and it avoids 'kill'. Bug: 186661301 Test: binderRpcTest Change-Id: If10f00de845eb8097813b4edbf8e2b8ffdc90c5f --- libs/binder/RpcServer.cpp | 10 +++++---- libs/binder/RpcSession.cpp | 3 ++- libs/binder/RpcState.cpp | 7 +++++-- libs/binder/tests/IBinderRpcTest.aidl | 1 + libs/binder/tests/binderRpcTest.cpp | 38 ++++++++++++++++++++++------------- 5 files changed, 38 insertions(+), 21 deletions(-) (limited to 'libs/binder/RpcSession.cpp') diff --git a/libs/binder/RpcServer.cpp b/libs/binder/RpcServer.cpp index bff5543c9b..93f970946a 100644 --- a/libs/binder/RpcServer.cpp +++ b/libs/binder/RpcServer.cpp @@ -193,10 +193,12 @@ bool RpcServer::shutdown() { mShutdownTrigger->trigger(); while (mJoinThreadRunning || !mConnectingThreads.empty() || !mSessions.empty()) { - ALOGI("Waiting for RpcServer to shut down. Join thread running: %d, Connecting threads: " - "%zu, Sessions: %zu", - mJoinThreadRunning, mConnectingThreads.size(), mSessions.size()); - mShutdownCv.wait(_l); + if (std::cv_status::timeout == mShutdownCv.wait_for(_l, std::chrono::seconds(1))) { + ALOGE("Waiting for RpcServer to shut down (1s w/o progress). Join thread running: %d, " + "Connecting threads: " + "%zu, Sessions: %zu. Is your server deadlocked?", + mJoinThreadRunning, mConnectingThreads.size(), mSessions.size()); + } } // At this point, we know join() is about to exit, but the thread that calls diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index d5ce324de1..ac7544fde8 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -199,7 +199,8 @@ void RpcSession::join(unique_fd client) { state()->getAndExecuteCommand(connection->fd, sp::fromExisting(this)); if (error != OK) { - ALOGI("Binder connection thread closing w/ status %s", statusToString(error).c_str()); + LOG_RPC_DETAIL("Binder connection thread closing w/ status %s", + statusToString(error).c_str()); break; } } diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp index d40fef6f09..1111b822c4 100644 --- a/libs/binder/RpcState.cpp +++ b/libs/binder/RpcState.cpp @@ -239,8 +239,11 @@ bool RpcState::rpcRec(const base::unique_fd& fd, const sp& session, if (status_t status = session->mShutdownTrigger->interruptableReadFully(fd.get(), data, size); status != OK) { - ALOGE("Failed to read %s (%zu bytes) on fd %d, error: %s", what, size, fd.get(), - statusToString(status).c_str()); + if (status != -ECANCELED) { + ALOGE("Failed to read %s (%zu bytes) on fd %d, error: %s", what, size, fd.get(), + statusToString(status).c_str()); + } + return false; } diff --git a/libs/binder/tests/IBinderRpcTest.aidl b/libs/binder/tests/IBinderRpcTest.aidl index 41daccc1cf..646bcc6f84 100644 --- a/libs/binder/tests/IBinderRpcTest.aidl +++ b/libs/binder/tests/IBinderRpcTest.aidl @@ -55,6 +55,7 @@ interface IBinderRpcTest { oneway void sleepMsAsync(int ms); void die(boolean cleanup); + void scheduleShutdown(); void useKernelBinderCallingId(); } diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp index 9b2d88d5bc..4d31673b6e 100644 --- a/libs/binder/tests/binderRpcTest.cpp +++ b/libs/binder/tests/binderRpcTest.cpp @@ -194,6 +194,18 @@ public: _exit(1); } } + + Status scheduleShutdown() override { + sp strongServer = server.promote(); + if (strongServer == nullptr) { + return Status::fromExceptionCode(Status::EX_NULL_POINTER); + } + std::thread([=] { + LOG_ALWAYS_FATAL_IF(!strongServer->shutdown(), "Could not shutdown"); + }).detach(); + return Status::ok(); + } + Status useKernelBinderCallingId() override { // this is WRONG! It does not make sense when using RPC binder, and // because it is SO wrong, and so much code calls this, it should abort! @@ -225,11 +237,13 @@ public: prctl(PR_SET_PDEATHSIG, SIGHUP); f(&mPipe); + + exit(0); } } ~Process() { if (mPid != 0) { - kill(mPid, SIGKILL); + waitpid(mPid, nullptr, 0); } } Pipe* getPipe() { return &mPipe; } @@ -290,11 +304,11 @@ struct BinderRpcTestProcessSession { sp rootIface; // whether session should be invalidated by end of run - bool expectInvalid = false; + bool expectAlreadyShutdown = false; BinderRpcTestProcessSession(BinderRpcTestProcessSession&&) = default; ~BinderRpcTestProcessSession() { - if (!expectInvalid) { + if (!expectAlreadyShutdown) { std::vector remoteCounts; // calling over any sessions counts across all sessions EXPECT_OK(rootIface->countBinders(&remoteCounts)); @@ -302,6 +316,8 @@ struct BinderRpcTestProcessSession { for (auto remoteCount : remoteCounts) { EXPECT_EQ(remoteCount, 1); } + + EXPECT_OK(rootIface->scheduleShutdown()); } rootIface = nullptr; @@ -373,6 +389,9 @@ public: configure(server); server->join(); + + // Another thread calls shutdown. Wait for it to complete. + (void)server->shutdown(); }), }; @@ -424,15 +443,6 @@ public: } }; -TEST_P(BinderRpc, RootObjectIsNull) { - auto proc = createRpcTestSocketServerProcess(1, 1, [](const sp& server) { - // this is the default, but to be explicit - server->setRootObject(nullptr); - }); - - EXPECT_EQ(nullptr, proc.sessions.at(0).root); -} - TEST_P(BinderRpc, Ping) { auto proc = createRpcTestSocketServerProcess(1); ASSERT_NE(proc.rootBinder, nullptr); @@ -900,7 +910,7 @@ TEST_P(BinderRpc, Die) { EXPECT_EQ(DEAD_OBJECT, proc.rootIface->die(doDeathCleanup).transactionError()) << "Do death cleanup: " << doDeathCleanup; - proc.expectInvalid = true; + proc.expectAlreadyShutdown = true; } } @@ -914,7 +924,7 @@ TEST_P(BinderRpc, UseKernelBinderCallingId) { // second time! we catch the error :) EXPECT_EQ(DEAD_OBJECT, proc.rootIface->useKernelBinderCallingId().transactionError()); - proc.expectInvalid = true; + proc.expectAlreadyShutdown = true; } TEST_P(BinderRpc, WorksWithLibbinderNdkPing) { -- cgit v1.2.3-59-g8ed1b From b0dd1184f0856a429b6efad0a47818331957b447 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Tue, 25 May 2021 01:56:46 +0000 Subject: libbinder: RpcSession check no shutdown trigger If a pipe can't be created, this will fail. This error would result in creating a session which can't be shutdown. Bug: 185167543 Test: binderRpcTest Change-Id: Id80da21cdc125783ea744c09fb864a5dc5771464 --- libs/binder/RpcSession.cpp | 10 +++++----- libs/binder/include/binder/RpcSession.h | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) (limited to 'libs/binder/RpcSession.cpp') diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp index ac7544fde8..d05b84834f 100644 --- a/libs/binder/RpcSession.cpp +++ b/libs/binder/RpcSession.cpp @@ -86,8 +86,7 @@ bool RpcSession::addNullDebuggingClient() { return false; } - addClientConnection(std::move(serverFd)); - return true; + return addClientConnection(std::move(serverFd)); } sp RpcSession::getRootObject() { @@ -312,24 +311,25 @@ bool RpcSession::setupOneSocketClient(const RpcSocketAddress& addr, int32_t id) LOG_RPC_DETAIL("Socket at %s client with fd %d", addr.toString().c_str(), serverFd.get()); - addClientConnection(std::move(serverFd)); - return true; + return addClientConnection(std::move(serverFd)); } ALOGE("Ran out of retries to connect to %s", addr.toString().c_str()); return false; } -void RpcSession::addClientConnection(unique_fd fd) { +bool RpcSession::addClientConnection(unique_fd fd) { std::lock_guard _l(mMutex); if (mShutdownTrigger == nullptr) { mShutdownTrigger = FdTrigger::make(); + if (mShutdownTrigger == nullptr) return false; } sp session = sp::make(); session->fd = std::move(fd); mClientConnections.push_back(session); + return true; } void RpcSession::setForServer(const wp& server, int32_t sessionId, diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h index 7c7feaadda..4401aaf3c8 100644 --- a/libs/binder/include/binder/RpcSession.h +++ b/libs/binder/include/binder/RpcSession.h @@ -102,6 +102,7 @@ private: /** This is not a pipe. */ struct FdTrigger { + /** Returns nullptr for error case */ static std::unique_ptr make(); /** @@ -155,7 +156,7 @@ private: bool setupSocketClient(const RpcSocketAddress& address); bool setupOneSocketClient(const RpcSocketAddress& address, int32_t sessionId); - void addClientConnection(base::unique_fd fd); + bool addClientConnection(base::unique_fd fd); void setForServer(const wp& server, int32_t sessionId, const std::shared_ptr& shutdownTrigger); sp assignServerToThisThread(base::unique_fd fd); -- cgit v1.2.3-59-g8ed1b