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

Commit 7957867c authored by Steven Moreland's avatar Steven Moreland
Browse files

sm: lazy service - fix race 

There is a possible race:
- service registers binder A
- service registers client callback (cc1) for A
- sm send cc1 "A hasClients"
- service registers binder A (again - bad behavior!)
  - side effect: "hasClients" implicitly set to false
- service registers client callback (cc2) for A
- sm sends cc1 and cc2 "A hasClients"

Due to an intentionally overly careful check in client
callbacks, they crash when this double-send of
'hasClients' is hit.

This CL retains the state of cc1 in order to fix the
issue. Comments are added with various details about
the implementation, and b/279948722 is filed to
resolve these comments.

Bug: 279898063
Test: aidl_lazy_test
Change-Id: Ida443d5b02f19736baabdc57ff283995cdcc2a87
parent 9843b07b

cmds/servicemanager/ServiceManager.cpp

+17 −1
Original line number Diff line number Diff line
@@ -372,8 +372,10 @@ Status ServiceManager::addService(const std::string& name, const sp<IBinder>& bi 
    }



    auto it = mNameToService.find(name);

    bool prevClients = false;

    if (it != mNameToService.end()) {

        const Service& existing = it->second;

        prevClients = existing.hasClients;



        // We could do better than this because if the other service dies, it

        // may not have an entry here. However, this case is unlikely. We are
@@ -401,10 +403,13 @@ Status ServiceManager::addService(const std::string& name, const sp<IBinder>& bi 
            .binder = binder,

            .allowIsolated = allowIsolated,

            .dumpPriority = dumpPriority,

            .hasClients = prevClients, // see b/279898063, matters if existing callbacks

            .guaranteeClient = false,  // handled below

            .ctx = ctx,

    };



    if (auto it = mNameToRegistrationCallback.find(name); it != mNameToRegistrationCallback.end()) {

        // TODO: this is only needed once

        // See also getService - handles case where client never gets the service,

        // we want the service to quit.

        mNameToService[name].guaranteeClient = true;
@@ -623,6 +628,14 @@ void ServiceManager::removeRegistrationCallback(const wp<IBinder>& who, 
void ServiceManager::binderDied(const wp<IBinder>& who) {

    for (auto it = mNameToService.begin(); it != mNameToService.end();) {

        if (who == it->second.binder) {

            // TODO: currently, this entry contains the state also

            // associated with mNameToClientCallback. If we allowed

            // other processes to register client callbacks, we

            // would have to preserve hasClients (perhaps moving

            // that state into mNameToClientCallback, which is complicated

            // because those callbacks are associated w/ particular binder

            // objects, though they are indexed by name now, they may

            // need to be indexed by binder at that point).

            it = mNameToService.erase(it);

        } else {

            ++it;
@@ -690,7 +703,10 @@ Status ServiceManager::registerClientCallback(const std::string& name, const sp< 
        return Status::fromExceptionCode(Status::EX_ILLEGAL_STATE, "Couldn't linkToDeath.");

    }



    // make sure all callbacks have been told about a consistent state - b/278038751

    // WARNING: binderDied makes an assumption about this. If we open up client

    // callbacks to other services, certain race conditions may lead to services

    // getting extra client callback notifications.

    // Make sure all callbacks have been told about a consistent state - b/278038751

    if (serviceIt->second.hasClients) {

        cb->onClients(service, true);

    }