From a8488c9c32de3d4cfa6f9b96bec362bbc337a91b Mon Sep 17 00:00:00 2001
From: Jayant Chowdhary <jchowdhary@google.com>
Date: Fri, 21 Jun 2019 12:45:34 -0700
Subject: [PATCH] camera2 ndk/vndk: cleanup->stop CameraDevice's looper in
 ~ACameraDevice()

It's possible that the following sequence happens:

1) hwbinder / binder thread T1: onResultReceived() starts -> promotes wp<CameraDevice> to sp<>;

2) Some other app thread T2 : ACameraDevice_close() -> delete ACameraDevice -> doesn't result in
                              CameraDevice's destructor running since mCameraDevice has another live
                              reference, app destroys some object O1.

3) T3 (callback looper thread): callback is received since looper is still running which accesses
                                dead app object O1 -> results in undefined behavior.

4) T1: onResultReceived completes and CameraDevice is destructed

We need to stop CameraDevice's looper thread (that waits for all callbacks queued to complete) in
~ACameraDevice() so we receive no callbacks after ACameraDevice is closed.

Bug: 135641415

Test: CTS native tests: no new failures
Test: AImageReaderVendorTest; enroll; while(1) auth;

Change-Id: Ia24de753f6ee409d941fff39616f09df2164880a
Merged-In: Ia24de753f6ee409d941fff39616f09df2164880a
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>
(cherry picked from commit 174084011ca8b593a8cf35412928517b9e864be9)
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>
---
 camera/ndk/impl/ACameraDevice.cpp            | 22 ++++++++++++++------
 camera/ndk/impl/ACameraDevice.h              |  5 ++++-
 camera/ndk/ndk_vendor/impl/ACameraDevice.cpp | 22 ++++++++++++++------
 camera/ndk/ndk_vendor/impl/ACameraDevice.h   |  5 ++++-
 4 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/camera/ndk/impl/ACameraDevice.cpp b/camera/ndk/impl/ACameraDevice.cpp
index 25a81ebc28..d24cb814a3 100644
--- a/camera/ndk/impl/ACameraDevice.cpp
+++ b/camera/ndk/impl/ACameraDevice.cpp
@@ -28,6 +28,10 @@
 
 #include "ACameraCaptureSession.inc"
 
+ACameraDevice::~ACameraDevice() {
+    mDevice->stopLooper();
+}
+
 namespace android {
 namespace acam {
 
@@ -116,14 +120,10 @@ CameraDevice::~CameraDevice() {
         if (!isClosed()) {
             disconnectLocked(session);
         }
+        LOG_ALWAYS_FATAL_IF(mCbLooper != nullptr,
+                "CameraDevice looper should've been stopped before ~CameraDevice");
         mCurrentSession = nullptr;
-        if (mCbLooper != nullptr) {
-            mCbLooper->unregisterHandler(mHandler->id());
-            mCbLooper->stop();
-        }
     }
-    mCbLooper.clear();
-    mHandler.clear();
 }
 
 void
@@ -892,6 +892,16 @@ CameraDevice::onCaptureErrorLocked(
     return;
 }
 
+void CameraDevice::stopLooper() {
+    Mutex::Autolock _l(mDeviceLock);
+    if (mCbLooper != nullptr) {
+      mCbLooper->unregisterHandler(mHandler->id());
+      mCbLooper->stop();
+    }
+    mCbLooper.clear();
+    mHandler.clear();
+}
+
 CameraDevice::CallbackHandler::CallbackHandler(const char* id) : mId(id) {
 }
 
diff --git a/camera/ndk/impl/ACameraDevice.h b/camera/ndk/impl/ACameraDevice.h
index c92a95f822..7a35bf0e81 100644
--- a/camera/ndk/impl/ACameraDevice.h
+++ b/camera/ndk/impl/ACameraDevice.h
@@ -109,6 +109,9 @@ class CameraDevice final : public RefBase {
 
     inline ACameraDevice* getWrapper() const { return mWrapper; };
 
+    // Stop the looper thread and unregister the handler
+    void stopLooper();
+
   private:
     friend ACameraCaptureSession;
     camera_status_t checkCameraClosedOrErrorLocked() const;
@@ -354,7 +357,7 @@ struct ACameraDevice {
                   sp<ACameraMetadata> chars) :
             mDevice(new android::acam::CameraDevice(id, cb, chars, this)) {}
 
-    ~ACameraDevice() {};
+    ~ACameraDevice();
 
     /*******************
      * NDK public APIs *
diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp
index 529c167edc..24b1cae2de 100644
--- a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp
+++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp
@@ -33,6 +33,10 @@
 
 using namespace android;
 
+ACameraDevice::~ACameraDevice() {
+    mDevice->stopLooper();
+}
+
 namespace android {
 namespace acam {
 
@@ -119,13 +123,9 @@ CameraDevice::~CameraDevice() {
             disconnectLocked(session);
         }
         mCurrentSession = nullptr;
-        if (mCbLooper != nullptr) {
-            mCbLooper->unregisterHandler(mHandler->id());
-            mCbLooper->stop();
-        }
+        LOG_ALWAYS_FATAL_IF(mCbLooper != nullptr,
+            "CameraDevice looper should've been stopped before ~CameraDevice");
     }
-    mCbLooper.clear();
-    mHandler.clear();
 }
 
 void
@@ -1422,6 +1422,16 @@ CameraDevice::checkAndFireSequenceCompleteLocked() {
     }
 }
 
+void CameraDevice::stopLooper() {
+    Mutex::Autolock _l(mDeviceLock);
+    if (mCbLooper != nullptr) {
+      mCbLooper->unregisterHandler(mHandler->id());
+      mCbLooper->stop();
+    }
+    mCbLooper.clear();
+    mHandler.clear();
+}
+
 /**
   * Camera service callback implementation
   */
diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.h b/camera/ndk/ndk_vendor/impl/ACameraDevice.h
index 829b08452e..3328a85686 100644
--- a/camera/ndk/ndk_vendor/impl/ACameraDevice.h
+++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.h
@@ -133,6 +133,9 @@ class CameraDevice final : public RefBase {
     bool setDeviceMetadataQueues();
     inline ACameraDevice* getWrapper() const { return mWrapper; };
 
+    // Stop the looper thread and unregister the handler
+    void stopLooper();
+
   private:
     friend ACameraCaptureSession;
 
@@ -383,7 +386,7 @@ struct ACameraDevice {
                   sp<ACameraMetadata> chars) :
             mDevice(new android::acam::CameraDevice(id, cb, std::move(chars), this)) {}
 
-    ~ACameraDevice() {};
+    ~ACameraDevice();
 
     /*******************
      * NDK public APIs *