Merge changes from topic "death_recipient_flag" into main am: e916262ee7 am:... (2f6385cb) · Commits · e / os / android_frameworks_base

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.bp

+19 −1

Original line number	Diff line number	Diff line
		@@ -15,7 +15,19 @@ license {
		],
		}

		cc_library_shared {
		soong_config_module_type {
		name: "cc_library_shared_for_libandroid_runtime",
		module_type: "cc_library_shared",
		config_namespace: "ANDROID",
		bool_variables: [
		"release_binder_death_recipient_weak_from_jni",
		],
		properties: [
		"cflags",
		],
		}

		cc_library_shared_for_libandroid_runtime {
		name: "libandroid_runtime",
		host_supported: true,
		cflags: [
		@@ -46,6 +58,12 @@ cc_library_shared {
		},
		},

		soong_config_variables: {
		release_binder_death_recipient_weak_from_jni: {
		cflags: ["-DBINDER_DEATH_RECIPIENT_WEAK_FROM_JNI"],
		},
		},

		cpp_std: "gnu++20",

		srcs: [

core/jni/android_util_Binder.cpp

+104 −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,48 @@ public:
		};

		// ----------------------------------------------------------------------------
		#ifdef BINDER_DEATH_RECIPIENT_WEAK_FROM_JNI
		#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__
		#endif // BINDER_DEATH_RECIPIENT_WEAK_FROM_JNI

		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.
		#ifdef BINDER_DEATH_RECIPIENT_WEAK_FROM_JNI
		if (target_sdk_is_at_least_vic()) {
		mObjectWeak = env->NewWeakGlobalRef(object);
		} else
		#endif
		{
		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 +610,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 +719,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 +1518,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>