HidlLazyUtils: avoid race w/ register & onClients am: 8578d50d84android-12.1.0_r9android-12.1.0_r8android-12.1.0_r7android-12.1.0_r22android-12.1.0_r21android-12.1.0_r20android-12.1.0_r19android-12.1.0_r11android-12.1.0_r10android12L-devandroid12-qpr3-s7-releaseandroid12-qpr3-s6-releaseandroid12-qpr3-s5-releaseandroid12-qpr3-s4-releaseandroid12-qpr3-s3-releaseandroid12-qpr3-s2-releaseandroid12-qpr3-s1-releaseandroid12-qpr3-release

Original change: https://googleplex-android-review.googlesource.com/c/platform/system/libhidl/+/15132861

Change-Id: Id2976726707def0cc1fe19a1a71bd8a7d7fe5099

2 files changed, 35 insertions, 24 deletions

diff --git a/transport/HidlLazyUtils.cpp b/transport/HidlLazyUtils.cpp
index be7470f..c5f8c74 100644
--- a/transport/HidlLazyUtils.cpp
+++ b/transport/HidlLazyUtils.cpp
@@ -36,9 +36,9 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall
 
     bool addRegisteredService(const sp<IBase>& service, const std::string& name);
 
-    bool tryUnregister();
+    bool tryUnregisterLocked();
 
-    void reRegister();
+    void reRegisterLocked();
 
     void setActiveServicesCallback(const std::function<bool(bool)>& activeServicesCallback);
 
@@ -58,18 +58,23 @@ class ClientCounterCallback : public ::android::hidl::manager::V1_2::IClientCall
      * Looks up service that is guaranteed to be registered (service from
      * onClients).
      */
-    Service& assertRegisteredService(const sp<IBase>& service);
+    Service& assertRegisteredServiceLocked(const sp<IBase>& service);
 
     /**
      * Registers or re-registers services. Returns whether successful.
      */
-    bool registerService(const sp<IBase>& service, const std::string& name);
+    bool registerServiceLocked(const sp<IBase>& service, const std::string& name);
 
     /**
      * Unregisters all services that we can. If we can't unregister all, re-register other
      * services.
      */
-    void tryShutdown();
+    void tryShutdownLocked();
+
+    /**
+     * For below.
+     */
+    std::mutex mMutex;
 
     /**
      * Number of services that have been registered.
@@ -103,7 +108,8 @@ class LazyServiceRegistrarImpl {
 
 bool ClientCounterCallback::addRegisteredService(const sp<IBase>& service,
                                                  const std::string& name) {
-    bool success = registerService(service, name);
+    std::lock_guard<std::mutex> lock(mMutex);
+    bool success = registerServiceLocked(service, name);
 
     if (success) {
         mRegisteredServices.push_back({service, name});
@@ -112,7 +118,7 @@ bool ClientCounterCallback::addRegisteredService(const sp<IBase>& service,
     return success;
 }
 
-ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredService(
+ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredServiceLocked(
         const sp<IBase>& service) {
     for (Service& registered : mRegisteredServices) {
         if (registered.service != service) continue;
@@ -123,7 +129,8 @@ ClientCounterCallback::Service& ClientCounterCallback::assertRegisteredService(
     __builtin_unreachable();
 }
 
-bool ClientCounterCallback::registerService(const sp<IBase>& service, const std::string& name) {
+bool ClientCounterCallback::registerServiceLocked(const sp<IBase>& service,
+                                                  const std::string& name) {
     auto manager = hardware::defaultServiceManager1_2();
 
     const std::string descriptor = getDescriptor(service.get());
@@ -145,13 +152,10 @@ bool ClientCounterCallback::registerService(const sp<IBase>& service, const std:
     return true;
 }
 
-/**
- * onClients is oneway, so no need to worry about multi-threading. Note that this means multiple
- * invocations could occur on different threads however.
- */
 Return<void> ClientCounterCallback::onClients(const sp<::android::hidl::base::V1_0::IBase>& service,
                                               bool clients) {
-    Service& registered = assertRegisteredService(service);
+    std::lock_guard<std::mutex> lock(mMutex);
+    Service& registered = assertRegisteredServiceLocked(service);
     if (registered.clients == clients) {
         LOG(FATAL) << "Process already thought " << getDescriptor(service.get()) << "/"
                    << registered.name << " had clients: " << registered.clients
@@ -181,13 +185,13 @@ Return<void> ClientCounterCallback::onClients(const sp<::android::hidl::base::V1
     // client count change event, try to shutdown the process if its services
     // have no clients.
     if (!handledInCallback && numWithClients == 0) {
-        tryShutdown();
+        tryShutdownLocked();
     }
 
     return Status::ok();
 }
 
-bool ClientCounterCallback::tryUnregister() {
+bool ClientCounterCallback::tryUnregisterLocked() {
     auto manager = hardware::defaultServiceManager1_2();
 
     for (Service& entry : mRegisteredServices) {
@@ -206,14 +210,14 @@ bool ClientCounterCallback::tryUnregister() {
     return true;
 }
 
-void ClientCounterCallback::reRegister() {
+void ClientCounterCallback::reRegisterLocked() {
     for (Service& entry : mRegisteredServices) {
         // re-register entry if not already registered
         if (entry.registered) {
             continue;
         }
 
-        if (!registerService(entry.service, entry.name)) {
+        if (!registerServiceLocked(entry.service, entry.name)) {
             // Must restart. Otherwise, clients will never be able to get ahold of this service.
             LOG(FATAL) << "Bad state: could not re-register " << getDescriptor(entry.service.get());
         }
@@ -222,22 +226,24 @@ void ClientCounterCallback::reRegister() {
     }
 }
 
-void ClientCounterCallback::tryShutdown() {
+void ClientCounterCallback::tryShutdownLocked() {
     LOG(INFO) << "Trying to exit HAL. No clients in use for any service in process.";
 
-    if (tryUnregister()) {
+    if (tryUnregisterLocked()) {
         LOG(INFO) << "Unregistered all clients and exiting";
         exit(EXIT_SUCCESS);
     }
 
     // At this point, we failed to unregister some of the services, leaving the
     // server in an inconsistent state. Re-register all services that were
-    // unregistered by tryUnregister().
-    reRegister();
+    // unregistered by tryUnregisterLocked().
+    reRegisterLocked();
 }
 
 void ClientCounterCallback::setActiveServicesCallback(
         const std::function<bool(bool)>& activeServicesCallback) {
+    std::lock_guard<std::mutex> lock(mMutex);
+
     mActiveServicesCallback = activeServicesCallback;
 }
 
@@ -251,11 +257,15 @@ status_t LazyServiceRegistrarImpl::registerService(
 }
 
 bool LazyServiceRegistrarImpl::tryUnregister() {
-    return mClientCallback->tryUnregister();
+    // see comments in header, this should only be called from the active
+    // services callback, see also b/191781736
+    return mClientCallback->tryUnregisterLocked();
 }
 
 void LazyServiceRegistrarImpl::reRegister() {
-    mClientCallback->reRegister();
+    // see comments in header, this should only be called from the active
+    // services callback, see also b/191781736
+    mClientCallback->reRegisterLocked();
 }
 
 void LazyServiceRegistrarImpl::setActiveServicesCallback(
diff --git a/transport/include/hidl/HidlLazyUtils.h b/transport/include/hidl/HidlLazyUtils.h
index 427611b..376b6ab 100644
--- a/transport/include/hidl/HidlLazyUtils.h
+++ b/transport/include/hidl/HidlLazyUtils.h
@@ -65,7 +65,8 @@ class LazyServiceRegistrar {
 
      /**
       * Try to unregister all services previously registered with 'registerService'.
-      * Returns 'true' if successful.
+      * Returns 'true' if successful. This should only be called within
+      * the callback registered by setActiveServicesCallback.
       */
      bool tryUnregister();