binder: setRpcClientDebug drops numThreads argument.
The old numThreads argument is useless and unused.
Also fix tests to reflect such expectations properly.
Test: binderLibTest
Bug: 190435077
Change-Id: Idb0229db368aea4cd6ba9b0dd2630052fc636e58
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 415b44e..65d2079 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -150,7 +150,7 @@
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 @@
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 = 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 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 @@
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 d162dda..53885ff 100644
--- a/libs/binder/include/binder/Binder.h
+++ b/libs/binder/include/binder/Binder.h
@@ -101,8 +101,7 @@
// 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 ce28d7c..06cb21d 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -157,12 +157,10 @@
* 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 @@
* 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 f61df08..a2fa842 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::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 @@
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 @@
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 @@
}
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 @@
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 c4eacfd..9370f52 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 @@
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 @@
}
};
-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 @@
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 @@
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 72c5bc4..1484fec 100644
--- a/libs/binder/tests/unit_fuzzers/BinderFuzzFunctions.h
+++ b/libs/binder/tests/unit_fuzzers/BinderFuzzFunctions.h
@@ -73,10 +73,8 @@
[](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