From 5802c2b0c76b821c63c989afc8078bd5d2ddf2cb Mon Sep 17 00:00:00 2001
From: Steven Moreland <smoreland@google.com>
Date: Wed, 12 May 2021 20:13:04 +0000
Subject: 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
---
 libs/binder/RpcServer.cpp | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

(limited to 'libs/binder/RpcServer.cpp')

diff --git a/libs/binder/RpcServer.cpp b/libs/binder/RpcServer.cpp
index 3a98cadb94..06c3a4206d 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 @@ void RpcServer::establishConnection(sp<RpcServer>&& server, base::unique_fd clie
 
     // 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 @@ void RpcServer::establishConnection(sp<RpcServer>&& server, base::unique_fd clie
         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 @@ void RpcServer::establishConnection(sp<RpcServer>&& server, base::unique_fd clie
             }
             session = it->second;
         }
+
+        detachGuard.Disable();
+        session->preJoin(std::move(thisThread));
     }
 
     // avoid strong cycle
@@ -199,7 +210,7 @@ void RpcServer::establishConnection(sp<RpcServer>&& server, base::unique_fd clie
     // 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) {
-- 
cgit v1.2.3-59-g8ed1b