From ad1291e7490b259ade603a8211c124ae748d3fcf Mon Sep 17 00:00:00 2001 From: Makoto Onuki Date: Thu, 30 May 2019 11:36:55 -0700 Subject: [PATCH] Avoid creating multiple death recipients for same observer Also do a WTF when the same process registers 1000 observers with the same observer instance. Fix: 133388082 Test: manual test: Launch leaky app and make sure WTF is raised Test: manual test: Open contacts app, close, and make sure the "Total number of unique observers" goes back Test: manual test: Open contacts app, kill it, and make sure the "Total number of unique observers" goes back Test: manual test: Open contacts app while contacts are syncing, and make sure the activity refreshes itself. Test: atest cts/tests/tests/content/src/android/content/cts/ContentResolverTest.java Test: atest FrameworksCoreTests:BinderDeathDispatcherTest Test: atest FrameworksServicesTests:com.android.server.content.ObserverNodeTest Change-Id: Id738ca66023c029b92db3d108d99a9fc240fe77c --- .../internal/os/BinderDeathDispatcher.java | 147 ++++++++++ .../com/android/internal/util/ArrayUtils.java | 4 + .../os/BinderDeathDispatcherTest.java | 265 ++++++++++++++++++ .../server/content/ContentService.java | 53 +++- .../server/content/ObserverNodeTest.java | 2 +- 5 files changed, 464 insertions(+), 7 deletions(-) create mode 100644 core/java/com/android/internal/os/BinderDeathDispatcher.java create mode 100644 core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java diff --git a/core/java/com/android/internal/os/BinderDeathDispatcher.java b/core/java/com/android/internal/os/BinderDeathDispatcher.java new file mode 100644 index 0000000000000..0c93f7f160e41 --- /dev/null +++ b/core/java/com/android/internal/os/BinderDeathDispatcher.java @@ -0,0 +1,147 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.internal.os; + +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.os.IBinder; +import android.os.IBinder.DeathRecipient; +import android.os.IInterface; +import android.os.RemoteException; +import android.util.ArrayMap; +import android.util.ArraySet; + +import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; + +import java.io.PrintWriter; + +/** + * Multiplexes multiple binder death recipients on the same binder objects, so that at the native + * level, we only need to keep track of one death recipient reference. This will help reduce the + * number of JNI strong references. + * + * test with: atest FrameworksCoreTests:BinderDeathDispatcherTest + */ +public class BinderDeathDispatcher { + private static final String TAG = "BinderDeathDispatcher"; + + private final Object mLock = new Object(); + + @GuardedBy("mLock") + private final ArrayMap mTargets = new ArrayMap<>(); + + @VisibleForTesting + class RecipientsInfo implements DeathRecipient { + final IBinder mTarget; + + /** + * Recipient list. If it's null, {@link #mTarget} has already died, but in that case + * this RecipientsInfo instance is removed from {@link #mTargets}. + */ + @GuardedBy("mLock") + @Nullable + ArraySet mRecipients = new ArraySet<>(); + + private RecipientsInfo(IBinder target) { + mTarget = target; + } + + @Override + public void binderDied() { + final ArraySet copy; + synchronized (mLock) { + copy = mRecipients; + mRecipients = null; + + // Also remove from the targets. + mTargets.remove(mTarget); + } + if (copy == null) { + return; + } + // Let's call it without holding the lock. + final int size = copy.size(); + for (int i = 0; i < size; i++) { + copy.valueAt(i).binderDied(); + } + } + } + + /** + * Add a {@code recipient} to the death recipient list on {@code target}. + * + * @return # of recipients in the recipient list, including {@code recipient}. Or, -1 + * if {@code target} is already dead, in which case recipient's + * {@link DeathRecipient#binderDied} won't be called. + */ + public int linkToDeath(@NonNull T target, @NonNull DeathRecipient recipient) { + final IBinder ib = target.asBinder(); + synchronized (mLock) { + RecipientsInfo info = mTargets.get(ib); + if (info == null) { + info = new RecipientsInfo(ib); + + // First recipient; need to link to death. + try { + ib.linkToDeath(info, 0); + } catch (RemoteException e) { + return -1; // Already dead. + } + mTargets.put(ib, info); + } + info.mRecipients.add(recipient); + return info.mRecipients.size(); + } + } + + public void unlinkToDeath(@NonNull T target, @NonNull DeathRecipient recipient) { + final IBinder ib = target.asBinder(); + + synchronized (mLock) { + final RecipientsInfo info = mTargets.get(ib); + if (info == null) { + return; + } + if (info.mRecipients.remove(recipient) && info.mRecipients.size() == 0) { + info.mTarget.unlinkToDeath(info, 0); + mTargets.remove(info.mTarget); + } + } + } + + public void dump(PrintWriter pw, String indent) { + synchronized (mLock) { + pw.print(indent); + pw.print("# of watched binders: "); + pw.println(mTargets.size()); + + pw.print(indent); + pw.print("# of death recipients: "); + int n = 0; + for (RecipientsInfo info : mTargets.values()) { + n += info.mRecipients.size(); + } + pw.println(n); + } + } + + @VisibleForTesting + public ArrayMap getTargetsForTest() { + return mTargets; + } +} diff --git a/core/java/com/android/internal/util/ArrayUtils.java b/core/java/com/android/internal/util/ArrayUtils.java index e8691fa5e23ef..b73ecd1974aa8 100644 --- a/core/java/com/android/internal/util/ArrayUtils.java +++ b/core/java/com/android/internal/util/ArrayUtils.java @@ -756,4 +756,8 @@ public class ArrayUtils { return String.valueOf(value); } } + + public static @Nullable T firstOrNull(T[] items) { + return items.length > 0 ? items[0] : null; + } } diff --git a/core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java b/core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java new file mode 100644 index 0000000000000..59148870fa3c4 --- /dev/null +++ b/core/tests/coretests/src/com/android/internal/os/BinderDeathDispatcherTest.java @@ -0,0 +1,265 @@ +/* + * Copyright (C) 2019 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.android.internal.os; + +import static com.google.common.truth.Truth.assertThat; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +import android.os.DeadObjectException; +import android.os.IBinder; +import android.os.IBinder.DeathRecipient; +import android.os.IInterface; +import android.os.Parcel; +import android.os.RemoteException; +import android.os.ResultReceiver; +import android.os.ShellCallback; + +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.io.FileDescriptor; + +import androidx.test.filters.SmallTest; +import androidx.test.runner.AndroidJUnit4; + +@SmallTest +@RunWith(AndroidJUnit4.class) +public class BinderDeathDispatcherTest { + private static class MyTarget implements IInterface, IBinder { + public boolean isAlive = true; + public DeathRecipient mRecipient; + + @Override + public String getInterfaceDescriptor() throws RemoteException { + return null; + } + + @Override + public boolean pingBinder() { + return false; + } + + @Override + public boolean isBinderAlive() { + return isAlive; + } + + @Override + public IInterface queryLocalInterface(String descriptor) { + return null; + } + + @Override + public void dump(FileDescriptor fd, String[] args) throws RemoteException { + + } + + @Override + public void dumpAsync(FileDescriptor fd, String[] args) throws RemoteException { + + } + + @Override + public void shellCommand(FileDescriptor in, FileDescriptor out, FileDescriptor err, + String[] args, ShellCallback shellCallback, ResultReceiver resultReceiver) + throws RemoteException { + + } + + @Override + public boolean transact(int code, Parcel data, Parcel reply, int flags) + throws RemoteException { + return false; + } + + @Override + public void linkToDeath(DeathRecipient recipient, int flags) throws RemoteException { + // In any situation, a single binder object should only have at most one death + // recipient. + assertThat(mRecipient).isNull(); + + if (!isAlive) { + throw new DeadObjectException(); + } + + mRecipient = recipient; + } + + @Override + public boolean unlinkToDeath(DeathRecipient recipient, int flags) { + if (!isAlive) { + return false; + } + assertThat(mRecipient).isSameAs(recipient); + mRecipient = null; + return true; + } + + @Override + public IBinder asBinder() { + return this; + } + + public void die() { + isAlive = false; + if (mRecipient != null) { + mRecipient.binderDied(); + } + mRecipient = null; + } + + public boolean hasDeathRecipient() { + return mRecipient != null; + } + } + + @Test + public void testRegisterAndUnregister() { + BinderDeathDispatcher d = new BinderDeathDispatcher<>(); + + MyTarget t1 = new MyTarget(); + MyTarget t2 = new MyTarget(); + MyTarget t3 = new MyTarget(); + + DeathRecipient r1 = mock(DeathRecipient.class); + DeathRecipient r2 = mock(DeathRecipient.class); + DeathRecipient r3 = mock(DeathRecipient.class); + DeathRecipient r4 = mock(DeathRecipient.class); + DeathRecipient r5 = mock(DeathRecipient.class); + + // Start hooking up. + + // Link 3 recipients to t1 -- only one real recipient will be set. + assertThat(d.linkToDeath(t1, r1)).isEqualTo(1); + assertThat(d.getTargetsForTest().size()).isEqualTo(1); + + assertThat(d.linkToDeath(t1, r2)).isEqualTo(2); + assertThat(d.linkToDeath(t1, r3)).isEqualTo(3); + assertThat(d.getTargetsForTest().size()).isEqualTo(1); + + // Unlink two -- the real recipient is still set. + d.unlinkToDeath(t1, r1); + d.unlinkToDeath(t1, r2); + + assertThat(t1.hasDeathRecipient()).isTrue(); + assertThat(d.getTargetsForTest().size()).isEqualTo(1); + + // Unlink the last one. The real recipient is also unlinked. + d.unlinkToDeath(t1, r3); + assertThat(t1.hasDeathRecipient()).isFalse(); + assertThat(d.getTargetsForTest().size()).isEqualTo(0); + + // Set recipients to t1, t2 and t3. t3 has two. + assertThat(d.linkToDeath(t1, r1)).isEqualTo(1); + assertThat(d.linkToDeath(t2, r1)).isEqualTo(1); + assertThat(d.linkToDeath(t3, r1)).isEqualTo(1); + assertThat(d.linkToDeath(t3, r2)).isEqualTo(2); + + + // They should all have a real recipient. + assertThat(t1.hasDeathRecipient()).isTrue(); + assertThat(t2.hasDeathRecipient()).isTrue(); + assertThat(t3.hasDeathRecipient()).isTrue(); + + assertThat(d.getTargetsForTest().size()).isEqualTo(3); + + // Unlink r1 from t3. t3 still has r2, so it should still have a real recipient. + d.unlinkToDeath(t3, r1); + assertThat(t1.hasDeathRecipient()).isTrue(); + assertThat(t2.hasDeathRecipient()).isTrue(); + assertThat(t3.hasDeathRecipient()).isTrue(); + assertThat(d.getTargetsForTest().size()).isEqualTo(3); + + // Unlink r2 from t3. Now t3 has no real recipient. + d.unlinkToDeath(t3, r2); + assertThat(t3.hasDeathRecipient()).isFalse(); + assertThat(d.getTargetsForTest().size()).isEqualTo(2); + } + + @Test + public void testRegisterAndKill() { + BinderDeathDispatcher d = new BinderDeathDispatcher<>(); + + MyTarget t1 = new MyTarget(); + MyTarget t2 = new MyTarget(); + MyTarget t3 = new MyTarget(); + + DeathRecipient r1 = mock(DeathRecipient.class); + DeathRecipient r2 = mock(DeathRecipient.class); + DeathRecipient r3 = mock(DeathRecipient.class); + DeathRecipient r4 = mock(DeathRecipient.class); + DeathRecipient r5 = mock(DeathRecipient.class); + + // Hook them up. + + d.linkToDeath(t1, r1); + d.linkToDeath(t1, r2); + d.linkToDeath(t1, r3); + + // r4 is linked then unlinked. It shouldn't be notified. + d.linkToDeath(t1, r4); + d.unlinkToDeath(t1, r4); + + d.linkToDeath(t2, r1); + + d.linkToDeath(t3, r3); + d.linkToDeath(t3, r5); + + assertThat(d.getTargetsForTest().size()).isEqualTo(3); + + // Kill the targets. + + t1.die(); + verify(r1, times(1)).binderDied(); + verify(r2, times(1)).binderDied(); + verify(r3, times(1)).binderDied(); + verify(r4, times(0)).binderDied(); + verify(r5, times(0)).binderDied(); + + assertThat(d.getTargetsForTest().size()).isEqualTo(2); + + reset(r1, r2, r3, r4, r5); + + t2.die(); + verify(r1, times(1)).binderDied(); + verify(r2, times(0)).binderDied(); + verify(r3, times(0)).binderDied(); + verify(r4, times(0)).binderDied(); + verify(r5, times(0)).binderDied(); + + assertThat(d.getTargetsForTest().size()).isEqualTo(1); + + reset(r1, r2, r3, r4, r5); + + t3.die(); + verify(r1, times(0)).binderDied(); + verify(r2, times(0)).binderDied(); + verify(r3, times(1)).binderDied(); + verify(r4, times(0)).binderDied(); + verify(r5, times(1)).binderDied(); + + assertThat(d.getTargetsForTest().size()).isEqualTo(0); + + // Try to register to a dead object -> should return -1. + assertThat(d.linkToDeath(t1, r1)).isEqualTo(-1); + + assertThat(d.getTargetsForTest().size()).isEqualTo(0); + } +} diff --git a/services/core/java/com/android/server/content/ContentService.java b/services/core/java/com/android/server/content/ContentService.java index 998ee1ed9ebee..7824a0ac31941 100644 --- a/services/core/java/com/android/server/content/ContentService.java +++ b/services/core/java/com/android/server/content/ContentService.java @@ -22,6 +22,7 @@ import android.annotation.Nullable; import android.annotation.RequiresPermission; import android.app.ActivityManager; import android.app.ActivityManagerInternal; +import android.app.AppGlobals; import android.app.AppOpsManager; import android.app.job.JobInfo; import android.content.BroadcastReceiver; @@ -56,6 +57,7 @@ import android.os.ShellCallback; import android.os.UserHandle; import android.text.TextUtils; import android.util.ArrayMap; +import android.util.ArraySet; import android.util.Log; import android.util.Pair; import android.util.Slog; @@ -63,6 +65,7 @@ import android.util.SparseArray; import android.util.SparseIntArray; import com.android.internal.annotations.GuardedBy; +import com.android.internal.os.BinderDeathDispatcher; import com.android.internal.util.ArrayUtils; import com.android.internal.util.DumpUtils; import com.android.internal.util.IndentingPrintWriter; @@ -83,6 +86,9 @@ public final class ContentService extends IContentService.Stub { static final String TAG = "ContentService"; static final boolean DEBUG = false; + /** Do a WTF if a single observer is registered more than this times. */ + private static final int TOO_MANY_OBSERVERS_THRESHOLD = 1000; + public static class Lifecycle extends SystemService { private ContentService mService; @@ -135,6 +141,12 @@ public final class ContentService extends IContentService.Stub { private SyncManager mSyncManager = null; private final Object mSyncManagerLock = new Object(); + private static final BinderDeathDispatcher sObserverDeathDispatcher = + new BinderDeathDispatcher<>(); + + @GuardedBy("sObserverLeakDetectedUid") + private static final ArraySet sObserverLeakDetectedUid = new ArraySet<>(0); + /** * Map from userId to providerPackageName to [clientPackageName, uri] to * value. This structure is carefully optimized to keep invalidation logic @@ -236,6 +248,13 @@ public final class ContentService extends IContentService.Stub { pw.println(); pw.print(" Total number of nodes: "); pw.println(counts[0]); pw.print(" Total number of observers: "); pw.println(counts[1]); + + sObserverDeathDispatcher.dump(pw, " "); + } + synchronized (sObserverLeakDetectedUid) { + pw.println(); + pw.print("Observer leaking UIDs: "); + pw.println(sObserverLeakDetectedUid.toString()); } synchronized (mCache) { @@ -1345,18 +1364,40 @@ public final class ContentService extends IContentService.Stub { private final Object observersLock; public ObserverEntry(IContentObserver o, boolean n, Object observersLock, - int _uid, int _pid, int _userHandle) { + int _uid, int _pid, int _userHandle, Uri uri) { this.observersLock = observersLock; observer = o; uid = _uid; pid = _pid; userHandle = _userHandle; notifyForDescendants = n; - try { - observer.asBinder().linkToDeath(this, 0); - } catch (RemoteException e) { + + final int entries = sObserverDeathDispatcher.linkToDeath(observer, this); + if (entries == -1) { binderDied(); + } else if (entries == TOO_MANY_OBSERVERS_THRESHOLD) { + boolean alreadyDetected; + + synchronized (sObserverLeakDetectedUid) { + alreadyDetected = sObserverLeakDetectedUid.contains(uid); + if (!alreadyDetected) { + sObserverLeakDetectedUid.add(uid); + } + } + if (!alreadyDetected) { + String caller = null; + try { + caller = ArrayUtils.firstOrNull(AppGlobals.getPackageManager() + .getPackagesForUid(uid)); + } catch (RemoteException ignore) { + } + Slog.wtf(TAG, "Observer registered too many times. Leak? cpid=" + pid + + " cuid=" + uid + + " cpkg=" + caller + + " url=" + uri); + } } + } @Override @@ -1454,7 +1495,7 @@ public final class ContentService extends IContentService.Stub { // If this is the leaf node add the observer if (index == countUriSegments(uri)) { mObservers.add(new ObserverEntry(observer, notifyForDescendants, observersLock, - uid, pid, userHandle)); + uid, pid, userHandle, uri)); return; } @@ -1498,7 +1539,7 @@ public final class ContentService extends IContentService.Stub { if (entry.observer.asBinder() == observerBinder) { mObservers.remove(i); // We no longer need to listen for death notifications. Remove it. - observerBinder.unlinkToDeath(entry, 0); + sObserverDeathDispatcher.unlinkToDeath(observer, entry); break; } } diff --git a/services/tests/servicestests/src/com/android/server/content/ObserverNodeTest.java b/services/tests/servicestests/src/com/android/server/content/ObserverNodeTest.java index 62b0ca805c1b6..891ca74a545f3 100644 --- a/services/tests/servicestests/src/com/android/server/content/ObserverNodeTest.java +++ b/services/tests/servicestests/src/com/android/server/content/ObserverNodeTest.java @@ -30,7 +30,7 @@ import com.android.server.content.ContentService.ObserverCall; import com.android.server.content.ContentService.ObserverNode; /** - * bit FrameworksServicesTests:com.android.server.content.ObserverNodeTest + * atest FrameworksServicesTests:com.android.server.content.ObserverNodeTest */ @SmallTest public class ObserverNodeTest extends AndroidTestCase {