From a10ab5bfd1ca27128fdbe43ce58a68e92b2896a1 Mon Sep 17 00:00:00 2001
From: Yin-Chia Yeh <yinchiayeh@google.com>
Date: Mon, 6 Nov 2017 17:16:22 -0800
Subject: [PATCH] Camera NDK: fix bug in lock order

AImage::close() can be called by AImageReader while holding
AImageReader::mLock.
Also fix ACaptureFailure double free issue.

Test: AR test app + CTS stress
Bug: 68885255
Change-Id: I17037e3e30e0f53b35ca538a3f321693c539cbdc
Merged-In: I17037e3e30e0f53b35ca538a3f321693c539cbdc
---
 camera/ndk/impl/ACameraDevice.cpp | 1 -
 media/ndk/NdkImage.cpp            | 4 ++--
 media/ndk/NdkImageReader.cpp      | 4 +++-
 media/ndk/NdkImageReaderPriv.h    | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/camera/ndk/impl/ACameraDevice.cpp b/camera/ndk/impl/ACameraDevice.cpp
index 3ae208aa14..af977b8ecb 100644
--- a/camera/ndk/impl/ACameraDevice.cpp
+++ b/camera/ndk/impl/ACameraDevice.cpp
@@ -941,7 +941,6 @@ void CameraDevice::CallbackHandler::onMessageReceived(
                     ACaptureRequest* request = allocateACaptureRequest(requestSp);
                     (*onFail)(context, session.get(), request, failure);
                     freeACaptureRequest(request);
-                    delete failure;
                     break;
                 }
                 case kWhatCaptureSeqEnd:
diff --git a/media/ndk/NdkImage.cpp b/media/ndk/NdkImage.cpp
index 87b649ad41..c4ff537687 100644
--- a/media/ndk/NdkImage.cpp
+++ b/media/ndk/NdkImage.cpp
@@ -53,7 +53,6 @@ AImage::isClosed() const {
 
 void
 AImage::close(int releaseFenceFd) {
-    lockReader();
     Mutex::Autolock _l(mLock);
     if (mIsClosed) {
         return;
@@ -71,7 +70,6 @@ AImage::close(int releaseFenceFd) {
     mBuffer = nullptr;
     mLockedBuffer = nullptr;
     mIsClosed = true;
-    unlockReader();
 }
 
 void
@@ -622,7 +620,9 @@ EXPORT
 void AImage_deleteAsync(AImage* image, int releaseFenceFd) {
     ALOGV("%s", __FUNCTION__);
     if (image != nullptr) {
+        image->lockReader();
         image->close(releaseFenceFd);
+        image->unlockReader();
         if (!image->isClosed()) {
             LOG_ALWAYS_FATAL("Image close failed!");
         }
diff --git a/media/ndk/NdkImageReader.cpp b/media/ndk/NdkImageReader.cpp
index e90783d0e7..fd6ecb51da 100644
--- a/media/ndk/NdkImageReader.cpp
+++ b/media/ndk/NdkImageReader.cpp
@@ -349,8 +349,8 @@ AImageReader::~AImageReader() {
     for (auto it = mAcquiredImages.begin();
               it != mAcquiredImages.end(); it++) {
         AImage* image = *it;
+        Mutex::Autolock _l(image->mLock);
         releaseImageLocked(image, /*releaseFenceFd*/-1);
-        image->close();
     }
 
     // Delete Buffer Items
@@ -502,6 +502,8 @@ AImageReader::releaseImageLocked(AImage* image, int releaseFenceFd) {
     mBufferItemConsumer->releaseBuffer(*buffer, bufferFence);
     returnBufferItemLocked(buffer);
     image->mBuffer = nullptr;
+    image->mLockedBuffer = nullptr;
+    image->mIsClosed = true;
 
     bool found = false;
     // cleanup acquired image list
diff --git a/media/ndk/NdkImageReaderPriv.h b/media/ndk/NdkImageReaderPriv.h
index 989b937815..bed8a21c91 100644
--- a/media/ndk/NdkImageReaderPriv.h
+++ b/media/ndk/NdkImageReaderPriv.h
@@ -85,7 +85,7 @@ struct AImageReader : public RefBase {
     // Called by AImageReader_acquireXXX to acquire a Buffer and setup AImage.
     media_status_t acquireImageLocked(/*out*/AImage** image, /*out*/int* fenceFd);
 
-    // Called by AImage to close image
+    // Called by AImage/~AImageReader to close image. Caller is responsible to grab AImage::mLock
     void releaseImageLocked(AImage* image, int releaseFenceFd);
 
     static int getBufferWidth(BufferItem* buffer);