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

Commit b098318c authored by Steven Moreland's avatar Steven Moreland
Browse files

servicemanager: use safer ref base semantics 

Bug: 184190315
Test: N/A
Change-Id: Iafa793f4f4a69651a281888a13369b802006be0e
parent 8911d46b

cmds/servicemanager/Android.bp

+1 −0
Original line number Diff line number Diff line
@@ -14,6 +14,7 @@ cc_defaults { 
        "-Wall",

        "-Wextra",

        "-Werror",

        "-DANDROID_UTILS_REF_BASE_DISABLE_IMPLICIT_CONSTRUCTION",

    ],



    srcs: [

cmds/servicemanager/ServiceManager.cpp

+8 −4
Original line number Diff line number Diff line
@@ -239,7 +239,8 @@ Status ServiceManager::addService(const std::string& name, const sp<IBinder>& bi 
#endif  // !VENDORSERVICEMANAGER



    // implicitly unlinked when the binder is removed

    if (binder->remoteBinder() != nullptr && binder->linkToDeath(this) != OK) {

    if (binder->remoteBinder() != nullptr &&

        binder->linkToDeath(sp<ServiceManager>::fromExisting(this)) != OK) {

        LOG(ERROR) << "Could not linkToDeath when adding " << name;

        return Status::fromExceptionCode(Status::EX_ILLEGAL_STATE);

    }
@@ -307,7 +308,9 @@ Status ServiceManager::registerForNotifications( 
        return Status::fromExceptionCode(Status::EX_NULL_POINTER);

    }



    if (OK != IInterface::asBinder(callback)->linkToDeath(this)) {

    if (OK !=

        IInterface::asBinder(callback)->linkToDeath(

                sp<ServiceManager>::fromExisting(this))) {

        LOG(ERROR) << "Could not linkToDeath when adding " << name;

        return Status::fromExceptionCode(Status::EX_ILLEGAL_STATE);

    }
@@ -461,7 +464,8 @@ Status ServiceManager::registerClientCallback(const std::string& name, const sp< 
        return Status::fromExceptionCode(Status::EX_ILLEGAL_ARGUMENT);

    }



    if (OK != IInterface::asBinder(cb)->linkToDeath(this)) {

    if (OK !=

        IInterface::asBinder(cb)->linkToDeath(sp<ServiceManager>::fromExisting(this))) {

        LOG(ERROR) << "Could not linkToDeath when adding client callback for " << name;

        return Status::fromExceptionCode(Status::EX_ILLEGAL_STATE);

    }
@@ -491,7 +495,7 @@ void ServiceManager::removeClientCallback(const wp<IBinder>& who, 
}



ssize_t ServiceManager::Service::getNodeStrongRefCount() {

    sp<BpBinder> bpBinder = binder->remoteBinder();

    sp<BpBinder> bpBinder = sp<BpBinder>::fromExisting(binder->remoteBinder());

    if (bpBinder == nullptr) return -1;



    return ProcessState::self()->getStrongRefCountForNode(bpBinder);

cmds/servicemanager/main.cpp

+4 −3
Original line number Diff line number Diff line
@@ -39,7 +39,7 @@ using ::android::sp; 
class BinderCallback : public LooperCallback {

public:

    static sp<BinderCallback> setupTo(const sp<Looper>& looper) {

        sp<BinderCallback> cb = new BinderCallback;

        sp<BinderCallback> cb = sp<BinderCallback>::make();



        int binder_fd = -1;

        IPCThreadState::self()->setupPolling(&binder_fd);
@@ -65,7 +65,7 @@ public: 
class ClientCallbackCallback : public LooperCallback {

public:

    static sp<ClientCallbackCallback> setupTo(const sp<Looper>& looper, const sp<ServiceManager>& manager) {

        sp<ClientCallbackCallback> cb = new ClientCallbackCallback(manager);

        sp<ClientCallbackCallback> cb = sp<ClientCallbackCallback>::make(manager);



        int fdTimer = timerfd_create(CLOCK_MONOTONIC, 0 /*flags*/);

        LOG_ALWAYS_FATAL_IF(fdTimer < 0, "Failed to timerfd_create: fd: %d err: %d", fdTimer, errno);
@@ -105,6 +105,7 @@ public: 
        return 1;  // Continue receiving callbacks.

    }

private:

    friend sp<ClientCallbackCallback>;

    ClientCallbackCallback(const sp<ServiceManager>& manager) : mManager(manager) {}

    sp<ServiceManager> mManager;

};
@@ -120,7 +121,7 @@ int main(int argc, char** argv) { 
    ps->setThreadPoolMaxThreadCount(0);

    ps->setCallRestriction(ProcessState::CallRestriction::FATAL_IF_NOT_ONEWAY);



    sp<ServiceManager> manager = new ServiceManager(std::make_unique<Access>());

    sp<ServiceManager> manager = sp<ServiceManager>::make(std::make_unique<Access>());

    if (!manager->addService("manager", manager, false /*allowIsolated*/, IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk()) {

        LOG(ERROR) << "Could not self register servicemanager";

    }

cmds/servicemanager/test_sm.cpp

+19 −19
Original line number Diff line number Diff line
@@ -46,7 +46,7 @@ static sp<IBinder> getBinder() { 
        }

    };



    return new LinkableBinder;

    return sp<LinkableBinder>::make();

}



class MockAccess : public Access {
@@ -71,7 +71,7 @@ static sp<ServiceManager> getPermissiveServiceManager() { 
    ON_CALL(*access, canFind(_, _)).WillByDefault(Return(true));

    ON_CALL(*access, canList(_)).WillByDefault(Return(true));



    sp<ServiceManager> sm = new NiceMock<MockServiceManager>(std::move(access));

    sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));

    return sm;

}
@@ -119,7 +119,7 @@ TEST(AddService, AddDisallowedFromApp) { 
            .uid = uid,

        }));

        EXPECT_CALL(*access, canAdd(_, _)).Times(0);

        sp<ServiceManager> sm = new NiceMock<MockServiceManager>(std::move(access));

        sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));



        EXPECT_FALSE(sm->addService("foo", getBinder(), false /*allowIsolated*/,

            IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());
@@ -161,7 +161,7 @@ TEST(AddService, NoPermissions) { 
    EXPECT_CALL(*access, getCallingContext()).WillOnce(Return(Access::CallingContext{}));

    EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(false));



    sp<ServiceManager> sm = new NiceMock<MockServiceManager>(std::move(access));

    sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));



    EXPECT_FALSE(sm->addService("foo", getBinder(), false /*allowIsolated*/,

        IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());
@@ -194,7 +194,7 @@ TEST(GetService, NoPermissionsForGettingService) { 
    EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));

    EXPECT_CALL(*access, canFind(_, _)).WillOnce(Return(false));



    sp<ServiceManager> sm = new NiceMock<MockServiceManager>(std::move(access));

    sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));



    EXPECT_TRUE(sm->addService("foo", getBinder(), false /*allowIsolated*/,

        IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());
@@ -218,7 +218,7 @@ TEST(GetService, AllowedFromIsolated) { 
    EXPECT_CALL(*access, canAdd(_, _)).WillOnce(Return(true));

    EXPECT_CALL(*access, canFind(_, _)).WillOnce(Return(true));



    sp<ServiceManager> sm = new NiceMock<MockServiceManager>(std::move(access));

    sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));



    sp<IBinder> service = getBinder();

    EXPECT_TRUE(sm->addService("foo", service, true /*allowIsolated*/,
@@ -244,7 +244,7 @@ TEST(GetService, NotAllowedFromIsolated) { 
    // TODO(b/136023468): when security check is first, this should be called first

    // EXPECT_CALL(*access, canFind(_, _)).WillOnce(Return(true));



    sp<ServiceManager> sm = new NiceMock<MockServiceManager>(std::move(access));

    sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));



    EXPECT_TRUE(sm->addService("foo", getBinder(), false /*allowIsolated*/,

        IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());
@@ -261,7 +261,7 @@ TEST(ListServices, NoPermissions) { 
    EXPECT_CALL(*access, getCallingContext()).WillOnce(Return(Access::CallingContext{}));

    EXPECT_CALL(*access, canList(_)).WillOnce(Return(false));



    sp<ServiceManager> sm = new NiceMock<MockServiceManager>(std::move(access));

    sp<ServiceManager> sm = sp<NiceMock<MockServiceManager>>::make(std::move(access));



    std::vector<std::string> out;

    EXPECT_FALSE(sm->listServices(IServiceManager::DUMP_FLAG_PRIORITY_ALL, &out).isOk());
@@ -329,9 +329,9 @@ TEST(ServiceNotifications, NoPermissionsRegister) { 
    EXPECT_CALL(*access, getCallingContext()).WillOnce(Return(Access::CallingContext{}));

    EXPECT_CALL(*access, canFind(_,_)).WillOnce(Return(false));



    sp<ServiceManager> sm = new ServiceManager(std::move(access));

    sp<ServiceManager> sm = sp<ServiceManager>::make(std::move(access));



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    EXPECT_EQ(sm->registerForNotifications("foofoo", cb).exceptionCode(),

        Status::EX_SECURITY);
@@ -343,9 +343,9 @@ TEST(ServiceNotifications, NoPermissionsUnregister) { 
    EXPECT_CALL(*access, getCallingContext()).WillOnce(Return(Access::CallingContext{}));

    EXPECT_CALL(*access, canFind(_,_)).WillOnce(Return(false));



    sp<ServiceManager> sm = new ServiceManager(std::move(access));

    sp<ServiceManager> sm = sp<ServiceManager>::make(std::move(access));



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    // should always hit security error first

    EXPECT_EQ(sm->unregisterForNotifications("foofoo", cb).exceptionCode(),
@@ -355,7 +355,7 @@ TEST(ServiceNotifications, NoPermissionsUnregister) { 
TEST(ServiceNotifications, InvalidName) {

    auto sm = getPermissiveServiceManager();



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    EXPECT_EQ(sm->registerForNotifications("foo@foo", cb).exceptionCode(),

        Status::EX_ILLEGAL_ARGUMENT);
@@ -371,7 +371,7 @@ TEST(ServiceNotifications, NullCallback) { 
TEST(ServiceNotifications, Unregister) {

    auto sm = getPermissiveServiceManager();



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    EXPECT_TRUE(sm->registerForNotifications("foofoo", cb).isOk());

    EXPECT_EQ(sm->unregisterForNotifications("foofoo", cb).exceptionCode(), 0);
@@ -380,7 +380,7 @@ TEST(ServiceNotifications, Unregister) { 
TEST(ServiceNotifications, UnregisterWhenNoRegistrationExists) {

    auto sm = getPermissiveServiceManager();



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    EXPECT_EQ(sm->unregisterForNotifications("foofoo", cb).exceptionCode(),

        Status::EX_ILLEGAL_STATE);
@@ -389,7 +389,7 @@ TEST(ServiceNotifications, UnregisterWhenNoRegistrationExists) { 
TEST(ServiceNotifications, NoNotification) {

    auto sm = getPermissiveServiceManager();



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    EXPECT_TRUE(sm->registerForNotifications("foofoo", cb).isOk());

    EXPECT_TRUE(sm->addService("otherservice", getBinder(),
@@ -402,7 +402,7 @@ TEST(ServiceNotifications, NoNotification) { 
TEST(ServiceNotifications, GetNotification) {

    auto sm = getPermissiveServiceManager();



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    sp<IBinder> service = getBinder();
@@ -417,7 +417,7 @@ TEST(ServiceNotifications, GetNotification) { 
TEST(ServiceNotifications, GetNotificationForAlreadyRegisteredService) {

    auto sm = getPermissiveServiceManager();



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    sp<IBinder> service = getBinder();
@@ -433,7 +433,7 @@ TEST(ServiceNotifications, GetNotificationForAlreadyRegisteredService) { 
TEST(ServiceNotifications, GetMultipleNotification) {

    auto sm = getPermissiveServiceManager();



    sp<CallbackHistorian> cb = new CallbackHistorian;

    sp<CallbackHistorian> cb = sp<CallbackHistorian>::make();



    sp<IBinder> binder1 = getBinder();

    sp<IBinder> binder2 = getBinder();

libs/binder/include/binder/IInterface.h

+1 −1
Original line number Diff line number Diff line
@@ -186,7 +186,7 @@ template<typename INTERFACE> 
inline sp<IInterface> BnInterface<INTERFACE>::queryLocalInterface(

        const String16& _descriptor)

{

    if (_descriptor == INTERFACE::descriptor) return this;

    if (_descriptor == INTERFACE::descriptor) return sp<IInterface>::fromExisting(this);

    return nullptr;

}