libbinder: RPC explicit connect thread ownership
- thread is detached when it is no longer owned (avoids abort)
- RpcServer passes connection thread ownership to RpcSession before it
lets go of its lock (otherwise, it's possible to take the lock for
both the session and the server, and have a relevant thread which
isn't reflected as owned in either of these objects). Currently this
only affects the fuzzer, but it will also be important for shutting
down these threadpools.
Future considerations - this code has a few messy parts, but it will
have to be rewritten to avoid the std::thread constructor (which throws
exceptions) and also to read a header instead of an ID.
Bug: 185167543
Test: binderRpcTest, binder_rpc_fuzzer (which is in-progress)
Change-Id: Ide630e36595d09a88e904af2e9ab6886ae4f2118
diff --git a/libs/binder/RpcServer.cpp b/libs/binder/RpcServer.cpp
index 3a98cad..06c3a42 100644
--- a/libs/binder/RpcServer.cpp
+++ b/libs/binder/RpcServer.cpp
@@ -22,6 +22,7 @@
#include <thread>
#include <vector>
+#include <android-base/scopeguard.h>
#include <binder/Parcel.h>
#include <binder/RpcServer.h>
#include <log/log.h>
@@ -32,6 +33,7 @@
namespace android {
+using base::ScopeGuard;
using base::unique_fd;
RpcServer::RpcServer() {}
@@ -157,10 +159,11 @@
// TODO(b/183988761): cannot trust this simple ID
LOG_ALWAYS_FATAL_IF(!mAgreedExperimental, "no!");
+ bool idValid = true;
int32_t id;
if (sizeof(id) != read(clientFd.get(), &id, sizeof(id))) {
ALOGE("Could not read ID from fd %d", clientFd.get());
- return;
+ idValid = false;
}
std::thread thisThread;
@@ -172,8 +175,13 @@
LOG_ALWAYS_FATAL_IF(threadId == mConnectingThreads.end(),
"Must establish connection on owned thread");
thisThread = std::move(threadId->second);
+ ScopeGuard detachGuard = [&]() { thisThread.detach(); };
mConnectingThreads.erase(threadId);
+ if (!idValid) {
+ return;
+ }
+
if (id == RPC_SESSION_ID_NEW) {
LOG_ALWAYS_FATAL_IF(mSessionIdCounter >= INT32_MAX, "Out of session IDs");
mSessionIdCounter++;
@@ -190,6 +198,9 @@
}
session = it->second;
}
+
+ detachGuard.Disable();
+ session->preJoin(std::move(thisThread));
}
// avoid strong cycle
@@ -199,7 +210,7 @@
// DO NOT ACCESS MEMBER VARIABLES BELOW
//
- session->join(std::move(thisThread), std::move(clientFd));
+ session->join(std::move(clientFd));
}
bool RpcServer::setupSocketServer(const RpcSocketAddress& addr) {
diff --git a/libs/binder/RpcSession.cpp b/libs/binder/RpcSession.cpp
index f32aa7a..05fa49e 100644
--- a/libs/binder/RpcSession.cpp
+++ b/libs/binder/RpcSession.cpp
@@ -131,14 +131,16 @@
return OK;
}
-void RpcSession::join(std::thread thread, unique_fd client) {
+void RpcSession::preJoin(std::thread thread) {
LOG_ALWAYS_FATAL_IF(thread.get_id() != std::this_thread::get_id(), "Must own this thread");
{
std::lock_guard<std::mutex> _l(mMutex);
mThreads[thread.get_id()] = std::move(thread);
}
+}
+void RpcSession::join(unique_fd client) {
// must be registered to allow arbitrary client code executing commands to
// be able to do nested calls (we can't only read from it)
sp<RpcConnection> connection = assignServerToThisThread(std::move(client));
diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h
index 92ee100..bcc213c 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -114,7 +114,10 @@
status_t readId();
- void join(std::thread thread, base::unique_fd client);
+ // transfer ownership of thread
+ void preJoin(std::thread thread);
+ // join on thread passed to preJoin
+ void join(base::unique_fd client);
void terminateLocked();
struct RpcConnection : public RefBase {