commit ebca5b9862df0175b913f172384d80123a3865e3
author Jayant Chowdhary <jchowdhary@google.com> Mon Jul 01 13:18:17 2019 -0700
committer Jayant Chowdhary <jchowdhary@google.com> Mon Jul 01 13:30:21 2019 -0700
tree 4a579fbe83201ceaec472df2a9d9cf6c39ebb3bb
parent 265eb9954ebf6ded1fc10577a29fa49081af9f9b

AImageReader: make sure ~AImageReader isn't called with FrameListener::mLock held.

The following sequence of events is possible:

t1: FrameListener::onFrameAvailable callback is called, mReader is
    promoted from wp<> to sp<>, t1 holds mLock.
  t2: AImageReader_delete is called, decStrong is called on AImageReader,
      but since its refcount isn't 0, ~AImageReader isn't called
    t1: onFrameAvailable completes, ~AImageReader is called with mLock
        held, ~AImageReader->
        setImageListenerLocked->FrameListener::setImageListener->tries
        to lock mLock again, t1 deadlocks.

We move the locking mLock to after the promotion of mReader to sp<> so
that it gets destructed before ~AImageReader is called.

The same is done for BufferRemovedListener.

Bug: 136193631

Test: Auth; unlock

Change-Id: I8bb8c7d59f3711fd9fe434159095938eb5db9153
Signed-off-by: Jayant Chowdhary <jchowdhary@google.com>

media/ndk/NdkImageReader.cpp

diff --git a/media/ndk/NdkImageReader.cpp b/media/ndk/NdkImageReader.cpp
index baa4fc7..830f752 100644
--- a/media/ndk/NdkImageReader.cpp
+++ b/media/ndk/NdkImageReader.cpp
@@ -113,12 +113,12 @@
 
 void
 AImageReader::FrameListener::onFrameAvailable(const BufferItem& /*item*/) {
-    Mutex::Autolock _l(mLock);
     sp<AImageReader> reader = mReader.promote();
     if (reader == nullptr) {
         ALOGW("A frame is available after AImageReader closed!");
         return; // reader has been closed
     }
+    Mutex::Autolock _l(mLock);
     if (mListener.onImageAvailable == nullptr) {
         return; // No callback registered
     }
@@ -143,12 +143,12 @@
 
 void
 AImageReader::BufferRemovedListener::onBufferFreed(const wp<GraphicBuffer>& graphicBuffer) {
-    Mutex::Autolock _l(mLock);
     sp<AImageReader> reader = mReader.promote();
     if (reader == nullptr) {
         ALOGW("A frame is available after AImageReader closed!");
         return; // reader has been closed
     }
+    Mutex::Autolock _l(mLock);
     if (mListener.onBufferRemoved == nullptr) {
         return; // No callback registered
     }

media/ndk/NdkImageReaderPriv.h

diff --git a/media/ndk/NdkImageReaderPriv.h b/media/ndk/NdkImageReaderPriv.h
index e328cb1..19bd704 100644
--- a/media/ndk/NdkImageReaderPriv.h
+++ b/media/ndk/NdkImageReaderPriv.h
@@ -134,7 +134,7 @@
 
       private:
         AImageReader_ImageListener mListener = {nullptr, nullptr};
-        wp<AImageReader>           mReader;
+        const wp<AImageReader>     mReader;
         Mutex                      mLock;
     };
     sp<FrameListener> mFrameListener;
@@ -149,7 +149,7 @@
 
        private:
         AImageReader_BufferRemovedListener mListener = {nullptr, nullptr};
-        wp<AImageReader>           mReader;
+        const wp<AImageReader>     mReader;
         Mutex                      mLock;
     };
     sp<BufferRemovedListener> mBufferRemovedListener;