commit 74faf2a51976af28fcea71c09835e965dc8c4b46
author Ray Chin <raychin@google.com> Fri Dec 15 19:07:17 2023 +0800
committer Ray Chin <raychin@google.com> Fri Dec 15 19:07:17 2023 +0800
tree 9798a39910bb4b662b9ae43fdd858131083bc2d6
parent 311d837128a2369dd8a210b3cd2acb3b7dd375ae

Fix FilterClientCallbackImpl crash caused by race condition

Bug: 315422402
Test: Run on reference design device Ohm.
Change-Id: I79062b338049b36189223814fdf30e90b34e85ab

media/jni/android_media_tv_Tuner.cpp

diff --git a/media/jni/android_media_tv_Tuner.cpp b/media/jni/android_media_tv_Tuner.cpp
index 757e9f8..2da9b9a 100644
--- a/media/jni/android_media_tv_Tuner.cpp
+++ b/media/jni/android_media_tv_Tuner.cpp
@@ -693,6 +693,8 @@
                                            mpuSequenceNumber, isPesPrivateData, sc,
                                            audioDescriptor.get(), presentationsJObj.get()));
 
+    // Protect mFilterClient from being set to null.
+    android::Mutex::Autolock autoLock(mLock);
     uint64_t avSharedMemSize = mFilterClient->getAvSharedHandleInfo().size;
     if (mediaEvent.avMemory.fds.size() > 0 || mediaEvent.avDataId != 0 ||
         (dataLength > 0 && (dataLength + offset) < avSharedMemSize)) {
@@ -939,38 +941,52 @@
             }
         }
     }
-    ScopedLocalRef filter(env, env->NewLocalRef(mFilterObj));
-    if (!env->IsSameObject(filter.get(), nullptr)) {
-        jmethodID methodID = gFields.onFilterEventID;
-        if (mSharedFilter) {
-            methodID = gFields.onSharedFilterEventID;
+
+    ScopedLocalRef<jobject> filter(env);
+    {
+        android::Mutex::Autolock autoLock(mLock);
+        if (env->IsSameObject(mFilterObj, nullptr)) {
+            ALOGE("FilterClientCallbackImpl::onFilterEvent:"
+                  "Filter object has been freed. Ignoring callback.");
+            return;
+        } else {
+            filter.reset(env->NewLocalRef(mFilterObj));
         }
-        env->CallVoidMethod(filter.get(), methodID, array.get());
-    } else {
-        ALOGE("FilterClientCallbackImpl::onFilterEvent:"
-              "Filter object has been freed. Ignoring callback.");
     }
+
+    jmethodID methodID = gFields.onFilterEventID;
+    if (mSharedFilter) {
+        methodID = gFields.onSharedFilterEventID;
+    }
+    env->CallVoidMethod(filter.get(), methodID, array.get());
 }
 
 void FilterClientCallbackImpl::onFilterStatus(const DemuxFilterStatus status) {
     ALOGV("FilterClientCallbackImpl::onFilterStatus");
     JNIEnv *env = AndroidRuntime::getJNIEnv();
-    ScopedLocalRef filter(env, env->NewLocalRef(mFilterObj));
-    if (!env->IsSameObject(filter.get(), nullptr)) {
-        jmethodID methodID = gFields.onFilterStatusID;
-        if (mSharedFilter) {
-            methodID = gFields.onSharedFilterStatusID;
+    ScopedLocalRef<jobject> filter(env);
+    {
+        android::Mutex::Autolock autoLock(mLock);
+        if (env->IsSameObject(filter.get(), nullptr)) {
+            ALOGE("FilterClientCallbackImpl::onFilterStatus:"
+                  "Filter object has been freed. Ignoring callback.");
+            return;
+        } else {
+            filter.reset(env->NewLocalRef(mFilterObj));
         }
-        env->CallVoidMethod(filter.get(), methodID, (jint)static_cast<uint8_t>(status));
-    } else {
-        ALOGE("FilterClientCallbackImpl::onFilterStatus:"
-              "Filter object has been freed. Ignoring callback.");
     }
+
+    jmethodID methodID = gFields.onFilterStatusID;
+    if (mSharedFilter) {
+        methodID = gFields.onSharedFilterStatusID;
+    }
+    env->CallVoidMethod(filter.get(), methodID, (jint)static_cast<uint8_t>(status));
 }
 
 void FilterClientCallbackImpl::setFilter(jweak filterObj, sp<FilterClient> filterClient) {
     ALOGV("FilterClientCallbackImpl::setFilter");
     // Java Object
+    android::Mutex::Autolock autoLock(mLock);
     mFilterObj = filterObj;
     mFilterClient = filterClient;
     mSharedFilter = false;
@@ -979,6 +995,7 @@
 void FilterClientCallbackImpl::setSharedFilter(jweak filterObj, sp<FilterClient> filterClient) {
     ALOGV("FilterClientCallbackImpl::setFilter");
     // Java Object
+    android::Mutex::Autolock autoLock(mLock);
     mFilterObj = filterObj;
     mFilterClient = filterClient;
     mSharedFilter = true;
@@ -1047,11 +1064,14 @@
 
 FilterClientCallbackImpl::~FilterClientCallbackImpl() {
     JNIEnv *env = AndroidRuntime::getJNIEnv();
-    if (mFilterObj != nullptr) {
-        env->DeleteWeakGlobalRef(mFilterObj);
-        mFilterObj = nullptr;
+    {
+        android::Mutex::Autolock autoLock(mLock);
+        if (mFilterObj != nullptr) {
+            env->DeleteWeakGlobalRef(mFilterObj);
+            mFilterObj = nullptr;
+        }
+        mFilterClient = nullptr;
     }
-    mFilterClient = nullptr;
     env->DeleteGlobalRef(mEventClass);
     env->DeleteGlobalRef(mSectionEventClass);
     env->DeleteGlobalRef(mMediaEventClass);

media/jni/android_media_tv_Tuner.h

diff --git a/media/jni/android_media_tv_Tuner.h b/media/jni/android_media_tv_Tuner.h
index 01c998d..3de3ab9 100644
--- a/media/jni/android_media_tv_Tuner.h
+++ b/media/jni/android_media_tv_Tuner.h
@@ -136,6 +136,7 @@
 private:
     jweak mFilterObj;
     sp<FilterClient> mFilterClient;
+    android::Mutex mLock;
     jclass mEventClass;
     jclass mSectionEventClass;
     jclass mMediaEventClass;