Fix UAF on singleton service instances

This corrects how the ServiceManager clears out identity mappings
to singleton services, preventing it from indefinitely holding a
raw Instance* to any singleton it starts.

BUG=None

Review-Url: https://codereview.chromium.org/2566663003
Cr-Commit-Position: refs/heads/master@{#437737}

services/service_manager/service_manager.cc

@@ -146,8 +146,8 @@ class ServiceManager::Instance
 
     std::unique_ptr<ConnectParams> params(std::move(*connect_params));
     if (!params->connect_callback().is_null()) {
-        params->connect_callback().Run(mojom::ConnectResult::SUCCEEDED,
-                                       identity_.user_id());
+      params->connect_callback().Run(mojom::ConnectResult::SUCCEEDED,
+                                     identity_.user_id());
     }
 
     InterfaceProviderSpecMap specs;
@@ -641,9 +641,7 @@ void ServiceManager::OnInstanceError(Instance* instance) {
   if (instance == service_manager_instance_)
     return;
 
-  const Identity identity = instance->identity();
-  identity_to_instance_.erase(identity);
-
+  EraseInstanceIdentity(instance);
   if (instance->parent()) {
     // Deletes |instance|.
     instance->parent()->RemoveChild(instance);
@@ -660,7 +658,7 @@ void ServiceManager::OnInstanceUnreachable(Instance* instance) {
   // If an Instance becomes unreachable, new connection requests for this
   // identity will elicit a new Instance instantiation. The unreachable instance
   // remains alive.
-  identity_to_instance_.erase(instance->identity());
+  EraseInstanceIdentity(instance);
 }
 
 void ServiceManager::OnInstanceStopped(const Identity& identity) {
@@ -719,6 +717,28 @@ ServiceManager::Instance* ServiceManager::GetExistingInstance(
   return nullptr;
 }
 
+void ServiceManager::EraseInstanceIdentity(Instance* instance) {
+  const Identity& identity = instance->identity();
+
+  auto it = identity_to_instance_.find(identity);
+  if (it != identity_to_instance_.end()) {
+    identity_to_instance_.erase(it);
+    return;
+  }
+
+  if (singletons_.find(identity.name()) != singletons_.end()) {
+    singletons_.erase(identity.name());
+    for (auto it = identity_to_instance_.begin();
+         it != identity_to_instance_.end(); ++it) {
+      if (it->first.name() == identity.name() &&
+          it->first.instance() == identity.instance()) {
+        identity_to_instance_.erase(it);
+        return;
+      }
+    }
+  }
+}
+
 void ServiceManager::NotifyServiceStarted(const Identity& identity,
                                           base::ProcessId pid) {
   listeners_.ForAllPtrs(

services/service_manager/service_manager.h

@@ -119,6 +119,10 @@ class ServiceManager {
   // running as a different user if one is available that services all users.
   Instance* GetExistingInstance(const Identity& identity) const;
 
+  // Erases any identities mapping to |instance|. Following this call it is
+  // impossible for any call to GetExistingInstance() to return |instance|.
+  void EraseInstanceIdentity(Instance* instance);
+
   void NotifyServiceStarted(const Identity& identity, base::ProcessId pid);
   void NotifyServiceFailedToStart(const Identity& identity);