From 6118857c97474f3e3e2ab6bc59c6122afb1dd727 Mon Sep 17 00:00:00 2001 From: Paul Hadfield Date: Tue, 7 Dec 2021 16:39:32 +0000 Subject: [PATCH] Trigger unbindBackupAgent if app dies during backup This is to fix a bug where, if an app crashes in restricted mode, when the user restarts the app, it would be launched still in restricted mode. That happens because ActivityManager still has a BackupRecord indicating that the app should be in restricted mode, so next time the app launches, attachApplicationLocked() calls IApplicationThread.bindApplication() with the restricted mode flag set. To avoid this we must complete the clean up actions done when backup agents report errors. UserBackupManagerService has a call-back method agentDisconnected() that is notified if an agent dies. In that method we call handleCancel() on all outstanding operations that are linked to the package name, according to OperationStorage. The result is a call to the Activity Manager's unbindBackupAgent() which then removes the obsolete BackupRecord. Two new tests for agentDisconnected() verify that appropriate calls are made to OperationStore. BUG: 161089758 Test: atest FrameworksServicesTests:com.android.server.backup atest BackupFrameworksServicesRoboTests atest CtsBackupHostTestCases atest CtsBackupTestCases atest GtsBackupTestCase Also, manual testing with an app that deliberately dies during full backup: the lifecycle operation storage is used to cancel the backup; the app restarts in normal and not restricted mode. Change-Id: Ic4cf3875d59e3580b026340e884093f3aa1b09b9 --- .../backup/UserBackupManagerService.java | 15 ++++++++- .../PerformFullTransportBackupTask.java | 24 +++++++++----- .../backup/UserBackupManagerServiceTest.java | 33 +++++++++++++++++++ 3 files changed, 62 insertions(+), 10 deletions(-) diff --git a/services/backup/java/com/android/server/backup/UserBackupManagerService.java b/services/backup/java/com/android/server/backup/UserBackupManagerService.java index 81d6381a22735..f8da03539ae6f 100644 --- a/services/backup/java/com/android/server/backup/UserBackupManagerService.java +++ b/services/backup/java/com/android/server/backup/UserBackupManagerService.java @@ -3718,7 +3718,6 @@ public class UserBackupManagerService { * only be called from the {@link ActivityManager}. */ public void agentDisconnected(String packageName) { - // TODO: handle backup being interrupted synchronized (mAgentConnectLock) { if (Binder.getCallingUid() == Process.SYSTEM_UID) { mConnectedAgent = null; @@ -3732,6 +3731,20 @@ public class UserBackupManagerService { + Binder.getCallingUid() + " claiming agent disconnected")); } + Slog.w(TAG, "agentDisconnected: the backup agent for " + packageName + + " died: cancel current operations"); + + // handleCancel() causes the PerformFullTransportBackupTask to go on to + // tearDownAgentAndKill: that will unbindBackupAgent in the Activity Manager, so + // that the package being backed up doesn't get stuck in restricted mode until the + // backup time-out elapses. + for (int token : mOperationStorage.operationTokensForPackage(packageName)) { + if (MORE_DEBUG) { + Slog.d(TAG, "agentDisconnected: will handleCancel(all) for token:" + + Integer.toHexString(token)); + } + handleCancel(token, true /* cancelAll */); + } mAgentConnectLock.notifyAll(); } } diff --git a/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java b/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java index 0ca77d1552aa7..e74a3b97edd17 100644 --- a/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java +++ b/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java @@ -56,12 +56,15 @@ import com.android.server.backup.utils.BackupEligibilityRules; import com.android.server.backup.utils.BackupManagerMonitorUtils; import com.android.server.backup.utils.BackupObserverUtils; +import com.google.android.collect.Sets; + import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Objects; +import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; @@ -195,8 +198,6 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba return; } - registerTask(); - for (String pkg : whichPackages) { try { PackageManager pm = backupManagerService.getPackageManager(); @@ -262,11 +263,15 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba } mPackages = backupManagerService.filterUserFacingPackages(mPackages); - } - private void registerTask() { + Set packageNames = Sets.newHashSet(); + for (PackageInfo pkgInfo : mPackages) { + packageNames.add(pkgInfo.packageName); + } + Slog.d(TAG, "backupmanager pftbt token=" + Integer.toHexString(mCurrentOpToken)); - mOperationStorage.registerOperation(mCurrentOpToken, OpState.PENDING, this, OpType.BACKUP); + mOperationStorage.registerOperationForPackages(mCurrentOpToken, OpState.PENDING, + packageNames, this, OpType.BACKUP); } // public, because called from KeyValueBackupTask.finishTask. @@ -834,12 +839,13 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba mBackupResult = BackupTransport.AGENT_ERROR; mQuota = quota; mTransportFlags = transportFlags; - registerTask(); + registerTask(target.packageName); } - void registerTask() { - mOperationStorage.registerOperation(mCurrentOpToken, - OpState.PENDING, this, OpType.BACKUP_WAIT); + void registerTask(String packageName) { + Set packages = Sets.newHashSet(packageName); + mOperationStorage.registerOperationForPackages(mCurrentOpToken, OpState.PENDING, + packages, this, OpType.BACKUP_WAIT); } void unregisterTask() { diff --git a/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java b/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java index bc953412a36ed..bccd8a0b14b48 100644 --- a/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/backup/UserBackupManagerServiceTest.java @@ -19,8 +19,12 @@ package com.android.server.backup; import static com.google.common.truth.Truth.assertThat; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import android.app.backup.BackupAgent; @@ -42,12 +46,16 @@ import com.android.server.backup.transport.BackupTransportClient; import com.android.server.backup.transport.TransportConnection; import com.android.server.backup.utils.BackupEligibilityRules; +import com.google.common.collect.ImmutableSet; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import java.util.function.IntConsumer; + @Presubmit @RunWith(AndroidJUnit4.class) public class UserBackupManagerServiceTest { @@ -163,6 +171,31 @@ public class UserBackupManagerServiceTest { assertThat(operationType).isEqualTo(OperationType.MIGRATION); } + @Test + public void testAgentDisconnected_cancelsCurrentOperations() throws Exception { + when(mOperationStorage.operationTokensForPackage(eq("com.android.foo"))).thenReturn( + ImmutableSet.of(123, 456, 789) + ); + + mService.agentDisconnected("com.android.foo"); + + verify(mOperationStorage).cancelOperation(eq(123), eq(true), any(IntConsumer.class)); + verify(mOperationStorage).cancelOperation(eq(456), eq(true), any()); + verify(mOperationStorage).cancelOperation(eq(789), eq(true), any()); + } + + @Test + public void testAgentDisconnected_unknownPackageName_cancelsNothing() throws Exception { + when(mOperationStorage.operationTokensForPackage(eq("com.android.foo"))).thenReturn( + ImmutableSet.of() + ); + + mService.agentDisconnected("com.android.foo"); + + verify(mOperationStorage, never()) + .cancelOperation(anyInt(), anyBoolean(), any(IntConsumer.class)); + } + private static PackageInfo getPackageInfo(String packageName) { PackageInfo packageInfo = new PackageInfo(); packageInfo.applicationInfo = new ApplicationInfo();