From 5df3f27df376bf275c50ac6ed8ac8c9251ab50c3 Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Wed, 23 Mar 2022 14:27:45 +0800 Subject: [PATCH] Drop task snapshot with invalid hw buffer The notifyTaskSnapshotChanged may not be always called after Binder.clearCallingIdentity. And because there was commit 67b0902 that avoids keeping unnecessary buffer in remote processes, it may mis-close the buffer in system server if the calling pid isn't cleared on binder thread. e.g. there are several suspects from ActivityTaskManagerInternal Though it might be the responsibility of the caller to clear binder identity before calling internal interfaces. Instead of adding clearCallingIdentity every where: 1. Explicitly set the listener as local to skip closing the buffer so it is independent of which thread is calling. 2. Protect the persister when using the buffer. Bug: 192143094 Bug: 220659717 Bug: 223950159 Bug: 225794452 Test: atest TaskSnapshotPersisterLoaderTest Change-Id: I94e8e706e56a73fffe8189023bcef5efd797a24c --- core/java/android/app/TaskStackListener.java | 12 +++++++++--- .../wm/TaskChangeNotificationController.java | 4 ++++ .../server/wm/TaskSnapshotController.java | 7 ++++++- .../server/wm/TaskSnapshotPersister.java | 4 ++++ .../android/server/wm/WindowManagerService.java | 17 +++++++++++------ .../wm/TaskSnapshotPersisterLoaderTest.java | 8 +++++++- .../wm/TaskSnapshotPersisterTestBase.java | 3 +-- .../com/android/server/wm/WindowTestsBase.java | 2 +- 8 files changed, 43 insertions(+), 14 deletions(-) diff --git a/core/java/android/app/TaskStackListener.java b/core/java/android/app/TaskStackListener.java index f523a7d297138..83fe29fa2ba67 100644 --- a/core/java/android/app/TaskStackListener.java +++ b/core/java/android/app/TaskStackListener.java @@ -19,7 +19,6 @@ package android.app; import android.app.ActivityManager.RunningTaskInfo; import android.compat.annotation.UnsupportedAppUsage; import android.content.ComponentName; -import android.os.Binder; import android.os.Build; import android.os.RemoteException; import android.window.TaskSnapshot; @@ -32,10 +31,18 @@ import android.window.TaskSnapshot; */ public abstract class TaskStackListener extends ITaskStackListener.Stub { + /** Whether this listener and the callback dispatcher are in different processes. */ + private boolean mIsRemote = true; + @UnsupportedAppUsage public TaskStackListener() { } + /** Indicates that this listener lives in system server. */ + public void setIsLocal() { + mIsRemote = false; + } + @Override @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public void onTaskStackChanged() throws RemoteException { @@ -154,8 +161,7 @@ public abstract class TaskStackListener extends ITaskStackListener.Stub { @Override @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.R, trackingBug = 170729553) public void onTaskSnapshotChanged(int taskId, TaskSnapshot snapshot) throws RemoteException { - if (Binder.getCallingPid() != android.os.Process.myPid() - && snapshot != null && snapshot.getHardwareBuffer() != null) { + if (mIsRemote && snapshot != null && snapshot.getHardwareBuffer() != null) { // Preemptively clear any reference to the buffer snapshot.getHardwareBuffer().close(); } diff --git a/services/core/java/com/android/server/wm/TaskChangeNotificationController.java b/services/core/java/com/android/server/wm/TaskChangeNotificationController.java index 3b7927470e648..61963c48a14c2 100644 --- a/services/core/java/com/android/server/wm/TaskChangeNotificationController.java +++ b/services/core/java/com/android/server/wm/TaskChangeNotificationController.java @@ -20,6 +20,7 @@ import android.app.ActivityManager; import android.app.ActivityManager.RunningTaskInfo; import android.app.ITaskStackListener; import android.app.TaskInfo; +import android.app.TaskStackListener; import android.content.ComponentName; import android.os.Binder; import android.os.Handler; @@ -286,6 +287,9 @@ class TaskChangeNotificationController { if (listener instanceof Binder) { synchronized (mLocalTaskStackListeners) { if (!mLocalTaskStackListeners.contains(listener)) { + if (listener instanceof TaskStackListener) { + ((TaskStackListener) listener).setIsLocal(); + } mLocalTaskStackListeners.add(listener); } } diff --git a/services/core/java/com/android/server/wm/TaskSnapshotController.java b/services/core/java/com/android/server/wm/TaskSnapshotController.java index cde99271b0aeb..814656db9fa0e 100644 --- a/services/core/java/com/android/server/wm/TaskSnapshotController.java +++ b/services/core/java/com/android/server/wm/TaskSnapshotController.java @@ -480,12 +480,17 @@ class TaskSnapshotController { } final HardwareBuffer buffer = screenshotBuffer == null ? null : screenshotBuffer.getHardwareBuffer(); - if (buffer == null || buffer.getWidth() <= 1 || buffer.getHeight() <= 1) { + if (isInvalidHardwareBuffer(buffer)) { return null; } return screenshotBuffer; } + static boolean isInvalidHardwareBuffer(HardwareBuffer buffer) { + return buffer == null || buffer.isClosed() // This must be checked before getting size. + || buffer.getWidth() <= 1 || buffer.getHeight() <= 1; + } + @Nullable TaskSnapshot snapshotTask(Task task) { return snapshotTask(task, PixelFormat.UNKNOWN); diff --git a/services/core/java/com/android/server/wm/TaskSnapshotPersister.java b/services/core/java/com/android/server/wm/TaskSnapshotPersister.java index 9fbcd7cc5d840..03098e3eaca98 100644 --- a/services/core/java/com/android/server/wm/TaskSnapshotPersister.java +++ b/services/core/java/com/android/server/wm/TaskSnapshotPersister.java @@ -407,6 +407,10 @@ class TaskSnapshotPersister { } boolean writeBuffer() { + if (TaskSnapshotController.isInvalidHardwareBuffer(mSnapshot.getHardwareBuffer())) { + Slog.e(TAG, "Invalid task snapshot hw buffer, taskId=" + mTaskId); + return false; + } final Bitmap bitmap = Bitmap.wrapHardwareBuffer( mSnapshot.getHardwareBuffer(), mSnapshot.getColorSpace()); if (bitmap == null) { diff --git a/services/core/java/com/android/server/wm/WindowManagerService.java b/services/core/java/com/android/server/wm/WindowManagerService.java index 41215014f8594..92864b6e3fd72 100644 --- a/services/core/java/com/android/server/wm/WindowManagerService.java +++ b/services/core/java/com/android/server/wm/WindowManagerService.java @@ -9045,13 +9045,18 @@ public class WindowManagerService extends IWindowManager.Stub } TaskSnapshot taskSnapshot; - synchronized (mGlobalLock) { - Task task = mRoot.anyTaskForId(taskId, MATCH_ATTACHED_TASK_OR_RECENT_TASKS); - if (task == null) { - throw new IllegalArgumentException( - "Failed to find matching task for taskId=" + taskId); + final long token = Binder.clearCallingIdentity(); + try { + synchronized (mGlobalLock) { + Task task = mRoot.anyTaskForId(taskId, MATCH_ATTACHED_TASK_OR_RECENT_TASKS); + if (task == null) { + throw new IllegalArgumentException( + "Failed to find matching task for taskId=" + taskId); + } + taskSnapshot = mTaskSnapshotController.captureTaskSnapshot(task, false); } - taskSnapshot = mTaskSnapshotController.captureTaskSnapshot(task, false); + } finally { + Binder.restoreCallingIdentity(token); } if (taskSnapshot == null || taskSnapshot.getHardwareBuffer() == null) { diff --git a/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterLoaderTest.java b/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterLoaderTest.java index 70d71bc627c0d..7409d62e175e3 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterLoaderTest.java +++ b/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterLoaderTest.java @@ -31,13 +31,13 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; import android.app.ActivityManager; -import android.window.TaskSnapshot; import android.content.res.Configuration; import android.graphics.Rect; import android.os.SystemClock; import android.platform.test.annotations.Presubmit; import android.util.ArraySet; import android.view.Surface; +import android.window.TaskSnapshot; import androidx.test.filters.MediumTest; @@ -83,6 +83,12 @@ public class TaskSnapshotPersisterLoaderTest extends TaskSnapshotPersisterTestBa assertEquals(TEST_INSETS, snapshot.getContentInsets()); assertNotNull(snapshot.getSnapshot()); assertEquals(Configuration.ORIENTATION_PORTRAIT, snapshot.getOrientation()); + + snapshot.getHardwareBuffer().close(); + mPersister.persistSnapshot(1, mTestUserId, snapshot); + mPersister.waitForQueueEmpty(); + assertTrueForFiles(files, file -> !file.exists(), + " snapshot files must be removed by invalid buffer"); } @Test diff --git a/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterTestBase.java b/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterTestBase.java index 437d418856845..f71ed2f08a22d 100644 --- a/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterTestBase.java +++ b/services/tests/wmtests/src/com/android/server/wm/TaskSnapshotPersisterTestBase.java @@ -131,8 +131,7 @@ class TaskSnapshotPersisterTestBase extends WindowTestsBase { } TaskSnapshot createSnapshot() { - return new TaskSnapshotBuilder() - .build(); + return new TaskSnapshotBuilder().setTopActivityComponent(getUniqueComponentName()).build(); } protected static void assertTrueForFiles(File[] files, Predicate predicate, diff --git a/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java b/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java index 6e0d8549defc7..bcab4a5cde279 100644 --- a/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java +++ b/services/tests/wmtests/src/com/android/server/wm/WindowTestsBase.java @@ -903,7 +903,7 @@ class WindowTestsBase extends SystemServiceTestsBase { doReturn(100).when(hardwareBuffer).getHeight(); } - private static ComponentName getUniqueComponentName() { + static ComponentName getUniqueComponentName() { return ComponentName.createRelative(DEFAULT_COMPONENT_PACKAGE_NAME, DEFAULT_COMPONENT_CLASS_NAME + sCurrentActivityId++); }