Implement keepAliveBinder for setRpcClientDebug
setRpcClientDebug can now be called multiple times. Each will
set up a different RpcServer and opens up a different port.
When keepAliveBinder dies, the corresponding RpcServer also
dies.
Also fixes tests to meet new expectations.
- Positive tests now only run on remote binder, because the
keepAliveBinder object cannot be a local binder to call
linkToDeath.
- Negative tests keeps running on local and remote binders.
- Use real servicedispatcher.
Test: binderLibTest
Test: manual with servicedispatcher.
Bug: 182914638
Change-Id: Ic46ac54bb7988ee7d65546563f56f0a02c29ce7c
diff --git a/libs/binder/Binder.cpp b/libs/binder/Binder.cpp
index 19a22a5..d811b17 100644
--- a/libs/binder/Binder.cpp
+++ b/libs/binder/Binder.cpp
@@ -17,6 +17,7 @@
#include <binder/Binder.h>
#include <atomic>
+#include <set>
#include <android-base/unique_fd.h>
#include <binder/BpBinder.h>
@@ -180,6 +181,38 @@
// ---------------------------------------------------------------------------
+class BBinder::RpcServerLink : public IBinder::DeathRecipient {
+public:
+ // On binder died, calls RpcServer::shutdown on @a rpcServer, and removes itself from @a binder.
+ RpcServerLink(const sp<RpcServer>& rpcServer, const sp<IBinder>& keepAliveBinder,
+ const wp<BBinder>& binder)
+ : mRpcServer(rpcServer), mKeepAliveBinder(keepAliveBinder), mBinder(binder) {}
+ void binderDied(const wp<IBinder>&) override {
+ LOG_RPC_DETAIL("RpcServerLink: binder died, shutting down RpcServer");
+ if (mRpcServer == nullptr) {
+ ALOGW("RpcServerLink: Unable to shut down RpcServer because it does not exist.");
+ } else {
+ ALOGW_IF(!mRpcServer->shutdown(),
+ "RpcServerLink: RpcServer did not shut down properly. Not started?");
+ }
+ mRpcServer.clear();
+
+ auto promoted = mBinder.promote();
+ if (promoted == nullptr) {
+ ALOGW("RpcServerLink: Unable to remove link from parent binder object because parent "
+ "binder object is gone.");
+ } else {
+ promoted->removeRpcServerLink(sp<RpcServerLink>::fromExisting(this));
+ }
+ mBinder.clear();
+ }
+
+private:
+ sp<RpcServer> mRpcServer;
+ sp<IBinder> mKeepAliveBinder; // hold to avoid automatically unlinking
+ wp<BBinder> mBinder;
+};
+
class BBinder::Extras
{
public:
@@ -192,7 +225,7 @@
// for below objects
Mutex mLock;
- sp<RpcServer> mRpcServer;
+ std::set<sp<RpcServerLink>> mRpcServerLinks;
BpBinder::ObjectManager mObjects;
};
@@ -489,24 +522,38 @@
return INVALID_OPERATION;
}
+ // Weak ref to avoid circular dependency:
+ // BBinder -> RpcServerLink ----> RpcServer -X-> BBinder
+ // `-X-> BBinder
+ auto weakThis = wp<BBinder>::fromExisting(this);
+
Extras* e = getOrCreateExtras();
AutoMutex _l(e->mLock);
- if (e->mRpcServer != nullptr) {
- ALOGE("%s: Already have RPC client", __PRETTY_FUNCTION__);
- return ALREADY_EXISTS;
+ auto rpcServer = RpcServer::make();
+ LOG_ALWAYS_FATAL_IF(rpcServer == nullptr, "RpcServer::make returns null");
+ rpcServer->iUnderstandThisCodeIsExperimentalAndIWillNotUseItInProduction();
+ auto link = sp<RpcServerLink>::make(rpcServer, keepAliveBinder, weakThis);
+ if (auto status = keepAliveBinder->linkToDeath(link, nullptr, 0); status != OK) {
+ ALOGE("%s: keepAliveBinder->linkToDeath returns %s", __PRETTY_FUNCTION__,
+ statusToString(status).c_str());
+ return status;
}
- e->mRpcServer = RpcServer::make();
- LOG_ALWAYS_FATAL_IF(e->mRpcServer == nullptr, "RpcServer::make returns null");
- e->mRpcServer->iUnderstandThisCodeIsExperimentalAndIWillNotUseItInProduction();
- // Weak ref to avoid circular dependency: BBinder -> RpcServer -X-> BBinder
- e->mRpcServer->setRootObjectWeak(wp<BBinder>::fromExisting(this));
- e->mRpcServer->setupExternalServer(std::move(socketFd));
- e->mRpcServer->setMaxThreads(binderThreadPoolMaxCount);
- e->mRpcServer->start();
+ rpcServer->setRootObjectWeak(weakThis);
+ rpcServer->setupExternalServer(std::move(socketFd));
+ rpcServer->setMaxThreads(binderThreadPoolMaxCount);
+ rpcServer->start();
+ e->mRpcServerLinks.emplace(link);
LOG_RPC_DETAIL("%s(fd=%d) successful", __PRETTY_FUNCTION__, socketFdForPrint);
return OK;
}
+void BBinder::removeRpcServerLink(const sp<RpcServerLink>& link) {
+ Extras* e = mExtras.load(std::memory_order_acquire);
+ if (!e) return;
+ AutoMutex _l(e->mLock);
+ (void)e->mRpcServerLinks.erase(link);
+}
+
BBinder::~BBinder()
{
Extras* e = mExtras.load(std::memory_order_relaxed);
diff --git a/libs/binder/include/binder/Binder.h b/libs/binder/include/binder/Binder.h
index 0cc0ef1..472e546 100644
--- a/libs/binder/include/binder/Binder.h
+++ b/libs/binder/include/binder/Binder.h
@@ -117,11 +117,13 @@
BBinder(const BBinder& o);
BBinder& operator=(const BBinder& o);
+ class RpcServerLink;
class Extras;
Extras* getOrCreateExtras();
[[nodiscard]] status_t setRpcClientDebug(const Parcel& data);
+ void removeRpcServerLink(const sp<RpcServerLink>& link);
std::atomic<Extras*> mExtras;
diff --git a/libs/binder/include/binder/IBinder.h b/libs/binder/include/binder/IBinder.h
index 122c595..f9cdac7 100644
--- a/libs/binder/include/binder/IBinder.h
+++ b/libs/binder/include/binder/IBinder.h
@@ -162,8 +162,8 @@
* 2. spawns 1 new thread that calls RpcServer::join()
* - 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.
+ * setRpcClientDebug() may be called multiple times. Each call will add a new RpcServer
+ * and opens up a TCP port.
*
* Note: A thread is spawned for each accept()'ed fd, which may call into functions of the
* interface freely. See RpcServer::join(). To avoid such race conditions, implement the service
diff --git a/libs/binder/tests/Android.bp b/libs/binder/tests/Android.bp
index c7c899f..326d9fd 100644
--- a/libs/binder/tests/Android.bp
+++ b/libs/binder/tests/Android.bp
@@ -62,6 +62,7 @@
shared_libs: [
"libbase",
"libbinder",
+ "liblog",
"libutils",
],
static_libs: [
@@ -104,6 +105,7 @@
shared_libs: [
"libbase",
"libbinder",
+ "liblog",
"libutils",
],
static_libs: [
diff --git a/libs/binder/tests/binderLibTest.cpp b/libs/binder/tests/binderLibTest.cpp
index abbb16e..1ecb0b7 100644
--- a/libs/binder/tests/binderLibTest.cpp
+++ b/libs/binder/tests/binderLibTest.cpp
@@ -15,14 +15,13 @@
*/
#include <errno.h>
-#include <fcntl.h>
-#include <fstream>
#include <poll.h>
#include <pthread.h>
#include <stdio.h>
#include <stdlib.h>
#include <chrono>
+#include <fstream>
#include <thread>
#include <gmock/gmock.h>
@@ -31,6 +30,7 @@
#include <android-base/properties.h>
#include <android-base/result-gmock.h>
#include <android-base/result.h>
+#include <android-base/strings.h>
#include <android-base/unique_fd.h>
#include <binder/Binder.h>
#include <binder/BpBinder.h>
@@ -55,6 +55,7 @@
using namespace std::string_literals;
using namespace std::chrono_literals;
using android::base::testing::HasValue;
+using android::base::testing::Ok;
using testing::ExplainMatchResult;
using testing::Not;
using testing::WithParamInterface;
@@ -1200,41 +1201,19 @@
}
};
-class BinderLibRpcTest : public BinderLibRpcTestBase, public WithParamInterface<bool> {
-public:
- sp<IBinder> GetService() {
- return GetParam() ? sp<IBinder>(addServer()) : sp<IBinder>(sp<BBinder>::make());
- }
- static std::string ParamToString(const testing::TestParamInfo<ParamType> &info) {
- return info.param ? "remote" : "local";
- }
-};
+class BinderLibRpcTest : public BinderLibRpcTestBase {};
-TEST_P(BinderLibRpcTest, SetRpcClientDebug) {
- auto binder = GetService();
+TEST_F(BinderLibRpcTest, SetRpcClientDebug) {
+ auto binder = addServer();
ASSERT_TRUE(binder != nullptr);
auto [socket, port] = CreateSocket();
ASSERT_TRUE(socket.ok());
EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), sp<BBinder>::make()), StatusEq(OK));
}
-TEST_P(BinderLibRpcTest, SetRpcClientDebugNoFd) {
- auto binder = GetService();
- ASSERT_TRUE(binder != nullptr);
- EXPECT_THAT(binder->setRpcClientDebug(android::base::unique_fd(), sp<BBinder>::make()),
- StatusEq(BAD_VALUE));
-}
-
-TEST_P(BinderLibRpcTest, SetRpcClientDebugNoKeepAliveBinder) {
- auto binder = GetService();
- ASSERT_TRUE(binder != nullptr);
- auto [socket, port] = CreateSocket();
- ASSERT_TRUE(socket.ok());
- EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), nullptr), StatusEq(UNEXPECTED_NULL));
-}
-
-TEST_P(BinderLibRpcTest, SetRpcClientDebugTwice) {
- auto binder = GetService();
+// Tests for multiple RpcServer's on the same binder object.
+TEST_F(BinderLibRpcTest, SetRpcClientDebugTwice) {
+ auto binder = addServer();
ASSERT_TRUE(binder != nullptr);
auto [socket1, port1] = CreateSocket();
@@ -1245,12 +1224,37 @@
auto [socket2, port2] = CreateSocket();
ASSERT_TRUE(socket2.ok());
auto keepAliveBinder2 = sp<BBinder>::make();
- EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2), keepAliveBinder2),
- StatusEq(ALREADY_EXISTS));
+ EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2), keepAliveBinder2), StatusEq(OK));
}
-INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcTest, testing::Bool(),
- BinderLibRpcTest::ParamToString);
+// Negative tests for RPC APIs on IBinder. Call should fail in the same way on both remote and
+// local binders.
+class BinderLibRpcTestP : public BinderLibRpcTestBase, public WithParamInterface<bool> {
+public:
+ sp<IBinder> GetService() {
+ return GetParam() ? sp<IBinder>(addServer()) : sp<IBinder>(sp<BBinder>::make());
+ }
+ static std::string ParamToString(const testing::TestParamInfo<ParamType> &info) {
+ return info.param ? "remote" : "local";
+ }
+};
+
+TEST_P(BinderLibRpcTestP, SetRpcClientDebugNoFd) {
+ auto binder = GetService();
+ ASSERT_TRUE(binder != nullptr);
+ EXPECT_THAT(binder->setRpcClientDebug(android::base::unique_fd(), sp<BBinder>::make()),
+ StatusEq(BAD_VALUE));
+}
+
+TEST_P(BinderLibRpcTestP, SetRpcClientDebugNoKeepAliveBinder) {
+ auto binder = GetService();
+ ASSERT_TRUE(binder != nullptr);
+ auto [socket, port] = CreateSocket();
+ ASSERT_TRUE(socket.ok());
+ EXPECT_THAT(binder->setRpcClientDebug(std::move(socket), nullptr), StatusEq(UNEXPECTED_NULL));
+}
+INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcTestP, testing::Bool(),
+ BinderLibRpcTestP::ParamToString);
class BinderLibTestService;
class BinderLibRpcClientTest : public BinderLibRpcTestBase,