Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8b890853 authored by Yifan Hong's avatar Yifan Hong
Browse files

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
parent 02530ec0

libs/binder/Binder.cpp

+59 −12
Original line number Diff line number Diff line
@@ -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 @@ status_t IBinder::setRpcClientDebug(android::base::unique_fd socketFd, 


// ---------------------------------------------------------------------------



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 @@ public: 


    // for below objects

    Mutex mLock;

    sp<RpcServer> mRpcServer;

    std::set<sp<RpcServerLink>> mRpcServerLinks;

    BpBinder::ObjectManager mObjects;

};
@@ -489,24 +522,38 @@ status_t BBinder::setRpcClientDebug(android::base::unique_fd socketFd, 
        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;

    }

    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();

    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;

    }

    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);

libs/binder/include/binder/Binder.h

+2 −0
Original line number Diff line number Diff line
@@ -117,11 +117,13 @@ private: 
                        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;

libs/binder/include/binder/IBinder.h

+2 −2
Original line number Diff line number Diff line
@@ -162,8 +162,8 @@ public: 
     * 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

libs/binder/tests/Android.bp

+2 −0
Original line number Diff line number Diff line
@@ -62,6 +62,7 @@ cc_test { 
    shared_libs: [

        "libbase",

        "libbinder",

        "liblog",

        "libutils",

    ],

    static_libs: [
@@ -104,6 +105,7 @@ cc_test { 
    shared_libs: [

        "libbase",

        "libbinder",

        "liblog",

        "libutils",

    ],

    static_libs: [

libs/binder/tests/binderLibTest.cpp

+36 −32
Original line number Diff line number Diff line
@@ -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 android; 
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,7 +1201,35 @@ public: 
    }

};



class BinderLibRpcTest : public BinderLibRpcTestBase, public WithParamInterface<bool> {

class BinderLibRpcTest : public BinderLibRpcTestBase {};



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));

}



// 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();

    ASSERT_TRUE(socket1.ok());

    auto keepAliveBinder1 = sp<BBinder>::make();

    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket1), keepAliveBinder1), StatusEq(OK));



    auto [socket2, port2] = CreateSocket();

    ASSERT_TRUE(socket2.ok());

    auto keepAliveBinder2 = sp<BBinder>::make();

    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2), keepAliveBinder2), StatusEq(OK));

}



// 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());
@@ -1210,47 +1239,22 @@ public: 
    }

};



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), sp<BBinder>::make()), StatusEq(OK));

}



TEST_P(BinderLibRpcTest, SetRpcClientDebugNoFd) {

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(BinderLibRpcTest, SetRpcClientDebugNoKeepAliveBinder) {

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));

}



TEST_P(BinderLibRpcTest, SetRpcClientDebugTwice) {

    auto binder = GetService();

    ASSERT_TRUE(binder != nullptr);



    auto [socket1, port1] = CreateSocket();

    ASSERT_TRUE(socket1.ok());

    auto keepAliveBinder1 = sp<BBinder>::make();

    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket1), keepAliveBinder1), StatusEq(OK));



    auto [socket2, port2] = CreateSocket();

    ASSERT_TRUE(socket2.ok());

    auto keepAliveBinder2 = sp<BBinder>::make();

    EXPECT_THAT(binder->setRpcClientDebug(std::move(socket2), keepAliveBinder2),

                StatusEq(ALREADY_EXISTS));

}



INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcTest, testing::Bool(),

                        BinderLibRpcTest::ParamToString);

INSTANTIATE_TEST_CASE_P(BinderLibTest, BinderLibRpcTestP, testing::Bool(),

                        BinderLibRpcTestP::ParamToString);



class BinderLibTestService;

class BinderLibRpcClientTest : public BinderLibRpcTestBase,