cameraserver: Avoiding deadlocks while calling isPublicallyHiddenSecureCamera().

The following scenario can occur:

 T1 serving Client A's disconnect() call:
   T2 : serving Client B's connect() call
     T2 : CameraProviderManager::openSession() locks mInterfaceMutex and waits on open() HAL
          interface call
       T1: updateStatus() locks mStatusListenerMutex
         T1: CameraProviderManager::isPublicallyHiddenSecureCamera() waits
             on mInterfaceMutex
           T2: while waiting on open(), gets a torchModeStatus() callback from the camera HAL
              T2: onStatusChanged()
                T2: broadcastTorchModeStatus() which waits on mStatusListenerMutex

As a result there's a possible circular hold and wait between T1 and T2.

We cache isPublicallyHiddenSecureCamera in CameraState. That doesn't completely
avoid having to hold mInterfaceLock while calling isPublicallyHiddenSecureCamera() in CameraService
(cache updates), instead it reduces it. We instead need to hold mCameraStates lock which has a
smaller scope.

Bug: 141756275

Test: CTS
Test: GCA (sanity)

Merged-In: I4a697c1eaccc3603007be4a595febea981fbeb64
Change-Id: Ie5508afb126a874f76fbbfc2dd19ef79ae6255e0
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>

services/camera/libcameraservice/CameraService.cpp

@@ -253,6 +253,15 @@ void CameraService::onNewProviderRegistered() {
     enumerateProviders();
 }
 
+bool CameraService::isPublicallyHiddenSecureCamera(const String8& cameraId) {
+    auto state = getCameraState(cameraId);
+    if (state != nullptr) {
+        return state->isPublicallyHiddenSecureCamera();
+    }
+    // Hidden physical camera ids won't have CameraState
+    return mCameraProviderManager->isPublicallyHiddenSecureCamera(cameraId.c_str());
+}
+
 void CameraService::updateCameraNumAndIds() {
     Mutex::Autolock l(mServiceLock);
     mNumberOfCameras = mCameraProviderManager->getCameraCount();
@@ -268,6 +277,8 @@ void CameraService::addStates(const String8 id) {
         ALOGE("Failed to query device resource cost: %s (%d)", strerror(-res), res);
         return;
     }
+    bool isPublicallyHiddenSecureCamera =
+            mCameraProviderManager->isPublicallyHiddenSecureCamera(id.string());
     std::set<String8> conflicting;
     for (size_t i = 0; i < cost.conflictingDevices.size(); i++) {
         conflicting.emplace(String8(cost.conflictingDevices[i].c_str()));
@@ -276,7 +287,8 @@ void CameraService::addStates(const String8 id) {
     {
         Mutex::Autolock lock(mCameraStatesLock);
         mCameraStates.emplace(id, std::make_shared<CameraState>(id, cost.resourceCost,
-                                                                conflicting));
+                                                                conflicting,
+                                                                isPublicallyHiddenSecureCamera));
     }
 
     if (mFlashlight->hasFlashUnit(id)) {
@@ -514,8 +526,16 @@ Status CameraService::getCameraCharacteristics(const String16& cameraId,
                 "Camera subsystem is not available");;
     }
 
-    Status ret{};
+    if (shouldRejectHiddenCameraConnection(String8(cameraId))) {
+        ALOGW("Attempting to retrieve characteristics for system-only camera id %s, rejected",
+              String8(cameraId).string());
+        return STATUS_ERROR_FMT(ERROR_DISCONNECTED,
+                                "No camera device with ID \"%s\" currently available",
+                                String8(cameraId).string());
+
+    }
 
+    Status ret{};
     status_t res = mCameraProviderManager->getCameraCharacteristics(
             String8(cameraId).string(), cameraInfo);
     if (res != OK) {
@@ -1330,7 +1350,7 @@ bool CameraService::shouldRejectHiddenCameraConnection(const String8 & cameraId)
     // publically hidden, we should reject the connection.
     if (!hardware::IPCThreadState::self()->isServingCall() &&
             CameraThreadState::getCallingPid() != getpid() &&
-            mCameraProviderManager->isPublicallyHiddenSecureCamera(cameraId.c_str())) {
+            isPublicallyHiddenSecureCamera(cameraId)) {
         return true;
     }
     return false;
@@ -1799,16 +1819,25 @@ Status CameraService::addListenerHelper(const sp<ICameraServiceListener>& listen
     {
         Mutex::Autolock lock(mCameraStatesLock);
         for (auto& i : mCameraStates) {
-            if (!isVendorListener &&
-                mCameraProviderManager->isPublicallyHiddenSecureCamera(i.first.c_str())) {
-                ALOGV("Cannot add public listener for hidden system-only %s for pid %d",
-                      i.first.c_str(), CameraThreadState::getCallingPid());
-                continue;
-            }
             cameraStatuses->emplace_back(i.first, mapToInterface(i.second->getStatus()));
         }
     }
 
+    // Remove the camera statuses that should be hidden from the client, we do
+    // this after collecting the states in order to avoid holding
+    // mCameraStatesLock and mInterfaceLock (held in
+    // isPublicallyHiddenSecureCamera()) at the same time.
+    cameraStatuses->erase(std::remove_if(cameraStatuses->begin(), cameraStatuses->end(),
+                [this, &isVendorListener](const hardware::CameraStatus& s) {
+                    bool ret = !isVendorListener && isPublicallyHiddenSecureCamera(s.cameraId);
+                    if (ret) {
+                        ALOGV("Cannot add public listener for hidden system-only %s for pid %d",
+                                s.cameraId.c_str(), CameraThreadState::getCallingPid());
+                    }
+                    return ret;
+                }),
+                cameraStatuses->end());
+
     /*
      * Immediately signal current torch status to this listener only
      * This may be a subset of all the devices, so don't include it in the response directly
@@ -2870,8 +2899,9 @@ void CameraService::SensorPrivacyPolicy::binderDied(const wp<IBinder>& /*who*/)
 // ----------------------------------------------------------------------------
 
 CameraService::CameraState::CameraState(const String8& id, int cost,
-        const std::set<String8>& conflicting) : mId(id),
-        mStatus(StatusInternal::NOT_PRESENT), mCost(cost), mConflicting(conflicting) {}
+        const std::set<String8>& conflicting, bool isHidden) : mId(id),
+        mStatus(StatusInternal::NOT_PRESENT), mCost(cost), mConflicting(conflicting),
+        mIsPublicallyHiddenSecureCamera(isHidden) {}
 
 CameraService::CameraState::~CameraState() {}
 
@@ -2900,6 +2930,10 @@ String8 CameraService::CameraState::getId() const {
     return mId;
 }
 
+bool CameraService::CameraState::isPublicallyHiddenSecureCamera() const {
+    return mIsPublicallyHiddenSecureCamera;
+}
+
 // ----------------------------------------------------------------------------
 //                  ClientEventListener
 // ----------------------------------------------------------------------------
@@ -3235,10 +3269,10 @@ void CameraService::updateStatus(StatusInternal status, const String8& cameraId,
                 cameraId.string());
         return;
     }
-
+    bool isHidden = isPublicallyHiddenSecureCamera(cameraId);
     // Update the status for this camera state, then send the onStatusChangedCallbacks to each
     // of the listeners with both the mStatusStatus and mStatusListenerLock held
-    state->updateStatus(status, cameraId, rejectSourceStates, [this]
+    state->updateStatus(status, cameraId, rejectSourceStates, [this,&isHidden]
             (const String8& cameraId, StatusInternal status) {
 
             if (status != StatusInternal::ENUMERATING) {
@@ -3260,8 +3294,7 @@ void CameraService::updateStatus(StatusInternal status, const String8& cameraId,
             Mutex::Autolock lock(mStatusListenerLock);
 
             for (auto& listener : mListenerList) {
-                if (!listener.first &&
-                    mCameraProviderManager->isPublicallyHiddenSecureCamera(cameraId.c_str())) {
+                if (!listener.first &&  isHidden) {
                     ALOGV("Skipping camera discovery callback for system-only camera %s",
                           cameraId.c_str());
                     continue;

services/camera/libcameraservice/CameraService.h

@@ -471,7 +471,8 @@ private:
          * Make a new CameraState and set the ID, cost, and conflicting devices using the values
          * returned in the HAL's camera_info struct for each device.
          */
-        CameraState(const String8& id, int cost, const std::set<String8>& conflicting);
+        CameraState(const String8& id, int cost, const std::set<String8>& conflicting,
+                bool isHidden);
         virtual ~CameraState();
 
         /**
@@ -523,6 +524,11 @@ private:
          */
         String8 getId() const;
 
+        /**
+         * Return if the camera device is a publically hidden secure camera
+         */
+        bool isPublicallyHiddenSecureCamera() const;
+
     private:
         const String8 mId;
         StatusInternal mStatus; // protected by mStatusLock
@@ -530,6 +536,7 @@ private:
         std::set<String8> mConflicting;
         mutable Mutex mStatusLock;
         CameraParameters mShimParams;
+        const bool mIsPublicallyHiddenSecureCamera;
     }; // class CameraState
 
     // Observer for UID lifecycle enforcing that UIDs in idle
@@ -635,7 +642,9 @@ private:
 
     // Should an operation attempt on a cameraId be rejected, if the camera id is
     // advertised as a publically hidden secure camera, by the camera HAL ?
-    bool shouldRejectHiddenCameraConnection(const String8 & cameraId);
+    bool shouldRejectHiddenCameraConnection(const String8& cameraId);
+
+    bool isPublicallyHiddenSecureCamera(const String8& cameraId);
 
     // Single implementation shared between the various connect calls
     template<class CALLBACK, class CLIENT>

services/camera/libcameraservice/common/CameraProviderManager.cpp

@@ -1052,19 +1052,35 @@ bool CameraProviderManager::isPublicallyHiddenSecureCamera(const std::string& id
 
 bool CameraProviderManager::isPublicallyHiddenSecureCameraLocked(const std::string& id) const {
     auto deviceInfo = findDeviceInfoLocked(id);
-    if (deviceInfo == nullptr) {
-        return false;
+    if (deviceInfo != nullptr) {
+        return deviceInfo->mIsPublicallyHiddenSecureCamera;
+    }
+    // If this is a hidden physical camera, we should return what kind of
+    // camera the enclosing logical camera is.
+    auto isHiddenAndParent = isHiddenPhysicalCameraInternal(id);
+    if (isHiddenAndParent.first) {
+        LOG_ALWAYS_FATAL_IF(id == isHiddenAndParent.second->mId,
+                "%s: hidden physical camera id %s and enclosing logical camera id %s are the same",
+                __FUNCTION__, id.c_str(), isHiddenAndParent.second->mId.c_str());
+        return isPublicallyHiddenSecureCameraLocked(isHiddenAndParent.second->mId);
     }
-    return deviceInfo->mIsPublicallyHiddenSecureCamera;
+    // Invalid camera id
+    return true;
 }
 
-bool CameraProviderManager::isHiddenPhysicalCamera(const std::string& cameraId) {
+bool CameraProviderManager::isHiddenPhysicalCamera(const std::string& cameraId) const {
+    return isHiddenPhysicalCameraInternal(cameraId).first;
+}
+
+std::pair<bool, CameraProviderManager::ProviderInfo::DeviceInfo *>
+CameraProviderManager::isHiddenPhysicalCameraInternal(const std::string& cameraId) const {
+    auto falseRet = std::make_pair(false, nullptr);
     for (auto& provider : mProviders) {
         for (auto& deviceInfo : provider->mDevices) {
             if (deviceInfo->mId == cameraId) {
                 // cameraId is found in public camera IDs advertised by the
                 // provider.
-                return false;
+                return falseRet;
             }
         }
     }
@@ -1076,7 +1092,7 @@ bool CameraProviderManager::isHiddenPhysicalCamera(const std::string& cameraId)
             if (res != OK) {
                 ALOGE("%s: Failed to getCameraCharacteristics for id %s", __FUNCTION__,
                         deviceInfo->mId.c_str());
-                return false;
+                return falseRet;
             }
 
             std::vector<std::string> physicalIds;
@@ -1088,16 +1104,16 @@ bool CameraProviderManager::isHiddenPhysicalCamera(const std::string& cameraId)
                     if (deviceVersion < CAMERA_DEVICE_API_VERSION_3_5) {
                         ALOGE("%s: Wrong deviceVersion %x for hiddenPhysicalCameraId %s",
                                 __FUNCTION__, deviceVersion, cameraId.c_str());
-                        return false;
+                        return falseRet;
                     } else {
-                        return true;
+                        return std::make_pair(true, deviceInfo.get());
                     }
                 }
             }
         }
     }
 
-    return false;
+    return falseRet;
 }
 
 status_t CameraProviderManager::addProviderLocked(const std::string& newProvider, bool expected) {

services/camera/libcameraservice/common/CameraProviderManager.h

@@ -270,7 +270,7 @@ public:
     bool isLogicalCamera(const std::string& id, std::vector<std::string>* physicalCameraIds);
 
     bool isPublicallyHiddenSecureCamera(const std::string& id) const;
-    bool isHiddenPhysicalCamera(const std::string& cameraId);
+    bool isHiddenPhysicalCamera(const std::string& cameraId) const;
 
     static const float kDepthARTolerance;
 private:
@@ -595,6 +595,11 @@ private:
     bool isPublicallyHiddenSecureCameraLocked(const std::string& id) const;
 
     void filterLogicalCameraIdsLocked(std::vector<std::string>& deviceIds) const;
+
+    bool isPublicallyHiddenSecureCameraLocked(const std::string& id);
+
+    std::pair<bool, CameraProviderManager::ProviderInfo::DeviceInfo *>
+            isHiddenPhysicalCameraInternal(const std::string& cameraId) const;
 };
 
 } // namespace android