From fd39db81e44e191baa476a579de6cc2618de025a Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Mon, 7 Oct 2019 15:26:41 -0700 Subject: [PATCH] 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 --- .../camera/libcameraservice/CameraService.cpp | 63 ++++++++++++++----- .../camera/libcameraservice/CameraService.h | 13 +++- .../common/CameraProviderManager.cpp | 34 +++++++--- .../common/CameraProviderManager.h | 7 ++- 4 files changed, 90 insertions(+), 27 deletions(-) diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp index e663485cd9..bdeb27376f 100644 --- a/services/camera/libcameraservice/CameraService.cpp +++ b/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 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(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& 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& /*who*/) // ---------------------------------------------------------------------------- CameraService::CameraState::CameraState(const String8& id, int cost, - const std::set& conflicting) : mId(id), - mStatus(StatusInternal::NOT_PRESENT), mCost(cost), mConflicting(conflicting) {} + const std::set& 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; diff --git a/services/camera/libcameraservice/CameraService.h b/services/camera/libcameraservice/CameraService.h index cf93a4147c..8bb78cde56 100644 --- a/services/camera/libcameraservice/CameraService.h +++ b/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& conflicting); + CameraState(const String8& id, int cost, const std::set& 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 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 diff --git a/services/camera/libcameraservice/common/CameraProviderManager.cpp b/services/camera/libcameraservice/common/CameraProviderManager.cpp index 47baeb2668..7f94e5525f 100644 --- a/services/camera/libcameraservice/common/CameraProviderManager.cpp +++ b/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 +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 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) { diff --git a/services/camera/libcameraservice/common/CameraProviderManager.h b/services/camera/libcameraservice/common/CameraProviderManager.h index c812f424b4..cd283b3e37 100644 --- a/services/camera/libcameraservice/common/CameraProviderManager.h +++ b/services/camera/libcameraservice/common/CameraProviderManager.h @@ -270,7 +270,7 @@ public: bool isLogicalCamera(const std::string& id, std::vector* 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& deviceIds) const; + + bool isPublicallyHiddenSecureCameraLocked(const std::string& id); + + std::pair + isHiddenPhysicalCameraInternal(const std::string& cameraId) const; }; } // namespace android