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

Commit 7e540682 authored by Shuzhen Wang's avatar Shuzhen Wang
Browse files

Camera: Fix race between callback and unregisteration of callbacks 

Because callbacks are called from a looper thread, if the subscriber
unsubscribes and frees the callback, invalid memory access may occur from the
looper thread.

Address this by draining all pending callbacks at the time of listener
unregistration.

NOTE: There is rare chance that the wait on the condition times out,
still resulting in the original NPE issue. We intensionally allow that
to avoid deadlock, in case the application uses the same mutex lock for
callbacks and unregister functions.

Test: for i in {1..10}; do atest -it NativeImageReaderTest; done
Test: ACameraNdkVendorTest
Bug: 148976953
Change-Id: I92f5a02a4e3e63f2e72e8043ff4f4ac16c1eec5d
parent 52266779

camera/ndk/impl/ACameraManager.cpp

+49 −1
Original line number Diff line number Diff line
@@ -35,6 +35,7 @@ const char* CameraManagerGlobal::kCameraIdKey = "CameraId"; 
const char* CameraManagerGlobal::kPhysicalCameraIdKey   = "PhysicalCameraId";

const char* CameraManagerGlobal::kCallbackFpKey = "CallbackFp";

const char* CameraManagerGlobal::kContextKey    = "CallbackContext";

const nsecs_t CameraManagerGlobal::kCallbackDrainTimeout = 5000000; // 5 ms

Mutex                CameraManagerGlobal::sLock;

CameraManagerGlobal* CameraManagerGlobal::sInstance = nullptr;
@@ -117,7 +118,7 @@ sp<hardware::ICameraService> CameraManagerGlobal::getCameraServiceLocked() { 
                return nullptr;

            }

            if (mHandler == nullptr) {

                mHandler = new CallbackHandler();

                mHandler = new CallbackHandler(this);

            }

            mCbLooper->registerHandler(mHandler);

        }
@@ -211,6 +212,9 @@ void CameraManagerGlobal::registerExtendedAvailabilityCallback( 
void CameraManagerGlobal::unregisterExtendedAvailabilityCallback(

        const ACameraManager_ExtendedAvailabilityCallbacks *callback) {

    Mutex::Autolock _l(mLock);



    drainPendingCallbacksLocked();



    Callback cb(callback);

    mCallbacks.erase(cb);

}
@@ -223,10 +227,32 @@ void CameraManagerGlobal::registerAvailabilityCallback( 
void CameraManagerGlobal::unregisterAvailabilityCallback(

        const ACameraManager_AvailabilityCallbacks *callback) {

    Mutex::Autolock _l(mLock);



    drainPendingCallbacksLocked();



    Callback cb(callback);

    mCallbacks.erase(cb);

}



void CameraManagerGlobal::onCallbackCalled() {

    Mutex::Autolock _l(mLock);

    if (mPendingCallbackCnt > 0) {

        mPendingCallbackCnt--;

    }

    mCallbacksCond.signal();

}



void CameraManagerGlobal::drainPendingCallbacksLocked() {

    while (mPendingCallbackCnt > 0) {

        auto res = mCallbacksCond.waitRelative(mLock, kCallbackDrainTimeout);

        if (res != NO_ERROR) {

            ALOGE("%s: Error waiting to drain callbacks: %s(%d)",

                    __FUNCTION__, strerror(-res), res);

            break;

        }

    }

}



template<class T>

void CameraManagerGlobal::registerAvailCallback(const T *callback) {

    Mutex::Autolock _l(mLock);
@@ -250,6 +276,7 @@ void CameraManagerGlobal::registerAvailCallback(const T *callback) { 
            msg->setPointer(kCallbackFpKey, (void *) cbFunc);

            msg->setPointer(kContextKey, cb.mContext);

            msg->setString(kCameraIdKey, AString(cameraId));

            mPendingCallbackCnt++;

            msg->post();



            // Physical camera unavailable callback
@@ -263,6 +290,7 @@ void CameraManagerGlobal::registerAvailCallback(const T *callback) { 
                msg->setPointer(kContextKey, cb.mContext);

                msg->setString(kCameraIdKey, AString(cameraId));

                msg->setString(kPhysicalCameraIdKey, AString(physicalCameraId));

                mPendingCallbackCnt++;

                msg->post();

            }

        }
@@ -324,6 +352,16 @@ bool CameraManagerGlobal::isStatusAvailable(int32_t status) { 


void CameraManagerGlobal::CallbackHandler::onMessageReceived(

      const sp<AMessage> &msg) {

    onMessageReceivedInternal(msg);

    if (msg->what() == kWhatSendSingleCallback ||

            msg->what() == kWhatSendSingleAccessCallback ||

            msg->what() == kWhatSendSinglePhysicalCameraCallback) {

        notifyParent();

    }

}



void CameraManagerGlobal::CallbackHandler::onMessageReceivedInternal(

        const sp<AMessage> &msg) {

    switch (msg->what()) {

        case kWhatSendSingleCallback:

        {
@@ -405,6 +443,13 @@ void CameraManagerGlobal::CallbackHandler::onMessageReceived( 
    }

}



void CameraManagerGlobal::CallbackHandler::notifyParent() {

    sp<CameraManagerGlobal> parent = mParent.promote();

    if (parent != nullptr) {

        parent->onCallbackCalled();

    }

}



binder::Status CameraManagerGlobal::CameraServiceListener::onCameraAccessPrioritiesChanged() {

    sp<CameraManagerGlobal> cm = mCameraManager.promote();

    if (cm != nullptr) {
@@ -445,6 +490,7 @@ void CameraManagerGlobal::onCameraAccessPrioritiesChanged() { 
        if (cbFp != nullptr) {

            msg->setPointer(kCallbackFpKey, (void *) cbFp);

            msg->setPointer(kContextKey, cb.mContext);

            mPendingCallbackCnt++;

            msg->post();

        }

    }
@@ -491,6 +537,7 @@ void CameraManagerGlobal::onStatusChangedLocked( 
            msg->setPointer(kCallbackFpKey, (void *) cbFp);

            msg->setPointer(kContextKey, cb.mContext);

            msg->setString(kCameraIdKey, AString(cameraId));

            mPendingCallbackCnt++;

            msg->post();

        }

    }
@@ -545,6 +592,7 @@ void CameraManagerGlobal::onStatusChangedLocked( 
            msg->setPointer(kContextKey, cb.mContext);

            msg->setString(kCameraIdKey, AString(cameraId));

            msg->setString(kPhysicalCameraIdKey, AString(physicalCameraId));

            mPendingCallbackCnt++;

            msg->post();

        }

    }

camera/ndk/impl/ACameraManager.h

+13 −1
Original line number Diff line number Diff line
@@ -163,6 +163,12 @@ class CameraManagerGlobal final : public RefBase { 
        ACameraManager_PhysicalCameraAvailabilityCallback mPhysicalCamUnavailable;

        void*                               mContext;

    };



    android::Condition mCallbacksCond;

    size_t mPendingCallbackCnt = 0;

    void onCallbackCalled();

    void drainPendingCallbacksLocked();



    std::set<Callback> mCallbacks;



    // definition of handler and message
@@ -175,10 +181,16 @@ class CameraManagerGlobal final : public RefBase { 
    static const char* kPhysicalCameraIdKey;

    static const char* kCallbackFpKey;

    static const char* kContextKey;

    static const nsecs_t kCallbackDrainTimeout;

    class CallbackHandler : public AHandler {

      public:

        CallbackHandler() {}

        CallbackHandler(wp<CameraManagerGlobal> parent) : mParent(parent) {}

        void onMessageReceived(const sp<AMessage> &msg) override;



      private:

        wp<CameraManagerGlobal> mParent;

        void notifyParent();

        void onMessageReceivedInternal(const sp<AMessage> &msg);

    };

    sp<CallbackHandler> mHandler;

    sp<ALooper>         mCbLooper; // Looper thread where callbacks actually happen on

camera/ndk/include/camera/NdkCameraManager.h

+6 −0
Original line number Diff line number Diff line
@@ -191,6 +191,9 @@ camera_status_t ACameraManager_registerAvailabilityCallback( 
 *

 * <p>Removing a callback that isn't registered has no effect.</p>

 *

 * <p>This function must not be called with a mutex lock also held by

 * the availability callbacks.</p>

 *

 * @param manager the {@link ACameraManager} of interest.

 * @param callback the {@link ACameraManager_AvailabilityCallbacks} to be unregistered.

 *
@@ -382,6 +385,9 @@ camera_status_t ACameraManager_registerExtendedAvailabilityCallback( 
 *

 * <p>Removing a callback that isn't registered has no effect.</p>

 *

 * <p>This function must not be called with a mutex lock also held by

 * the extended availability callbacks.</p>

 *

 * @param manager the {@link ACameraManager} of interest.

 * @param callback the {@link ACameraManager_ExtendedAvailabilityCallbacks} to be unregistered.

 *

camera/ndk/ndk_vendor/impl/ACameraManager.cpp

+43 −1
Original line number Diff line number Diff line
@@ -45,6 +45,7 @@ const char* CameraManagerGlobal::kCameraIdKey = "CameraId"; 
const char* CameraManagerGlobal::kPhysicalCameraIdKey   = "PhysicalCameraId";

const char* CameraManagerGlobal::kCallbackFpKey = "CallbackFp";

const char* CameraManagerGlobal::kContextKey    = "CallbackContext";

const nsecs_t CameraManagerGlobal::kCallbackDrainTimeout = 5000000; // 5 ms

Mutex                CameraManagerGlobal::sLock;

CameraManagerGlobal* CameraManagerGlobal::sInstance = nullptr;
@@ -249,7 +250,7 @@ sp<ICameraService> CameraManagerGlobal::getCameraService() { 
                return nullptr;

            }

            if (mHandler == nullptr) {

                mHandler = new CallbackHandler();

                mHandler = new CallbackHandler(this);

            }

            mCbLooper->registerHandler(mHandler);

        }
@@ -317,6 +318,7 @@ void CameraManagerGlobal::registerAvailabilityCallback( 
void CameraManagerGlobal::unregisterAvailabilityCallback(

        const ACameraManager_AvailabilityCallbacks *callback) {

    Mutex::Autolock _l(mLock);

    drainPendingCallbacksLocked();

    Callback cb(callback);

    mCallbacks.erase(cb);

}
@@ -329,10 +331,30 @@ void CameraManagerGlobal::registerExtendedAvailabilityCallback( 
void CameraManagerGlobal::unregisterExtendedAvailabilityCallback(

        const ACameraManager_ExtendedAvailabilityCallbacks *callback) {

    Mutex::Autolock _l(mLock);

    drainPendingCallbacksLocked();

    Callback cb(callback);

    mCallbacks.erase(cb);

}



void CameraManagerGlobal::onCallbackCalled() {

    Mutex::Autolock _l(mLock);

    if (mPendingCallbackCnt > 0) {

        mPendingCallbackCnt--;

    }

    mCallbacksCond.signal();

}



void CameraManagerGlobal::drainPendingCallbacksLocked() {

    while (mPendingCallbackCnt > 0) {

        auto res = mCallbacksCond.waitRelative(mLock, kCallbackDrainTimeout);

        if (res != NO_ERROR) {

            ALOGE("%s: Error waiting to drain callbacks: %s(%d)",

                    __FUNCTION__, strerror(-res), res);

            break;

        }

    }

}



template <class T>

void CameraManagerGlobal::registerAvailCallback(const T *callback) {

    Mutex::Autolock _l(mLock);
@@ -351,6 +373,7 @@ void CameraManagerGlobal::registerAvailCallback(const T *callback) { 
            msg->setPointer(kCallbackFpKey, (void *) cbFunc);

            msg->setPointer(kContextKey, cb.mContext);

            msg->setString(kCameraIdKey, AString(cameraId.c_str()));

            mPendingCallbackCnt++;

            msg->post();



            // Physical camera unavailable callback
@@ -363,6 +386,7 @@ void CameraManagerGlobal::registerAvailCallback(const T *callback) { 
                msg->setPointer(kContextKey, cb.mContext);

                msg->setString(kCameraIdKey, AString(cameraId.c_str()));

                msg->setString(kPhysicalCameraIdKey, AString(physicalCameraId.c_str()));

                mPendingCallbackCnt++;

                msg->post();

            }

        }
@@ -407,6 +431,15 @@ bool CameraManagerGlobal::isStatusAvailable(CameraDeviceStatus status) { 


void CameraManagerGlobal::CallbackHandler::onMessageReceived(

      const sp<AMessage> &msg) {

    onMessageReceivedInternal(msg);

    if (msg->what() == kWhatSendSingleCallback ||

            msg->what() == kWhatSendSinglePhysicalCameraCallback) {

        notifyParent();

    }

}



void CameraManagerGlobal::CallbackHandler::onMessageReceivedInternal(

        const sp<AMessage> &msg) {

    switch (msg->what()) {

        case kWhatSendSingleCallback:

        {
@@ -466,6 +499,13 @@ void CameraManagerGlobal::CallbackHandler::onMessageReceived( 
    }

}



void CameraManagerGlobal::CallbackHandler::notifyParent() {

    sp<CameraManagerGlobal> parent = mParent.promote();

    if (parent != nullptr) {

        parent->onCallbackCalled();

    }

}



hardware::Return<void> CameraManagerGlobal::CameraServiceListener::onStatusChanged(

        const CameraStatusAndId &statusAndId) {

    sp<CameraManagerGlobal> cm = mCameraManager.promote();
@@ -512,6 +552,7 @@ void CameraManagerGlobal::onStatusChangedLocked( 
        msg->setPointer(kCallbackFpKey, (void *) cbFp);

        msg->setPointer(kContextKey, cb.mContext);

        msg->setString(kCameraIdKey, AString(cameraId.c_str()));

        mPendingCallbackCnt++;

        msg->post();

    }

    if (status == CameraDeviceStatus::STATUS_NOT_PRESENT) {
@@ -577,6 +618,7 @@ void CameraManagerGlobal::onStatusChangedLocked( 
            msg->setPointer(kContextKey, cb.mContext);

            msg->setString(kCameraIdKey, AString(cameraId.c_str()));

            msg->setString(kPhysicalCameraIdKey, AString(physicalCameraId.c_str()));

            mPendingCallbackCnt++;

            msg->post();

        }

    }

camera/ndk/ndk_vendor/impl/ACameraManager.h

+12 −1
Original line number Diff line number Diff line
@@ -156,6 +156,12 @@ class CameraManagerGlobal final : public RefBase { 
        ACameraManager_PhysicalCameraAvailabilityCallback mPhysicalCamUnavailable;

        void*                               mContext;

    };



    android::Condition mCallbacksCond;

    size_t mPendingCallbackCnt = 0;

    void onCallbackCalled();

    void drainPendingCallbacksLocked();



    std::set<Callback> mCallbacks;



    // definition of handler and message
@@ -167,10 +173,15 @@ class CameraManagerGlobal final : public RefBase { 
    static const char* kPhysicalCameraIdKey;

    static const char* kCallbackFpKey;

    static const char* kContextKey;

    static const nsecs_t kCallbackDrainTimeout;

    class CallbackHandler : public AHandler {

      public:

        CallbackHandler() {}

        CallbackHandler(wp<CameraManagerGlobal> parent) : mParent(parent) {}

        void onMessageReceived(const sp<AMessage> &msg) override;

      private:

        wp<CameraManagerGlobal> mParent;

        void notifyParent();

        void onMessageReceivedInternal(const sp<AMessage> &msg);

    };

    sp<CallbackHandler> mHandler;

    sp<ALooper>         mCbLooper; // Looper thread where callbacks actually happen on