commit 7e540682b362a3e8679e274084aeaefdf01314da
author Shuzhen Wang <shuzhenwang@google.com> Fri Apr 10 13:30:25 2020 -0700
committer Shuzhen Wang <shuzhenwang@google.com> Mon Apr 13 17:48:27 2020 -0700
tree 9a4a88c8548c0a4b672e2d208ecfa88b45580d8d
parent 5226677901af08551446c103bf7d15797e87756c

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

camera/ndk/impl/ACameraManager.cpp

diff --git a/camera/ndk/impl/ACameraManager.cpp b/camera/ndk/impl/ACameraManager.cpp
index f408b6a..b58ebe2 100644
--- a/camera/ndk/impl/ACameraManager.cpp
+++ b/camera/ndk/impl/ACameraManager.cpp
@@ -35,6 +35,7 @@
 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 @@
                 return nullptr;
             }
             if (mHandler == nullptr) {
-                mHandler = new CallbackHandler();
+                mHandler = new CallbackHandler(this);
             }
             mCbLooper->registerHandler(mHandler);
         }
@@ -211,6 +212,9 @@
 void CameraManagerGlobal::unregisterExtendedAvailabilityCallback(
         const ACameraManager_ExtendedAvailabilityCallbacks *callback) {
     Mutex::Autolock _l(mLock);
+
+    drainPendingCallbacksLocked();
+
     Callback cb(callback);
     mCallbacks.erase(cb);
 }
@@ -223,10 +227,32 @@
 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 @@
             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 @@
                 msg->setPointer(kContextKey, cb.mContext);
                 msg->setString(kCameraIdKey, AString(cameraId));
                 msg->setString(kPhysicalCameraIdKey, AString(physicalCameraId));
+                mPendingCallbackCnt++;
                 msg->post();
             }
         }
@@ -323,6 +351,16 @@
 }
 
 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::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 @@
         if (cbFp != nullptr) {
             msg->setPointer(kCallbackFpKey, (void *) cbFp);
             msg->setPointer(kContextKey, cb.mContext);
+            mPendingCallbackCnt++;
             msg->post();
         }
     }
@@ -491,6 +537,7 @@
             msg->setPointer(kCallbackFpKey, (void *) cbFp);
             msg->setPointer(kContextKey, cb.mContext);
             msg->setString(kCameraIdKey, AString(cameraId));
+            mPendingCallbackCnt++;
             msg->post();
         }
     }
@@ -545,6 +592,7 @@
             msg->setPointer(kContextKey, cb.mContext);
             msg->setString(kCameraIdKey, AString(cameraId));
             msg->setString(kPhysicalCameraIdKey, AString(physicalCameraId));
+            mPendingCallbackCnt++;
             msg->post();
         }
     }