From 666ad3a9ac1492744256e86ada9617a897c0b799 Mon Sep 17 00:00:00 2001
From: Avichal Rakesh <arakesh@google.com>
Date: Tue, 21 Dec 2021 17:10:35 -0800
Subject: [PATCH] Prevent onImageAvailable from being called on stale listeners

ImageReader queues up calls to OnImageAvailableListener#onImageAvailable
(one for each frame) by wrapping them in Runnables and passing the
Runnables to an Executor, which in turn executes Runnables in order.
Before queuing the Runnable, ImageReader reads the value of listener
and passes a reference to the listener that is current at the time of
queueing to the Runnable. This reference is used when calling
onImageAvailable.

If the listener changes between when a Runnable is queued and when the
Runnable is executed, it is possible for the Runnable to call into a
stale listener.

This CL replaces the mutex used to guard the state of ImageReader with a
StampedLock behaving as a Read-Write Lock. The Runnables wrapping calls
to OnImageAvailableListener#onImageAvailable grab a read lock and calls
the onImageAvailable method of the listener referenced by ImageReader at
the time of Runnable's execution. Any call to modify the listener grabs
the write lock.

Bug: 204438677
Test: Existing CTS tests pass

Change-Id: I7b11243962595e8c41912a483a80ecd8d3c3e945
---
 media/java/android/media/ImageReader.java | 64 +++++++++++++++--------
 1 file changed, 43 insertions(+), 21 deletions(-)

diff --git a/media/java/android/media/ImageReader.java b/media/java/android/media/ImageReader.java
index 09d7fbd1df36a..c6f66d7592ee7 100644
--- a/media/java/android/media/ImageReader.java
+++ b/media/java/android/media/ImageReader.java
@@ -43,6 +43,7 @@ import java.util.Objects;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.Executor;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.locks.StampedLock;
 
 /**
  * <p>The ImageReader class allows direct application access to image data
@@ -675,7 +676,8 @@ public class ImageReader implements AutoCloseable {
      *            If no handler specified and the calling thread has no looper.
      */
     public void setOnImageAvailableListener(OnImageAvailableListener listener, Handler handler) {
-        synchronized (mListenerLock) {
+        long writeStamp = mListenerLock.writeLock();
+        try {
             if (listener != null) {
                 Looper looper = handler != null ? handler.getLooper() : Looper.myLooper();
                 if (looper == null) {
@@ -691,6 +693,8 @@ public class ImageReader implements AutoCloseable {
                 mListenerExecutor = null;
             }
             mListener = listener;
+        } finally {
+            mListenerLock.unlockWrite(writeStamp);
         }
     }
 
@@ -713,9 +717,12 @@ public class ImageReader implements AutoCloseable {
             throw new IllegalArgumentException("executor must not be null");
         }
 
-        synchronized (mListenerLock) {
+        long writeStamp = mListenerLock.writeLock();
+        try {
             mListenerExecutor = executor;
             mListener = listener;
+        } finally {
+            mListenerLock.unlockWrite(writeStamp);
         }
     }
 
@@ -731,6 +738,8 @@ public class ImageReader implements AutoCloseable {
         /**
          * Callback that is called when a new image is available from ImageReader.
          *
+         * This callback must not modify or close the passed {@code reader}.
+         *
          * @param reader the ImageReader the callback is associated with.
          * @see ImageReader
          * @see Image
@@ -889,28 +898,41 @@ public class ImageReader implements AutoCloseable {
             return;
         }
 
-        final Executor executor;
-        final OnImageAvailableListener listener;
-        synchronized (ir.mListenerLock) {
-            executor = ir.mListenerExecutor;
-            listener = ir.mListener;
-        }
-        final boolean isReaderValid;
         synchronized (ir.mCloseLock) {
-            isReaderValid = ir.mIsReaderValid;
+            if (!ir.mIsReaderValid) {
+                // It's dangerous to fire onImageAvailable() callback when the ImageReader
+                // is being closed, as application could acquire next image in the
+                // onImageAvailable() callback.
+                return;
+            }
         }
 
-        // It's dangerous to fire onImageAvailable() callback when the ImageReader
-        // is being closed, as application could acquire next image in the
-        // onImageAvailable() callback.
-        if (executor != null && listener != null && isReaderValid) {
-            executor.execute(new Runnable() {
-                @Override
-                public void run() {
-                    listener.onImageAvailable(ir);
-                }
-            });
+        final Executor executor;
+        final long readStamp = ir.mListenerLock.readLock();
+        try {
+            executor = ir.mListenerExecutor;
+            if (executor == null) {
+                return;
+            }
+        } finally {
+            ir.mListenerLock.unlockRead(readStamp);
         }
+
+        executor.execute(() -> {
+            // Acquire readlock to ensure that the ImageReader does not change its
+            // state while a listener is actively processing.
+            final long rStamp = ir.mListenerLock.readLock();
+            try {
+                // Fire onImageAvailable of the latest non-null listener
+                // This ensures that if the listener changes while messages are in queue, the
+                // in-flight messages will call onImageAvailable of the new listener instead
+                if (ir.mListener != null) {
+                    ir.mListener.onImageAvailable(ir);
+                }
+            } finally {
+                ir.mListenerLock.unlockRead(rStamp);
+            }
+        });
     }
 
     /**
@@ -1070,7 +1092,7 @@ public class ImageReader implements AutoCloseable {
     private Surface mSurface;
     private int mEstimatedNativeAllocBytes;
 
-    private final Object mListenerLock = new Object();
+    private final StampedLock mListenerLock = new StampedLock();
     private final Object mCloseLock = new Object();
     private boolean mIsReaderValid = false;
     private OnImageAvailableListener mListener;