Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 407571f7 authored by Jayant Chowdhary's avatar Jayant Chowdhary
Browse files

cameraserver: fix deadlock scenario in torchModeStatusChanged callback. 



The following scenario can occur:

T1: CameraService::connectDevice()
      CameraService::connectDeviceHelper()
         CameraProviderManager::openSession() ---> holds mInterfaceLock
         .
         .
         . on the same thread before openSession execution completes
           CameraProviderManager::ProviderInfo::torchModeStatusChange() callback from HAL
           .
             CameraService::onTorchStatusChanged()
               CameraProviderManager::getSystemCameraKind tries to lock mInterfaceLock -> deadlock.

We now pass in system camera kind to onTorchStatusChanged in
CameraProviderManager::torchModeStatusChange() instead of calling getSystemCameraKind

This CL also removes CameraProviderManager::mStatusListenerMutex, since
it wasn't protecting any data structure.

Bug: 202198748

Test: camera CTS, GCA (basic validity)

Merged-In: Id95a2aa061b6cb4db4a25b1a2aa6a390f898af87
Change-Id: Id95a2aa061b6cb4db4a25b1a2aa6a390f898af87
Signed-off-by: default avatarJayant Chowdhary <jchowdhary@google.com>
parent cd2b79a8

services/camera/libcameraservice/CameraService.cpp

+7 −0
Original line number Diff line number Diff line
@@ -506,6 +506,13 @@ void CameraService::onTorchStatusChanged(const String8& cameraId, 
    onTorchStatusChangedLocked(cameraId, newStatus);

}





void CameraService::onTorchStatusChanged(const String8& cameraId,

        TorchModeStatus newStatus, SystemCameraKind systemCameraKind) {

    Mutex::Autolock al(mTorchStatusMutex);

    onTorchStatusChangedLocked(cameraId, newStatus, systemCameraKind);

}



void CameraService::onTorchStatusChangedLocked(const String8& cameraId,

        TorchModeStatus newStatus) {

    ALOGI("%s: Torch status changed for cameraId=%s, newStatus=%d",

services/camera/libcameraservice/CameraService.h

+8 −0
Original line number Diff line number Diff line
@@ -109,8 +109,16 @@ public: 
    virtual void        onDeviceStatusChanged(const String8 &cameraId,

            const String8 &physicalCameraId,

            hardware::camera::common::V1_0::CameraDeviceStatus newHalStatus) override;

    // This method may hold CameraProviderManager::mInterfaceMutex as a part

    // of calling getSystemCameraKind() internally. Care should be taken not to

    // directly / indirectly call this from callers who also hold

    // mInterfaceMutex.

    virtual void        onTorchStatusChanged(const String8& cameraId,

            hardware::camera::common::V1_0::TorchModeStatus newStatus) override;

    // Does not hold CameraProviderManager::mInterfaceMutex.

    virtual void        onTorchStatusChanged(const String8& cameraId,

            hardware::camera::common::V1_0::TorchModeStatus newStatus,

            SystemCameraKind kind) override;

    virtual void        onNewProviderRegistered() override;



    /////////////////////////////////////////////////////////////////////

services/camera/libcameraservice/common/CameraProviderManager.cpp

+15 −4
Original line number Diff line number Diff line
@@ -1836,16 +1836,19 @@ hardware::Return<void> CameraProviderManager::ProviderInfo::torchModeStatusChang 
        const hardware::hidl_string& cameraDeviceName,

        TorchModeStatus newStatus) {

    sp<StatusListener> listener;

    SystemCameraKind systemCameraKind = SystemCameraKind::PUBLIC;

    std::string id;

    {

        std::lock_guard<std::mutex> lock(mManager->mStatusListenerMutex);

    bool known = false;

    {

        // Hold mLock for accessing mDevices

        std::lock_guard<std::mutex> lock(mLock);

        for (auto& deviceInfo : mDevices) {

            if (deviceInfo->mName == cameraDeviceName) {

                ALOGI("Camera device %s torch status is now %s", cameraDeviceName.c_str(),

                        torchStatusToString(newStatus));

                id = deviceInfo->mId;

                known = true;

                systemCameraKind = deviceInfo->mSystemCameraKind;

                if (TorchModeStatus::AVAILABLE_ON != newStatus) {

                    mManager->removeRef(DeviceMode::TORCH, id);

                }
@@ -1857,11 +1860,19 @@ hardware::Return<void> CameraProviderManager::ProviderInfo::torchModeStatusChang 
                    mProviderName.c_str(), cameraDeviceName.c_str(), newStatus);

            return hardware::Void();

        }

        // no lock needed since listener is set up only once during

        // CameraProviderManager initialization and then never changed till it is

        // destructed.

        listener = mManager->getStatusListener();

     }

    // Call without lock held to allow reentrancy into provider manager

    // The problem with holding mLock here is that we

    // might be limiting re-entrancy : CameraService::onTorchStatusChanged calls

    // back into CameraProviderManager which might try to hold mLock again (eg:

    // findDeviceInfo, which should be holding mLock while iterating through

    // each provider's devices).

    if (listener != nullptr) {

        listener->onTorchStatusChanged(String8(id.c_str()), newStatus);

        listener->onTorchStatusChanged(String8(id.c_str()), newStatus, systemCameraKind);

    }

    return hardware::Void();

}

services/camera/libcameraservice/common/CameraProviderManager.h

+3 −2
Original line number Diff line number Diff line
@@ -142,6 +142,9 @@ public: 
        virtual void onDeviceStatusChanged(const String8 &cameraId,

                const String8 &physicalCameraId,

                hardware::camera::common::V1_0::CameraDeviceStatus newStatus) = 0;

        virtual void onTorchStatusChanged(const String8 &cameraId,

                hardware::camera::common::V1_0::TorchModeStatus newStatus,

                SystemCameraKind kind) = 0;

        virtual void onTorchStatusChanged(const String8 &cameraId,

                hardware::camera::common::V1_0::TorchModeStatus newStatus) = 0;

        virtual void onNewProviderRegistered() = 0;
@@ -325,8 +328,6 @@ private: 
    // All private members, unless otherwise noted, expect mInterfaceMutex to be locked before use

    mutable std::mutex mInterfaceMutex;



    // the status listener update callbacks will lock mStatusMutex

    mutable std::mutex mStatusListenerMutex;

    wp<StatusListener> mListener;

    ServiceInteractionProxy* mServiceProxy;

services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp

+2 −0
Original line number Diff line number Diff line
@@ -253,6 +253,8 @@ struct TestStatusListener : public CameraProviderManager::StatusListener { 
            hardware::camera::common::V1_0::CameraDeviceStatus) override {}

    void onTorchStatusChanged(const String8 &,

            hardware::camera::common::V1_0::TorchModeStatus) override {}

    void onTorchStatusChanged(const String8 &,

            hardware::camera::common::V1_0::TorchModeStatus, SystemCameraKind) override {}

    void onNewProviderRegistered() override {}

};