gugelfrei
/
android_frameworks_av
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

cameraserver: Avoiding deadlocks while calling getSystemCameraKind()

Browse Source 
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() / getSystemCameraKind() 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 the system camera kind in CameraState. That doesn't completely
avoid having to hold mInterfaceLock while calling getSystemCameraKind 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)

Change-Id: I4a697c1eaccc3603007be4a595febea981fbeb64
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>
gugelfrei
Jayant Chowdhary 5 years ago
parent 4bebef8d59
commit 33e8ef86ec
4 changed files with 147 additions and 46 deletions
Show Stats Download Patch File Download Diff File

96
services/camera/libcameraservice/CameraService.cpp
Unescape View File

@ -268,8 +268,12 @@ void CameraService::filterAPI1SystemCameraLocked(
const std::vector<std::string> &normalDeviceIds) {
mNormalDeviceIdsWithoutSystemCamera.clear();
for (auto &deviceId : normalDeviceIds) {
if (getSystemCameraKind(String8(deviceId.c_str())) ==
SystemCameraKind::SYSTEM_ONLY_CAMERA) {
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKind(String8(deviceId.c_str()), &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping", __FUNCTION__, deviceId.c_str());
continue;
}
if (deviceKind == SystemCameraKind::SYSTEM_ONLY_CAMERA) {
// All system camera ids will necessarily come after public camera
// device ids as per the HAL interface contract.
break;
@ -280,6 +284,16 @@ void CameraService::filterAPI1SystemCameraLocked(
mNormalDeviceIdsWithoutSystemCamera.size());
}
status_t CameraService::getSystemCameraKind(const String8& cameraId, SystemCameraKind *kind) const {
auto state = getCameraState(cameraId);
if (state != nullptr) {
*kind = state->getSystemCameraKind();
return OK;
}
// Hidden physical camera ids won't have CameraState
return mCameraProviderManager->getSystemCameraKind(cameraId.c_str(), kind);
}
void CameraService::updateCameraNumAndIds() {
Mutex::Autolock l(mServiceLock);
std::pair<int, int> systemAndNonSystemCameras = mCameraProviderManager->getCameraCount();
@ -296,10 +310,16 @@ void CameraService::addStates(const String8 id) {
std::string cameraId(id.c_str());
hardware::camera::common::V1_0::CameraResourceCost cost;
status_t res = mCameraProviderManager->getResourceCost(cameraId, &cost);
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (res != OK) {
ALOGE("Failed to query device resource cost: %s (%d)", strerror(-res), res);
return;
}
res = mCameraProviderManager->getSystemCameraKind(cameraId, &deviceKind);
if (res != OK) {
ALOGE("Failed to query device kind: %s (%d)", strerror(-res), res);
return;
}
std::set<String8> conflicting;
for (size_t i = 0; i < cost.conflictingDevices.size(); i++) {
conflicting.emplace(String8(cost.conflictingDevices[i].c_str()));
@ -308,7 +328,7 @@ void CameraService::addStates(const String8 id) {
{
Mutex::Autolock lock(mCameraStatesLock);
mCameraStates.emplace(id, std::make_shared<CameraState>(id, cost.resourceCost,
conflicting));
conflicting, deviceKind));
}
if (mFlashlight->hasFlashUnit(id)) {
@ -594,7 +614,12 @@ Status CameraService::getCameraCharacteristics(const String16& cameraId,
"characteristics for device %s: %s (%d)", String8(cameraId).string(),
strerror(-res), res);
}
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKind(String8(cameraId), &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping", __FUNCTION__, String8(cameraId).string());
return STATUS_ERROR_FMT(ERROR_INVALID_OPERATION, "Unable to retrieve camera kind "
"for device %s", String8(cameraId).string());
}
int callingPid = CameraThreadState::getCallingPid();
int callingUid = CameraThreadState::getCallingUid();
std::vector<int32_t> tagsRemoved;
@ -602,7 +627,7 @@ Status CameraService::getCameraCharacteristics(const String16& cameraId,
// android.permission.CAMERA is required. If android.permission.SYSTEM_CAMERA was needed,
// it would've already been checked in shouldRejectSystemCameraConnection.
if ((callingPid != getpid()) &&
(getSystemCameraKind(String8(cameraId)) != SystemCameraKind::SYSTEM_ONLY_CAMERA) &&
(deviceKind != SystemCameraKind::SYSTEM_ONLY_CAMERA) &&
!checkPermission(sCameraPermission, callingPid, callingUid)) {
res = cameraInfo->removePermissionEntries(
mCameraProviderManager->getProviderTagIdLocked(String8(cameraId).string()),
@ -1049,11 +1074,19 @@ Status CameraService::validateClientPermissionsLocked(const String8& cameraId,
return STATUS_ERROR_FMT(ERROR_DISCONNECTED, "No camera device with ID \"%s\" is"
"available", cameraId.string());
}
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKind(cameraId, &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping", __FUNCTION__, cameraId.string());
return STATUS_ERROR_FMT(ERROR_ILLEGAL_ARGUMENT, "No camera device with ID \"%s\""
"found while trying to query device kind", cameraId.string());
}
// If it's not calling from cameraserver, check the permission if the
// device isn't a system only camera (shouldRejectSystemCameraConnection already checks for
// android.permission.SYSTEM_CAMERA for system only camera devices).
if (callingPid != getpid() &&
(getSystemCameraKind(cameraId) != SystemCameraKind::SYSTEM_ONLY_CAMERA) &&
(deviceKind != SystemCameraKind::SYSTEM_ONLY_CAMERA) &&
!checkPermission(sCameraPermission, clientPid, clientUid)) {
ALOGE("Permission Denial: can't use the camera pid=%d, uid=%d", clientPid, clientUid);
return STATUS_ERROR_FMT(ERROR_PERMISSION_DENIED,
@ -1407,9 +1440,8 @@ Status CameraService::connectLegacy(
return ret;
}
bool CameraService::shouldSkipStatusUpdates(const String8& cameraId, bool isVendorListener,
int clientPid, int clientUid) const {
SystemCameraKind systemCameraKind = getSystemCameraKind(cameraId);
bool CameraService::shouldSkipStatusUpdates(SystemCameraKind systemCameraKind,
bool isVendorListener, int clientPid, int clientUid) {
// If the client is not a vendor client, don't add listener if
// a) the camera is a publicly hidden secure camera OR
// b) the camera is a system only camera and the client doesn't
@ -1435,7 +1467,11 @@ bool CameraService::shouldRejectSystemCameraConnection(const String8& cameraId)
int cPid = CameraThreadState::getCallingPid();
int cUid = CameraThreadState::getCallingUid();
SystemCameraKind systemCameraKind = getSystemCameraKind(cameraId);
SystemCameraKind systemCameraKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKind(cameraId, &systemCameraKind) != OK) {
ALOGE("%s: Invalid camera id %s, ", __FUNCTION__, cameraId.c_str());
return true;
}
// (1) Cameraserver trying to connect, accept.
if (CameraThreadState::getCallingPid() == getpid()) {
@ -1926,14 +1962,24 @@ Status CameraService::addListenerHelper(const sp<ICameraServiceListener>& listen
{
Mutex::Autolock lock(mCameraStatesLock);
for (auto& i : mCameraStates) {
if (shouldSkipStatusUpdates(i.first, isVendorListener, clientPid, clientUid)) {
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 getSystemCameraKind()) at
// the same time.
cameraStatuses->erase(std::remove_if(cameraStatuses->begin(), cameraStatuses->end(),
[this, &isVendorListener, &clientPid, &clientUid](const hardware::CameraStatus& s) {
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKind(s.cameraId, &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping status update",
__FUNCTION__, s.cameraId.c_str());
return true;
}
return shouldSkipStatusUpdates(deviceKind, isVendorListener, clientPid,
clientUid);}), cameraStatuses->end());
/*
* Immediately signal current torch status to this listener only
@ -3023,8 +3069,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, SystemCameraKind systemCameraKind) : mId(id),
mStatus(StatusInternal::NOT_PRESENT), mCost(cost), mConflicting(conflicting),
mSystemCameraKind(systemCameraKind) {}
CameraService::CameraState::~CameraState() {}
@ -3053,6 +3100,10 @@ String8 CameraService::CameraState::getId() const {
return mId;
}
SystemCameraKind CameraService::CameraState::getSystemCameraKind() const {
return mSystemCameraKind;
}
// ----------------------------------------------------------------------------
// ClientEventListener
// ----------------------------------------------------------------------------
@ -3391,9 +3442,16 @@ void CameraService::updateStatus(StatusInternal status, const String8& cameraId,
return;
}
// Avoid calling getSystemCameraKind() with mStatusListenerLock held (b/141756275)
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKind(cameraId, &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping", __FUNCTION__, cameraId.string());
return;
}
// 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, &deviceKind]
(const String8& cameraId, StatusInternal status) {
if (status != StatusInternal::ENUMERATING) {
@ -3415,7 +3473,7 @@ void CameraService::updateStatus(StatusInternal status, const String8& cameraId,
Mutex::Autolock lock(mStatusListenerLock);
for (auto& listener : mListenerList) {
if (shouldSkipStatusUpdates(cameraId, listener->isVendorListener(),
if (shouldSkipStatusUpdates(deviceKind, listener->isVendorListener(),
listener->getListenerPid(), listener->getListenerUid())) {
ALOGV("Skipping camera discovery callback for system-only camera %s",
cameraId.c_str());

23
services/camera/libcameraservice/CameraService.h
Unescape View File

@ -490,7 +490,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,
SystemCameraKind deviceKind);
virtual ~CameraState();
/**
@ -542,6 +543,11 @@ private:
*/
String8 getId() const;
/**
* Return the kind (SystemCameraKind) of this camera device.
*/
SystemCameraKind getSystemCameraKind() const;
private:
const String8 mId;
StatusInternal mStatus; // protected by mStatusLock
@ -549,6 +555,7 @@ private:
std::set<String8> mConflicting;
mutable Mutex mStatusLock;
CameraParameters mShimParams;
const SystemCameraKind mSystemCameraKind;
}; // class CameraState
// Observer for UID lifecycle enforcing that UIDs in idle
@ -660,12 +667,14 @@ private:
// Should a device status update be skipped for a particular camera device ? (this can happen
// under various conditions. For example if a camera device is advertised as
// system only or hidden secure camera, amongst possible others.
bool shouldSkipStatusUpdates(const String8& cameraId, bool isVendorListener, int clientPid,
int clientUid) const;
inline SystemCameraKind getSystemCameraKind(const String8& cameraId) const {
return mCameraProviderManager->getSystemCameraKind(cameraId.c_str());
}
static bool shouldSkipStatusUpdates(SystemCameraKind systemCameraKind, bool isVendorListener,
int clientPid, int clientUid);
// Gets the kind of camera device (i.e public, hidden secure or system only)
// getSystemCameraKind() needs mInterfaceMutex which might lead to deadlocks
// if held along with mStatusListenerLock (depending on lock ordering, b/141756275), it is
// recommended that we don't call this function with mStatusListenerLock held.
status_t getSystemCameraKind(const String8& cameraId, SystemCameraKind *kind) const;
// Update the set of API1Compatible camera devices without including system
// cameras and secure cameras. This is used for hiding system only cameras

67
services/camera/libcameraservice/common/CameraProviderManager.cpp
Unescape View File

@ -110,7 +110,12 @@ std::pair<int, int> CameraProviderManager::getCameraCount() const {
int publicCameraCount = 0;
for (auto& provider : mProviders) {
for (auto &id : provider->mUniqueCameraIds) {
switch(getSystemCameraKindLocked(id)) {
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKindLocked(id, &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping", __FUNCTION__, id.c_str());
continue;
}
switch(deviceKind) {
case SystemCameraKind::PUBLIC:
publicCameraCount++;
break;
@ -140,7 +145,12 @@ void CameraProviderManager::collectDeviceIdsLocked(const std::vector<std::string
std::vector<std::string>& publicDeviceIds,
std::vector<std::string>& systemDeviceIds) const {
for (auto &deviceId : deviceIds) {
if (getSystemCameraKindLocked(deviceId) == SystemCameraKind::SYSTEM_ONLY_CAMERA) {
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKindLocked(deviceId, &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping", __FUNCTION__, deviceId.c_str());
continue;
}
if (deviceKind == SystemCameraKind::SYSTEM_ONLY_CAMERA) {
systemDeviceIds.push_back(deviceId);
} else {
publicDeviceIds.push_back(deviceId);
@ -158,9 +168,13 @@ std::vector<std::string> CameraProviderManager::getAPI1CompatibleCameraDeviceIds
// Secure cameras should not be exposed through camera 1 api
providerDeviceIds.erase(std::remove_if(providerDeviceIds.begin(), providerDeviceIds.end(),
[this](const std::string& s) {
bool rem = this->getSystemCameraKindLocked(s) ==
SystemCameraKind::HIDDEN_SECURE_CAMERA;
return rem;}), providerDeviceIds.end());
SystemCameraKind deviceKind = SystemCameraKind::PUBLIC;
if (getSystemCameraKindLocked(s, &deviceKind) != OK) {
ALOGE("%s: Invalid camera id %s, skipping", __FUNCTION__, s.c_str());
return true;
}
return deviceKind == SystemCameraKind::HIDDEN_SECURE_CAMERA;}),
providerDeviceIds.end());
// API1 app doesn't handle logical and physical camera devices well. So
// for each camera facing, only take the first id advertised by HAL in
// all [logical, physical1, physical2, ...] id combos, and filter out the rest.
@ -1089,26 +1103,45 @@ bool CameraProviderManager::isLogicalCamera(const std::string& id,
return deviceInfo->mIsLogicalCamera;
}
SystemCameraKind CameraProviderManager::getSystemCameraKind(const std::string& id) const {
status_t CameraProviderManager::getSystemCameraKind(const std::string& id,
SystemCameraKind *kind) const {
std::lock_guard<std::mutex> lock(mInterfaceMutex);
return getSystemCameraKindLocked(id);
return getSystemCameraKindLocked(id, kind);
}
SystemCameraKind CameraProviderManager::getSystemCameraKindLocked(const std::string& id) const {
status_t CameraProviderManager::getSystemCameraKindLocked(const std::string& id,
SystemCameraKind *kind) const {
auto deviceInfo = findDeviceInfoLocked(id);
if (deviceInfo == nullptr) {
return SystemCameraKind::PUBLIC;
if (deviceInfo != nullptr) {
*kind = deviceInfo->mSystemCameraKind;
return OK;
}
return deviceInfo->mSystemCameraKind;
// 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 getSystemCameraKindLocked(isHiddenAndParent.second->mId, kind);
}
// Neither a hidden physical camera nor a logical camera
return NAME_NOT_FOUND;
}
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;
}
}
}
@ -1120,7 +1153,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;
@ -1132,16 +1165,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) {

7
services/camera/libcameraservice/common/CameraProviderManager.h
Unescape View File

@ -292,8 +292,8 @@ public:
*/
bool isLogicalCamera(const std::string& id, std::vector<std::string>* physicalCameraIds);
SystemCameraKind getSystemCameraKind(const std::string& id) const;
bool isHiddenPhysicalCamera(const std::string& cameraId);
status_t getSystemCameraKind(const std::string& id, SystemCameraKind *kind) const;
bool isHiddenPhysicalCamera(const std::string& cameraId) const;
static const float kDepthARTolerance;
private:
@ -616,7 +616,8 @@ private:
CameraMetadata* characteristics) const;
void filterLogicalCameraIdsLocked(std::vector<std::string>& deviceIds) const;
SystemCameraKind getSystemCameraKindLocked(const std::string& id) const;
status_t getSystemCameraKindLocked(const std::string& id, SystemCameraKind *kind) const;
std::pair<bool, ProviderInfo::DeviceInfo *> isHiddenPhysicalCameraInternal(const std::string& cameraId) const;
void collectDeviceIdsLocked(const std::vector<std::string> deviceIds,
std::vector<std::string>& normalDeviceIds,

Write Preview
Loading…
Cancel
Save