Merge "binder: race condition by parcel finalize" am: 52f933552a am: 5cb323c59e

Original change: https://android-review.googlesource.com/c/platform/frameworks/base/+/1553275

MUST ONLY BE SUBMITTED BY AUTOMERGER

Change-Id: Ic3edc89022f831d7d0301bdd4734e45b62c49649

4 files changed, 33 insertions, 8 deletions

diff --git a/core/java/android/os/BinderProxy.java b/core/java/android/os/BinderProxy.java
index 0185ba444ca4..16d041ac60f2 100644
--- a/core/java/android/os/BinderProxy.java
+++ b/core/java/android/os/BinderProxy.java
@@ -547,7 +547,8 @@ public final class BinderProxy implements IBinder {
         }
 
         try {
-            return transactNative(code, data, reply, flags);
+            boolean replyOwnsNative = (reply == null) ? false : reply.ownsNativeParcelObject();
+            return transactNative(code, data, reply, replyOwnsNative, flags);
         } finally {
             AppOpsManager.resumeNotedAppOpsCollection(prevCollection);
 
@@ -572,7 +573,7 @@ public final class BinderProxy implements IBinder {
      * Native implementation of transact() for proxies
      */
     public native boolean transactNative(int code, Parcel data, Parcel reply,
-            int flags) throws RemoteException;
+            boolean replyOwnsNativeParcelObject, int flags) throws RemoteException;
     /**
      * See {@link IBinder#linkToDeath(DeathRecipient, int)}
      */
diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java
index 6acdcc4722d2..f0a99ed5c2fd 100644
--- a/core/java/android/os/Parcel.java
+++ b/core/java/android/os/Parcel.java
@@ -3691,4 +3691,9 @@ public final class Parcel {
     public long getBlobAshmemSize() {
         return nativeGetBlobAshmemSize(mNativePtr);
     }
+
+    /** @hide */
+    /*package*/ boolean ownsNativeParcelObject() {
+        return mOwnsNativeParcelObject;
+    }
 }
diff --git a/core/jni/android_os_Parcel.cpp b/core/jni/android_os_Parcel.cpp
index 241570a7f9d3..0fdab722dd05 100644
--- a/core/jni/android_os_Parcel.cpp
+++ b/core/jni/android_os_Parcel.cpp
@@ -36,6 +36,7 @@
 #include <utils/List.h>
 #include <utils/KeyedVector.h>
 #include <binder/Parcel.h>
+#include <binder/ParcelRef.h>
 #include <binder/ProcessState.h>
 #include <binder/IServiceManager.h>
 #include <utils/threads.h>
@@ -529,8 +530,9 @@ static jobject android_os_Parcel_readFileDescriptor(JNIEnv* env, jclass clazz, j
 
 static jlong android_os_Parcel_create(JNIEnv* env, jclass clazz)
 {
-    Parcel* parcel = new Parcel();
-    return reinterpret_cast<jlong>(parcel);
+    sp<ParcelRef> parcelRef = ParcelRef::create();
+    parcelRef->incStrong(reinterpret_cast<const void*>(android_os_Parcel_create));
+    return reinterpret_cast<jlong>(static_cast<Parcel *>(parcelRef.get()));
 }
 
 static jlong android_os_Parcel_freeBuffer(JNIEnv* env, jclass clazz, jlong nativePtr)
@@ -545,8 +547,8 @@ static jlong android_os_Parcel_freeBuffer(JNIEnv* env, jclass clazz, jlong nativ
 
 static void android_os_Parcel_destroy(JNIEnv* env, jclass clazz, jlong nativePtr)
 {
-    Parcel* parcel = reinterpret_cast<Parcel*>(nativePtr);
-    delete parcel;
+    ParcelRef* derivative = static_cast<ParcelRef*>(reinterpret_cast<Parcel*>(nativePtr));
+    derivative->decStrong(reinterpret_cast<const void*>(android_os_Parcel_create));
 }
 
 static jbyteArray android_os_Parcel_marshall(JNIEnv* env, jclass clazz, jlong nativePtr)
diff --git a/core/jni/android_util_Binder.cpp b/core/jni/android_util_Binder.cpp
index 581dc0848a28..f71b42c4793f 100644
--- a/core/jni/android_util_Binder.cpp
+++ b/core/jni/android_util_Binder.cpp
@@ -35,6 +35,7 @@
 #include <binder/IPCThreadState.h>
 #include <binder/IServiceManager.h>
 #include <binder/Parcel.h>
+#include <binder/ParcelRef.h>
 #include <binder/ProcessState.h>
 #include <binder/Stability.h>
 #include <binderthreadstate/CallerUtils.h>
@@ -1374,7 +1375,8 @@ static bool should_time_binder_calls() {
 }
 
 static jboolean android_os_BinderProxy_transact(JNIEnv* env, jobject obj,
-        jint code, jobject dataObj, jobject replyObj, jint flags) // throws RemoteException
+        jint code, jobject dataObj, jobject replyObj, jboolean replyObjOwnsNativeParcel,
+        jint flags) // throws RemoteException
 {
     if (dataObj == NULL) {
         jniThrowNullPointerException(env, NULL);
@@ -1416,6 +1418,21 @@ static jboolean android_os_BinderProxy_transact(JNIEnv* env, jobject obj,
     status_t err = target->transact(code, *data, reply, flags);
     //if (reply) printf("Transact from Java code to %p received: ", target); reply->print();
 
+    if (reply) {
+        if (replyObjOwnsNativeParcel) {
+            // as per Parcel java class constructor, here, "reply" MUST be a "ParcelRef"
+            // only for Parcel that contained Binder objects
+            if (reply->objectsCount() > 0) {
+                IPCThreadState::self()->createTransactionReference(static_cast<ParcelRef*>(reply));
+            }
+        } else {
+            // as per Parcel.java, if Parcel java object NOT owning native Parcel object, it will
+            // NOT destroy the native Parcel object upon GC(finalize()), so, there will be no race
+            // condtion in this case. Please refer to the java class methods: Parcel.finalize(),
+            // Parcel.destroy().
+        }
+    }
+
     if (kEnableBinderSample) {
         if (time_binder_calls) {
             conditionally_log_binder_call(start_millis, target, code);
@@ -1542,7 +1559,7 @@ static const JNINativeMethod gBinderProxyMethods[] = {
     {"pingBinder",          "()Z", (void*)android_os_BinderProxy_pingBinder},
     {"isBinderAlive",       "()Z", (void*)android_os_BinderProxy_isBinderAlive},
     {"getInterfaceDescriptor", "()Ljava/lang/String;", (void*)android_os_BinderProxy_getInterfaceDescriptor},
-    {"transactNative",      "(ILandroid/os/Parcel;Landroid/os/Parcel;I)Z", (void*)android_os_BinderProxy_transact},
+    {"transactNative",      "(ILandroid/os/Parcel;Landroid/os/Parcel;ZI)Z", (void*)android_os_BinderProxy_transact},
     {"linkToDeath",         "(Landroid/os/IBinder$DeathRecipient;I)V", (void*)android_os_BinderProxy_linkToDeath},
     {"unlinkToDeath",       "(Landroid/os/IBinder$DeathRecipient;I)Z", (void*)android_os_BinderProxy_unlinkToDeath},
     {"getNativeFinalizer",  "()J", (void*)android_os_BinderProxy_getNativeFinalizer},