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
diff --git a/cmds/servicemanager/ServiceManager.cpp b/cmds/servicemanager/ServiceManager.cpp
index 56c9d46..4472db1 100644
--- a/cmds/servicemanager/ServiceManager.cpp
+++ b/cmds/servicemanager/ServiceManager.cpp
@@ -372,8 +372,10 @@
}
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 @@
.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::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 @@
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);
}