Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 646cc266 authored by Jiyong Park's avatar Jiyong Park
Browse files

binder: fix death recipient leak for apps targeting >= V 

Before this change, when a death recipient is set on a binder proxy via
linkToDeath, a JNI global ref to the recipient object was created. That
global ref is cleared only when unlinkToDeath is explicitly called or
binderDied is notified.

In addition, since binderDied didn't give the IBinder which has died,
people has kept a strong reference to IBinder in the death recipient
object. Ex:

class FooHolder implements Binder.DeathRecipient {
    private IFoo mFoo;
    public FooHolder(IFoo foo) {
        mFoo = foo; // this!!!
        mFoo.linkToDeath(this, 0);
    }
    @Override
    public void binderDied() {
        // know that IFoo has died
    }
}

Unfortunately, this is prone to leak. Even if there's no reference to
FooHolder in your program, it is kept in memory due to the JNI global
ref as mentioned above. It means that you keep IFoo as well, and that
in turn keeps the binder service in the remote side. As a result,
binderDied will never be called (well, except when the server process
crashes).

The only way to release this object is calling unlinkToDeath explicitly
when you drop references to FooHolder. However, it's error prone and
keeping that practice is hard to be enforced.

Recently, the need for this pattern has become weaker as we introduced
binderDied(IBinder who). However, the API is quite new and its use is
not mandated. There still are many cases where this pattern is used.

This change is an attempt to fix the issue without having to touch the
existing uses. The idea is to change the way that death recipient
objects are strongly referenced - depending on whether you are targeting
Android V+ or not.

If targeting Android V+, the death recipient object is "weakly"
referenced from JNI. Instead, it is "strongly" referenced from the
BinderProxy object it is registered at. This means that if you drop
a BinderProxy object, you are dropping its death recipients as well,
unless you keep references to the recipients separately.

For apps targeting pre-V versions, we keep the JNI strong reference.

An important implication of this is that you won't get binderDied if you
drop BinderProxy object before the binder actually dies. This actually
is the documented behavior and has been the actual behavior "if you
don't use the FooHolder pattern mentioned above". I'd argue that this CL
fixes the undocumented incorrect behavior. However, we should be
conservative when making any behavioral change, thus we are hiding this
change behind the target SDK level.

Bug: 298374304
Test: atest BinderLeakTest BinderLeakTest_legacy

Change-Id: Ibb371f4de45530670d5f783f8ead8404c39381b4
parent 9fb33563

core/java/android/os/BinderProxy.java

+25 −3
Original line number Diff line number Diff line
@@ -32,7 +32,9 @@ import java.io.FileDescriptor; 
import java.lang.ref.WeakReference;

import java.util.ArrayList;

import java.util.Arrays;

import java.util.Collections;

import java.util.HashMap;

import java.util.List;

import java.util.Map;

import java.util.concurrent.ExecutorService;

import java.util.concurrent.Executors;
@@ -613,15 +615,35 @@ public final class BinderProxy implements IBinder { 
     */

    public native boolean transactNative(int code, Parcel data, Parcel reply,

            int flags) throws RemoteException;



    /* This list is to hold strong reference to the death recipients that are waiting for the death

     * of binder that this proxy references. Previously, the death recipients were strongy

     * referenced from JNI, but that can cause memory leak (b/298374304) when the application has a

     * strong reference from the death recipient to the proxy object. The JNI reference is now weak.

     * And this strong reference is to keep death recipients at least until the proxy is GC'ed. */

    private List<DeathRecipient> mDeathRecipients = Collections.synchronizedList(new ArrayList<>());



    /**

     * See {@link IBinder#linkToDeath(DeathRecipient, int)}

     */

    public native void linkToDeath(DeathRecipient recipient, int flags)

            throws RemoteException;

    public void linkToDeath(DeathRecipient recipient, int flags)

            throws RemoteException {

        linkToDeathNative(recipient, flags);

        mDeathRecipients.add(recipient);

    }



    /**

     * See {@link IBinder#unlinkToDeath}

     */

    public native boolean unlinkToDeath(DeathRecipient recipient, int flags);

    public boolean unlinkToDeath(DeathRecipient recipient, int flags) {

        mDeathRecipients.remove(recipient);

        return unlinkToDeathNative(recipient, flags);

    }



    private native void linkToDeathNative(DeathRecipient recipient, int flags)

            throws RemoteException;



    private native boolean unlinkToDeathNative(DeathRecipient recipient, int flags);



    /**

     * Perform a dump on the remote object

core/jni/android_util_Binder.cpp

+99 −36
Original line number Diff line number Diff line
@@ -17,19 +17,8 @@ 
#define LOG_TAG "JavaBinder"

//#define LOG_NDEBUG 0



#include "android_os_Parcel.h"

#include "android_util_Binder.h"



#include <atomic>

#include <fcntl.h>

#include <inttypes.h>

#include <mutex>

#include <stdio.h>

#include <string>

#include <sys/stat.h>

#include <sys/types.h>

#include <unistd.h>



#include <android-base/stringprintf.h>

#include <binder/BpBinder.h>

#include <binder/IInterface.h>
@@ -40,7 +29,16 @@ 
#include <binder/Stability.h>

#include <binderthreadstate/CallerUtils.h>

#include <cutils/atomic.h>

#include <fcntl.h>

#include <inttypes.h>

#include <log/log.h>

#include <nativehelper/JNIHelp.h>

#include <nativehelper/ScopedLocalRef.h>

#include <nativehelper/ScopedUtfChars.h>

#include <stdio.h>

#include <sys/stat.h>

#include <sys/types.h>

#include <unistd.h>

#include <utils/KeyedVector.h>

#include <utils/List.h>

#include <utils/Log.h>
@@ -48,10 +46,11 @@ 
#include <utils/SystemClock.h>

#include <utils/threads.h>



#include <nativehelper/JNIHelp.h>

#include <nativehelper/ScopedLocalRef.h>

#include <nativehelper/ScopedUtfChars.h>

#include <atomic>

#include <mutex>

#include <string>



#include "android_os_Parcel.h"

#include "core_jni_helpers.h"



//#undef ALOGV
@@ -557,14 +556,43 @@ public: 
};



// ----------------------------------------------------------------------------

#if __BIONIC__

#include <android/api-level.h>

static bool target_sdk_is_at_least_vic() {

    return android_get_application_target_sdk_version() >= __ANDROID_API_V__;

}

#else

static constexpr bool target_sdk_is_at_least_vic() {

    // If not built for Android (i.e. glibc host), follow the latest behavior as there's no compat

    // requirement there.

    return true;

}

#endif // __BIONIC__



class JavaDeathRecipient : public IBinder::DeathRecipient

{

public:

    JavaDeathRecipient(JNIEnv* env, jobject object, const sp<DeathRecipientList>& list)

        : mVM(jnienv_to_javavm(env)), mObject(env->NewGlobalRef(object)),

          mObjectWeak(NULL), mList(list)

    {

          : mVM(jnienv_to_javavm(env)), mObject(NULL), mObjectWeak(NULL), mList(list) {

        // b/298374304: For apps targeting Android V or beyond, we no longer hold the global JNI ref

        // to the death recipient objects. This is to prevent the memory leak which can happen when

        // the death recipient object internally has a strong reference to the proxy object. Under

        // the old behavior, you were unable to kill the binder service by dropping all references

        // to the proxy object - because it is still strong referenced from JNI (here). The only way

        // to cut the strong reference was to call unlinkDeath(), but it was easy to forget.

        //

        // Now, the strong reference to the death recipient is held in the Java-side proxy object.

        // See BinderProxy.mDeathRecipients. From JNI, only the weak reference is kept. An

        // implication of this is that you may not receive binderDied() if you drop all references

        // to the proxy object before the service dies. This should be okay for most cases because

        // you normally are not interested in the death of a binder service which you don't have any

        // reference to. If however you want to get binderDied() regardless of the proxy object's

        // lifecycle, keep a strong reference to the death recipient object by yourself.

        if (target_sdk_is_at_least_vic()) {

            mObjectWeak = env->NewWeakGlobalRef(object);

        } else {

            mObject = env->NewGlobalRef(object);

        }

        // These objects manage their own lifetimes so are responsible for final bookkeeping.

        // The list holds a strong reference to this object.

        LOGDEATH("Adding JDR %p to DRL %p", this, list.get());
@@ -577,26 +605,49 @@ public: 
    void binderDied(const wp<IBinder>& who)

    {

        LOGDEATH("Receiving binderDied() on JavaDeathRecipient %p\n", this);

        if (mObject != NULL) {

        if (mObject == NULL && mObjectWeak == NULL) {

            return;

        }

        JNIEnv* env = javavm_to_jnienv(mVM);

        ScopedLocalRef<jobject> jBinderProxy(env, javaObjectForIBinder(env, who.promote()));

            env->CallStaticVoidMethod(gBinderProxyOffsets.mClass,

                                      gBinderProxyOffsets.mSendDeathNotice, mObject,

                                      jBinderProxy.get());



        // Hold a local reference to the recipient. This may fail if the recipient is weakly

        // referenced, in which case we can't deliver the death notice.

        ScopedLocalRef<jobject> jRecipient(env,

                                           env->NewLocalRef(mObject != NULL ? mObject

                                                                            : mObjectWeak));

        if (jRecipient.get() == NULL) {

            ALOGW("Binder died, but death recipient is already garbage collected. If your target "

                  "sdk level is at or above 35, this can happen when you dropped all references to "

                  "the binder service before it died. If you want to get a death notice for a "

                  "binder service which you have no reference to, keep a strong reference to the "

                  "death recipient by yourself.");

            return;

        }



        if (mFired) {

            ALOGW("Received multiple death notices for the same binder object. Binder driver bug?");

            return;

        }

        mFired = true;



        env->CallStaticVoidMethod(gBinderProxyOffsets.mClass, gBinderProxyOffsets.mSendDeathNotice,

                                  jRecipient.get(), jBinderProxy.get());

        if (env->ExceptionCheck()) {

            jthrowable excep = env->ExceptionOccurred();

            binder_report_exception(env, excep,

                                    "*** Uncaught exception returned from death notification!");

        }



            // Serialize with our containing DeathRecipientList so that we can't

            // delete the global ref on mObject while the list is being iterated.

        // Demote from strong ref (if exists) to weak after binderDied() has been delivered, to

        // allow the DeathRecipient and BinderProxy to be GC'd if no longer needed. Do this in sync

        // with our containing DeathRecipientList so that we can't delete the global ref on mObject

        // while the list is being iterated.

        if (mObject != NULL) {

            sp<DeathRecipientList> list = mList.promote();

            if (list != NULL) {

                AutoMutex _l(list->lock());



                // Demote from strong ref to weak after binderDied() has been delivered,

                // to allow the DeathRecipient and BinderProxy to be GC'd if no longer needed.

                mObjectWeak = env->NewWeakGlobalRef(mObject);

                env->DeleteGlobalRef(mObject);

                mObject = NULL;
@@ -663,9 +714,19 @@ protected: 


private:

    JavaVM* const mVM;

    jobject mObject;  // Initial strong ref to Java-side DeathRecipient. Cleared on binderDied().

    jweak mObjectWeak; // Weak ref to the same Java-side DeathRecipient after binderDied().



    // If target sdk version < 35, the Java-side DeathRecipient is strongly referenced from mObject

    // upon linkToDeath() and then after binderDied() is called, the strong reference is demoted to

    // a weak reference (mObjectWeak).

    // If target sdk version >= 35, the strong reference is never made here (i.e. mObject == NULL

    // always). Instead, the strong reference to the Java-side DeathRecipient is made in

    // BinderProxy.mDeathRecipients. In the native world, only the weak reference is kept.

    jobject mObject;

    jweak mObjectWeak;

    wp<DeathRecipientList> mList;



    // Whether binderDied was called or not.

    bool mFired = false;

};



// ----------------------------------------------------------------------------
@@ -1452,17 +1513,19 @@ static jobject android_os_BinderProxy_getExtension(JNIEnv* env, jobject obj) { 


// ----------------------------------------------------------------------------



// clang-format off

static const JNINativeMethod gBinderProxyMethods[] = {

     /* name, signature, funcPtr */

    {"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},

    {"linkToDeath",         "(Landroid/os/IBinder$DeathRecipient;I)V", (void*)android_os_BinderProxy_linkToDeath},

    {"unlinkToDeath",       "(Landroid/os/IBinder$DeathRecipient;I)Z", (void*)android_os_BinderProxy_unlinkToDeath},

    {"linkToDeathNative",   "(Landroid/os/IBinder$DeathRecipient;I)V", (void*)android_os_BinderProxy_linkToDeath},

    {"unlinkToDeathNative", "(Landroid/os/IBinder$DeathRecipient;I)Z", (void*)android_os_BinderProxy_unlinkToDeath},

    {"getNativeFinalizer",  "()J", (void*)android_os_BinderProxy_getNativeFinalizer},

    {"getExtension",        "()Landroid/os/IBinder;", (void*)android_os_BinderProxy_getExtension},

};

// clang-format on



const char* const kBinderProxyPathName = "android/os/BinderProxy";

tests/BinderLeakTest/Android.bp

0 → 100644
+40 −0
Original line number Diff line number Diff line 
package {

    // See: http://go/android-license-faq

    // A large-scale-change added 'default_applicable_licenses' to import

    // all of the 'license_kinds' from "frameworks_base_license"

    // to get the below license kinds:

    //   SPDX-license-identifier-Apache-2.0

    default_applicable_licenses: ["frameworks_base_license"],

}



filegroup {

    name: "binder_leak_test_aidl",

    srcs: ["**/*.aidl"],

    path: "aidl",

}



java_defaults {

    name: "BinderTest.defaults",

    srcs: [

        "**/*.java",

        ":binder_leak_test_aidl",

    ],

    static_libs: [

        "androidx.test.ext.junit",

        "androidx.test.rules",

        "androidx.test.runner",

    ],

}



// Built with target_sdk_version: current

android_test {

    name: "BinderLeakTest",

    defaults: ["BinderTest.defaults"],

}



// Built with target_sdk_version: 33

android_test {

    name: "BinderLeakTest_legacy",

    defaults: ["BinderTest.defaults"],

    manifest: "AndroidManifest_legacy.xml",

}

tests/BinderLeakTest/AndroidManifest.xml

0 → 100644
+17 −0
Original line number Diff line number Diff line 
<?xml version="1.0" encoding="utf-8"?>

<manifest xmlns:android="http://schemas.android.com/apk/res/android"

          package="com.android.test.binder">

    <application>

        <service

            android:name=".MyService"

            android:enabled="true"

            android:exported="true"

            android:process=":service">

        </service>

    </application>



    <instrumentation android:name="androidx.test.runner.AndroidJUnitRunner"

        android:targetPackage="com.android.test.binder"

        android:label="Binder leak test">

    </instrumentation>

</manifest>

tests/BinderLeakTest/AndroidManifest_legacy.xml

0 → 100644
+20 −0
Original line number Diff line number Diff line 
<?xml version="1.0" encoding="utf-8"?>

<manifest xmlns:android="http://schemas.android.com/apk/res/android"

          package="com.android.test.binder">

    <uses-sdk android:minSdkVersion="33"

          android:targetSdkVersion="33"

          android:maxSdkVersion="33" />

    <application>

        <service

            android:name=".MyService"

            android:enabled="true"

            android:exported="true"

            android:process=":service">

        </service>

    </application>



    <instrumentation android:name="androidx.test.runner.AndroidJUnitRunner"

        android:targetPackage="com.android.test.binder"

        android:label="Binder leak test">

    </instrumentation>

</manifest>