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

Commit c524813b authored by Yin-Chia Yeh's avatar Yin-Chia Yeh
Browse files

Camera: fix race issue in Camera2 clients 

Updating of the mDevice sp<> is racy.
This change will cause the Camera3Device object lives until
the client is destructed (which can be a few seconds after
camera app closed and GC kicking in to delete the client)
The benefit is the dumpDevice can remain lock free.

Also fix a problem where we call virtual function disconnect in
Camera3Device destructor.

Test: the double free problem cannot reproduce
Bug: 112639939
Change-Id: I77762db8f59cf33891631d13fc3bdb3830ae08fb
parent c5be8b20

services/camera/libcameraservice/api1/Camera2Client.cpp

+0 −2
Original line number Diff line number Diff line
@@ -453,8 +453,6 @@ binder::Status Camera2Client::disconnect() { 


    mDevice->disconnect();



    mDevice.clear();



    CameraService::Client::disconnect();



    return res;

services/camera/libcameraservice/common/Camera2ClientBase.cpp

+1 −3
Original line number Diff line number Diff line
@@ -57,13 +57,13 @@ Camera2ClientBase<TClientBase>::Camera2ClientBase( 
                cameraId, api1CameraId, cameraFacing, clientPid, clientUid, servicePid),

        mSharedCameraCallbacks(remoteCallback),

        mDeviceVersion(cameraService->getDeviceVersion(TClientBase::mCameraIdStr)),

        mDevice(new Camera3Device(cameraId)),

        mDeviceActive(false), mApi1CameraId(api1CameraId)

{

    ALOGI("Camera %s: Opened. Client: %s (PID %d, UID %d)", cameraId.string(),

            String8(clientPackageName).string(), clientPid, clientUid);



    mInitialClientPid = clientPid;

    mDevice = new Camera3Device(cameraId);

    LOG_ALWAYS_FATAL_IF(mDevice == 0, "Device should never be NULL here.");

}
@@ -206,8 +206,6 @@ void Camera2ClientBase<TClientBase>::detachDevice() { 
    if (mDevice == 0) return;

    mDevice->disconnect();



    mDevice.clear();



    ALOGV("Camera %s: Detach complete", TClientBase::mCameraIdStr.string());

}

services/camera/libcameraservice/common/Camera2ClientBase.h

+4 −1
Original line number Diff line number Diff line
@@ -130,7 +130,10 @@ protected: 
    /** CameraDeviceBase instance wrapping HAL3+ entry */



    const int mDeviceVersion;

    sp<CameraDeviceBase>  mDevice;



    // Set to const to avoid mDevice being updated (update of sp<> is racy) during

    // dumpDevice (which is important to be lock free for debugging purpose)

    const sp<CameraDeviceBase>  mDevice;



    /** Utility members */

services/camera/libcameraservice/device3/Camera3Device.cpp

+5 −1
Original line number Diff line number Diff line
@@ -90,7 +90,7 @@ Camera3Device::~Camera3Device() 
{

    ATRACE_CALL();

    ALOGV("%s: Tearing down for camera id %s", __FUNCTION__, mId.string());

    disconnect();

    disconnectImpl();

}



const String8& Camera3Device::getId() const {
@@ -275,6 +275,10 @@ status_t Camera3Device::initializeCommonLocked() { 
}



status_t Camera3Device::disconnect() {

    return disconnectImpl();

}



status_t Camera3Device::disconnectImpl() {

    ATRACE_CALL();

    Mutex::Autolock il(mInterfaceLock);

    Mutex::Autolock stLock(mTrackerLock);

services/camera/libcameraservice/device3/Camera3Device.h

+2 −0
Original line number Diff line number Diff line
@@ -210,6 +210,8 @@ class Camera3Device : 


  private:



    status_t disconnectImpl();



    // internal typedefs

    using RequestMetadataQueue = hardware::MessageQueue<uint8_t, hardware::kSynchronizedReadWrite>;

    using ResultMetadataQueue  = hardware::MessageQueue<uint8_t, hardware::kSynchronizedReadWrite>;