Merge "binder: setRpcClientDebug drops numThreads argument."

6 files changed, 49 insertions, 84 deletions

diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 415b44e683..65d2079463 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -150,7 +150,7 @@ status_t IBinder::getDebugPid(pid_t* out) {
     return OK;
 }
 
-status_t IBinder::setRpcClientDebug(android::base::unique_fd socketFd, uint32_t maxRpcThreads) {
+status_t IBinder::setRpcClientDebug(android::base::unique_fd socketFd) {
     if constexpr (!kEnableRpcDevServers) {
         ALOGW("setRpcClientDebug disallowed because RPC is not enabled");
         return INVALID_OPERATION;
@@ -158,7 +158,7 @@ status_t IBinder::setRpcClientDebug(android::base::unique_fd socketFd, uint32_t
 
     BBinder* local = this->localBinder();
     if (local != nullptr) {
-        return local->BBinder::setRpcClientDebug(std::move(socketFd), maxRpcThreads);
+        return local->BBinder::setRpcClientDebug(std::move(socketFd));
     }
 
     BpBinder* proxy = this->remoteBinder();
@@ -173,7 +173,6 @@ status_t IBinder::setRpcClientDebug(android::base::unique_fd socketFd, uint32_t
         status = data.writeFileDescriptor(socketFd.release(), true /* own */);
         if (status != OK) return status;
     }
-    if (status = data.writeUint32(maxRpcThreads); status != OK) return status;
     return transact(SET_RPC_CLIENT_TRANSACTION, data, &reply);
 }
 
@@ -449,36 +448,29 @@ status_t BBinder::setRpcClientDebug(const Parcel& data) {
     status_t status;
     bool hasSocketFd;
     android::base::unique_fd clientFd;
-    uint32_t maxRpcThreads;
 
     if (status = data.readBool(&hasSocketFd); status != OK) return status;
     if (hasSocketFd) {
         if (status = data.readUniqueFileDescriptor(&clientFd); status != OK) return status;
     }
-    if (status = data.readUint32(&maxRpcThreads); status != OK) return status;
 
-    return setRpcClientDebug(std::move(clientFd), maxRpcThreads);
+    return setRpcClientDebug(std::move(clientFd));
 }
 
-status_t BBinder::setRpcClientDebug(android::base::unique_fd socketFd, uint32_t maxRpcThreads) {
+status_t BBinder::setRpcClientDebug(android::base::unique_fd socketFd) {
     if constexpr (!kEnableRpcDevServers) {
         ALOGW("%s: disallowed because RPC is not enabled", __PRETTY_FUNCTION__);
         return INVALID_OPERATION;
     }
 
     const int socketFdForPrint = socketFd.get();
-    LOG_RPC_DETAIL("%s(%d, %" PRIu32 ")", __PRETTY_FUNCTION__, socketFdForPrint, maxRpcThreads);
+    LOG_RPC_DETAIL("%s(fd=%d)", __PRETTY_FUNCTION__, socketFdForPrint);
 
     if (!socketFd.ok()) {
         ALOGE("%s: No socket FD provided.", __PRETTY_FUNCTION__);
         return BAD_VALUE;
     }
-    if (maxRpcThreads <= 0) {
-        ALOGE("%s: RPC is useless with %" PRIu32 " threads.", __PRETTY_FUNCTION__, maxRpcThreads);
-        return BAD_VALUE;
-    }
 
-    // TODO(b/182914638): RPC and binder should share the same thread pool count.
     size_t binderThreadPoolMaxCount = ProcessState::self()->getThreadPoolMaxThreadCount();
     if (binderThreadPoolMaxCount <= 1) {
         ALOGE("%s: ProcessState thread pool max count is %zu. RPC is disabled for this service "
@@ -500,8 +492,7 @@ status_t BBinder::setRpcClientDebug(android::base::unique_fd socketFd, uint32_t
     e->mRpcServer->setRootObjectWeak(wp<BBinder>::fromExisting(this));
     e->mRpcServer->setupExternalServer(std::move(socketFd));
     e->mRpcServer->start();
-    LOG_RPC_DETAIL("%s(%d, %" PRIu32 ") successful", __PRETTY_FUNCTION__, socketFdForPrint,
-                   maxRpcThreads);
+    LOG_RPC_DETAIL("%s(fd=%d) successful", __PRETTY_FUNCTION__, socketFdForPrint);
     return OK;
 }
 
diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h
index d162dda16c..53885ffc4e 100644
--- a/libs/binder/include/binder/Binder.h
+++ b/libs/binder/include/binder/Binder.h
@@ -101,8 +101,7 @@ public:
     // to another process.
     void setParceled();
 
-    [[nodiscard]] status_t setRpcClientDebug(android::base::unique_fd clientFd,
-                                             uint32_t maxRpcThreads);
+    [[nodiscard]] status_t setRpcClientDebug(android::base::unique_fd clientFd);
 
 protected:
     virtual             ~BBinder();
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index ce28d7c706..06cb21dac4 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -157,12 +157,10 @@ public:
      * Set the RPC client fd to this binder service, for debugging. This is only available on
      * debuggable builds.
      *
-     * |maxRpcThreads| must be positive because RPC is useless without threads.
-     *
      * When this is called on a binder service, the service:
      * 1. sets up RPC server
      * 2. spawns 1 new thread that calls RpcServer::join()
-     *    - join() spawns at most |maxRpcThreads| threads that accept() connections; see RpcServer
+     *    - join() spawns some number of threads that accept() connections; see RpcServer
      *
      * setRpcClientDebug() may only be called once.
      * TODO(b/182914638): If allow to shut down the client, addRpcClient can be called repeatedly.
@@ -171,8 +169,7 @@ public:
      * interface freely. See RpcServer::join(). To avoid such race conditions, implement the service
      * functions with multithreading support.
      */
-    [[nodiscard]] status_t setRpcClientDebug(android::base::unique_fd socketFd,
-                                             uint32_t maxRpcThreads);
+    [[nodiscard]] status_t setRpcClientDebug(android::base::unique_fd socketFd);
 
     // NOLINTNEXTLINE(google-default-arguments)
     virtual status_t        transact(   uint32_t code,
diff --git a/libs/binder/servicedispatcher.cpp b/libs/binder/servicedispatcher.cpp
index f61df084fc..a2fa842cf6 100644
--- a/libs/binder/servicedispatcher.cpp
+++ b/libs/binder/servicedispatcher.cpp
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-#include <stdint.h>
 #include <sysexits.h>
 #include <unistd.h>
 
@@ -22,7 +21,6 @@
 
 #include <android-base/file.h>
 #include <android-base/logging.h>
-#include <android-base/parseint.h>
 #include <android-base/properties.h>
 #include <android-base/stringprintf.h>
 #include <binder/IServiceManager.h>
@@ -39,7 +37,6 @@ using android::base::InitLogging;
 using android::base::LogdLogger;
 using android::base::LogId;
 using android::base::LogSeverity;
-using android::base::ParseUint;
 using android::base::StdioLogger;
 using android::base::StringPrintf;
 
@@ -47,15 +44,14 @@ namespace {
 int Usage(const char* program) {
     auto format = R"(dispatch calls to RPC service.
 Usage:
-  %s [-n <num_threads>] <service_name>
-    -n <num_threads>: number of RPC threads added to the service (default 1).
+  %s <service_name>
     <service_name>: the service to connect to.
 )";
     LOG(ERROR) << StringPrintf(format, Basename(program).c_str());
     return EX_USAGE;
 }
 
-int Dispatch(const char* name, uint32_t numThreads) {
+int Dispatch(const char* name) {
     auto sm = defaultServiceManager();
     if (nullptr == sm) {
         LOG(ERROR) << "No servicemanager";
@@ -78,13 +74,12 @@ int Dispatch(const char* name, uint32_t numThreads) {
         return EX_SOFTWARE;
     }
     auto socket = rpcServer->releaseServer();
-    auto status = binder->setRpcClientDebug(std::move(socket), numThreads);
+    auto status = binder->setRpcClientDebug(std::move(socket));
     if (status != OK) {
         LOG(ERROR) << "setRpcClientDebug failed with " << statusToString(status);
         return EX_SOFTWARE;
     }
-    LOG(INFO) << "Finish setting up RPC on service " << name << " with " << numThreads
-              << " threads on port" << port;
+    LOG(INFO) << "Finish setting up RPC on service " << name << " on port" << port;
 
     std::cout << port << std::endl;
     return EX_OK;
@@ -117,15 +112,9 @@ int main(int argc, char* argv[]) {
     }
     LOG(WARNING) << "WARNING: servicedispatcher is debug only. Use with caution.";
 
-    uint32_t numThreads = 1;
     int opt;
-    while (-1 != (opt = getopt(argc, argv, "n:"))) {
+    while (-1 != (opt = getopt(argc, argv, ""))) {
         switch (opt) {
-            case 'n': {
-                if (!ParseUint(optarg, &numThreads)) {
-                    return Usage(argv[0]);
-                }
-            } break;
             default: {
                 return Usage(argv[0]);
             }
@@ -134,5 +123,5 @@ int main(int argc, char* argv[]) {
     if (optind + 1 != argc) return Usage(argv[0]);
     auto name = argv[optind];
 
-    return Dispatch(name, numThreads);
+    return Dispatch(name);
 }
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index c4eacfdc9e..9370f5259d 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -33,6 +33,7 @@
 #include <android-base/result.h>
 #include <android-base/unique_fd.h>
 #include <binder/Binder.h>
+#include <binder/BpBinder.h>
 #include <binder/IBinder.h>
 #include <binder/IPCThreadState.h>
 #include <binder/IServiceManager.h>
@@ -112,7 +113,6 @@ enum BinderLibTestTranscationCode {
     BINDER_LIB_TEST_ECHO_VECTOR,
     BINDER_LIB_TEST_REJECT_BUF,
     BINDER_LIB_TEST_CAN_GET_SID,
-    BINDER_LIB_TEST_USLEEP,
     BINDER_LIB_TEST_CREATE_TEST_SERVICE,
 };
 
@@ -1210,39 +1210,31 @@ public:
     }
 };
 
-TEST_P(BinderLibRpcTest, SetRpcMaxThreads) {
+TEST_P(BinderLibRpcTest, SetRpcClientDebug) {
     auto binder = GetService();
     ASSERT_TRUE(binder != nullptr);
     auto [socket, port] = CreateSocket();
     ASSERT_TRUE(socket.ok());
-    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), 1), StatusEq(OK));
+    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket)), StatusEq(OK));
 }
 
-TEST_P(BinderLibRpcTest, SetRpcClientNoFd) {
+TEST_P(BinderLibRpcTest, SetRpcClientDebugNoFd) {
     auto binder = GetService();
     ASSERT_TRUE(binder != nullptr);
-    EXPECT_THAT(binder->setRpcClientDebug(android::base::unique_fd(), 1), StatusEq(BAD_VALUE));
+    EXPECT_THAT(binder->setRpcClientDebug(android::base::unique_fd()), StatusEq(BAD_VALUE));
 }
 
-TEST_P(BinderLibRpcTest, SetRpcMaxThreadsZero) {
-    auto binder = GetService();
-    ASSERT_TRUE(binder != nullptr);
-    auto [socket, port] = CreateSocket();
-    ASSERT_TRUE(socket.ok());
-    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), 0), StatusEq(BAD_VALUE));
-}
-
-TEST_P(BinderLibRpcTest, SetRpcMaxThreadsTwice) {
+TEST_P(BinderLibRpcTest, SetRpcClientDebugTwice) {
     auto binder = GetService();
     ASSERT_TRUE(binder != nullptr);
 
     auto [socket1, port1] = CreateSocket();
     ASSERT_TRUE(socket1.ok());
-    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket1), 1), StatusEq(OK));
+    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket1)), StatusEq(OK));
 
     auto [socket2, port2] = CreateSocket();
     ASSERT_TRUE(socket2.ok());
-    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2), 1), StatusEq(ALREADY_EXISTS));
+    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2)), StatusEq(ALREADY_EXISTS));
 }
 
 INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcTest, testing::Bool(),
@@ -1288,42 +1280,47 @@ TEST_P(BinderLibRpcClientTest, Test) {
         auto [socket, socketPort] = CreateSocket();
         ASSERT_TRUE(socket.ok());
         port = socketPort;
-        ASSERT_THAT(server->setRpcClientDebug(std::move(socket), numThreads), StatusEq(OK));
+        ASSERT_THAT(server->setRpcClientDebug(std::move(socket)), StatusEq(OK));
     }
 
-    auto callUsleep = [](sp<IBinder> server, uint64_t us) {
-        Parcel data, reply;
-        data.markForBinder(server);
-        const char *name = data.isForRpc() ? "RPC" : "binder";
-        EXPECT_THAT(data.writeUint64(us), StatusEq(OK));
-        EXPECT_THAT(server->transact(BINDER_LIB_TEST_USLEEP, data, &reply), StatusEq(OK))
-                << "for " << name << " server";
-    };
+    std::mutex mutex;
+    std::condition_variable cv;
+    bool start = false;
 
     auto threadFn = [&](size_t threadNum) {
-        usleep(threadNum * 50 * 1000); // threadNum * 50ms. Need this to avoid SYN flooding.
+        usleep(threadNum * 10 * 1000); // threadNum * 10ms. Need this to avoid SYN flooding.
         auto rpcSession = RpcSession::make();
         ASSERT_TRUE(rpcSession->setupInetClient("127.0.0.1", port));
         auto rpcServerBinder = rpcSession->getRootObject();
         ASSERT_NE(nullptr, rpcServerBinder);
-
-        EXPECT_EQ(OK, rpcServerBinder->pingBinder());
-
         // Check that |rpcServerBinder| and |server| points to the same service.
-        EXPECT_THAT(GetId(rpcServerBinder), HasValue(id));
+        EXPECT_THAT(GetId(rpcServerBinder), HasValue(id)) << "For thread #" << threadNum;
 
-        // Occupy the server thread. The server should still have enough threads to handle
-        // other connections.
-        // (numThreads - threadNum) * 100ms
-        callUsleep(rpcServerBinder, (numThreads - threadNum) * 100 * 1000);
+        {
+            std::unique_lock<std::mutex> lock(mutex);
+            ASSERT_TRUE(cv.wait_for(lock, 1s, [&] { return start; }));
+        }
+        // Let all threads almost simultaneously ping the service.
+        for (size_t i = 0; i < 100; ++i) {
+            EXPECT_THAT(rpcServerBinder->pingBinder(), StatusEq(OK))
+                    << "For thread #" << threadNum << ", iteration " << i;
+        }
     };
+
     std::vector<std::thread> threads;
     for (size_t i = 0; i < numThreads; ++i) threads.emplace_back(std::bind(threadFn, i));
+
+    {
+        std::lock_guard<std::mutex> lock(mutex);
+        start = true;
+    }
+    cv.notify_all();
+
     for (auto &t : threads) t.join();
 }
 
 INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcClientTest,
-                        testing::Combine(testing::Bool(), testing::Range(1u, 10u)),
+                        testing::Combine(testing::Bool(), testing::Values(1u, 10u)),
                         BinderLibRpcClientTest::ParamToString);
 
 class BinderLibTestService : public BBinder {
@@ -1640,12 +1637,6 @@ public:
             case BINDER_LIB_TEST_CAN_GET_SID: {
                 return IPCThreadState::self()->getCallingSid() == nullptr ? BAD_VALUE : NO_ERROR;
             }
-            case BINDER_LIB_TEST_USLEEP: {
-                uint64_t us;
-                if (status_t status = data.readUint64(&us); status != NO_ERROR) return status;
-                usleep(us);
-                return NO_ERROR;
-            }
             case BINDER_LIB_TEST_CREATE_TEST_SERVICE: {
                 int32_t id;
                 if (status_t status = data.readInt32(&id); status != NO_ERROR) return status;
diff --git a/libs/binder/tests/unit_fuzzers/BinderFuzzFunctions.h b/libs/binder/tests/unit_fuzzers/BinderFuzzFunctions.h
index 72c5bc4342..1484fec786 100644
--- a/libs/binder/tests/unit_fuzzers/BinderFuzzFunctions.h
+++ b/libs/binder/tests/unit_fuzzers/BinderFuzzFunctions.h
@@ -73,10 +73,8 @@ static const std::vector<std::function<void(FuzzedDataProvider*, const sp<BBinde
                               [](FuzzedDataProvider*, const sp<BBinder>& bbinder) -> void {
                                   bbinder->getDebugPid();
                               },
-                              [](FuzzedDataProvider* fdp, const sp<BBinder>& bbinder) -> void {
-                                  auto rpcMaxThreads = fdp->ConsumeIntegralInRange<uint32_t>(0, 20);
-                                  (void)bbinder->setRpcClientDebug(android::base::unique_fd(),
-                                                                   rpcMaxThreads);
+                              [](FuzzedDataProvider*, const sp<BBinder>& bbinder) -> void {
+                                  (void)bbinder->setRpcClientDebug(android::base::unique_fd());
                               }};
 
 } // namespace android