Merge "Don't kill apps if they are not in restricted mode" into main

9 files changed, 99 insertions, 110 deletions

diff --git a/services/backup/java/com/android/server/backup/BackupAgentConnectionManager.java b/services/backup/java/com/android/server/backup/BackupAgentConnectionManager.java
index 02f186557ca4..6ced096e8778 100644
--- a/services/backup/java/com/android/server/backup/BackupAgentConnectionManager.java
+++ b/services/backup/java/com/android/server/backup/BackupAgentConnectionManager.java
@@ -101,11 +101,16 @@ public class BackupAgentConnectionManager {
 
     private static final class BackupAgentConnection {
         public final ApplicationInfo appInfo;
+        public final int backupMode;
+        public final boolean inRestrictedMode;
         public IBackupAgent backupAgent;
         public boolean connecting = true; // Assume we are trying to connect on creation.
 
-        private BackupAgentConnection(ApplicationInfo appInfo) {
+        private BackupAgentConnection(ApplicationInfo appInfo, int backupMode,
+                boolean inRestrictedMode) {
             this.appInfo = appInfo;
+            this.backupMode = backupMode;
+            this.inRestrictedMode = inRestrictedMode;
         }
     }
 
@@ -113,26 +118,31 @@ public class BackupAgentConnectionManager {
      * Fires off a backup agent, blocking until it attaches (i.e. ActivityManager calls
      * {@link #agentConnected(String, IBinder)}) or until this operation times out.
      *
-     * @param mode a {@code BACKUP_MODE} from {@link android.app.ApplicationThreadConstants}.
+     * @param backupMode a {@code BACKUP_MODE} from {@link android.app.ApplicationThreadConstants}.
      */
     @Nullable
-    public IBackupAgent bindToAgentSynchronous(ApplicationInfo app, int mode,
+    public IBackupAgent bindToAgentSynchronous(ApplicationInfo app, int backupMode,
             @BackupAnnotations.BackupDestination int backupDestination) {
+        if (app == null) {
+            Slog.w(TAG, mUserIdMsg + "bindToAgentSynchronous for null app");
+            return null;
+        }
+
         synchronized (mAgentConnectLock) {
-            boolean useRestrictedMode = shouldUseRestrictedBackupModeForPackage(mode,
+            boolean useRestrictedMode = shouldUseRestrictedBackupModeForPackage(backupMode,
                     app.packageName);
             if (mCurrentConnection != null) {
                 Slog.e(TAG, mUserIdMsg + "binding to new agent before unbinding from old one: "
                         + mCurrentConnection.appInfo.packageName);
             }
-            mCurrentConnection = new BackupAgentConnection(app);
+            mCurrentConnection = new BackupAgentConnection(app, backupMode, useRestrictedMode);
 
             // bindBackupAgent() is an async API. It will kick off the app's process and call
             // agentConnected() when it receives the agent from the app.
             boolean startedBindSuccessfully = false;
             try {
-                startedBindSuccessfully = mActivityManager.bindBackupAgent(app.packageName, mode,
-                        mUserId, backupDestination, useRestrictedMode);
+                startedBindSuccessfully = mActivityManager.bindBackupAgent(app.packageName,
+                        backupMode, mUserId, backupDestination, useRestrictedMode);
             } catch (RemoteException e) {
                 // can't happen - ActivityManager is local
             }
@@ -173,28 +183,66 @@ public class BackupAgentConnectionManager {
     /**
      * Tell the ActivityManager that we are done with the {@link IBackupAgent} of this {@code app}.
      * It will tell the app to destroy the agent.
+     *
+     * <p>If {@code allowKill} is set, this will kill the app's process if the app is in restricted
+     * mode or if it was started for restore and specified {@code android:killAfterRestore} in its
+     * manifest.
+     *
+     * @see #shouldUseRestrictedBackupModeForPackage(int, String)
      */
-    public void unbindAgent(ApplicationInfo app) {
-        synchronized (mAgentConnectLock) {
-            if (mCurrentConnection == null) {
-                Slog.w(TAG, mUserIdMsg + "unbindAgent but no current connection");
-            } else if (!mCurrentConnection.appInfo.packageName.equals(app.packageName)) {
-                Slog.w(TAG, mUserIdMsg + "unbindAgent for unexpected package: " + app.packageName
-                        + " expected: " + mCurrentConnection.appInfo.packageName);
-            } else {
-                mCurrentConnection = null;
-            }
+    public void unbindAgent(ApplicationInfo app, boolean allowKill) {
+        if (app == null) {
+            Slog.w(TAG, mUserIdMsg + "unbindAgent for null app");
+            return;
+        }
 
+        synchronized (mAgentConnectLock) {
             // Even if we weren't expecting to be bound to this agent, we should still call
             // ActivityManager just in case. It will ignore the call if it also wasn't expecting it.
             try {
                 mActivityManager.unbindBackupAgent(app);
+
+                // Evaluate this before potentially setting mCurrentConnection = null.
+                boolean willKill = allowKill && shouldKillAppOnUnbind(app);
+
+                if (mCurrentConnection == null) {
+                    Slog.w(TAG, mUserIdMsg + "unbindAgent but no current connection");
+                } else if (!mCurrentConnection.appInfo.packageName.equals(app.packageName)) {
+                    Slog.w(TAG,
+                            mUserIdMsg + "unbindAgent for unexpected package: " + app.packageName
+                                    + " expected: " + mCurrentConnection.appInfo.packageName);
+                } else {
+                    mCurrentConnection = null;
+                }
+
+                if (willKill) {
+                    Slog.i(TAG, mUserIdMsg + "Killing agent host process");
+                    mActivityManager.killApplicationProcess(app.processName, app.uid);
+                }
             } catch (RemoteException e) {
                 // Can't happen - activity manager is local
             }
         }
     }
 
+    @GuardedBy("mAgentConnectLock")
+    private boolean shouldKillAppOnUnbind(ApplicationInfo app) {
+        // We don't ask system UID processes to be killed.
+        if (UserHandle.isCore(app.uid)) {
+            return false;
+        }
+
+        // If the app is in restricted mode or if we're not sure if it is because our internal
+        // state is messed up, we need to avoid it being stuck in it.
+        if (mCurrentConnection == null || mCurrentConnection.inRestrictedMode) {
+            return true;
+        }
+
+        // App was doing restore and asked to be killed afterwards.
+        return isBackupModeRestore(mCurrentConnection.backupMode)
+                && (app.flags & ApplicationInfo.FLAG_KILL_AFTER_RESTORE) != 0;
+    }
+
     /**
      * Callback: a requested backup agent has been instantiated. This should only be called from
      * the {@link ActivityManager} when it's telling us that an agent is ready after a call to
@@ -307,16 +355,16 @@ public class BackupAgentConnectionManager {
      */
     private boolean shouldUseRestrictedBackupModeForPackage(
             @BackupAnnotations.OperationType int mode, String packageName) {
-        if (!Flags.enableRestrictedModeChanges()) {
-            return true;
-        }
-
         // Key/Value apps are never put in restricted mode.
         if (mode == ApplicationThreadConstants.BACKUP_MODE_INCREMENTAL
                 || mode == ApplicationThreadConstants.BACKUP_MODE_RESTORE) {
             return false;
         }
 
+        if (!Flags.enableRestrictedModeChanges()) {
+            return true;
+        }
+
         try {
             PackageManager.Property property = mPackageManager.getPropertyAsUser(
                     PackageManager.PROPERTY_USE_RESTRICTED_BACKUP_MODE,
@@ -352,6 +400,11 @@ public class BackupAgentConnectionManager {
         return true;
     }
 
+    private static boolean isBackupModeRestore(int backupMode) {
+        return backupMode == ApplicationThreadConstants.BACKUP_MODE_RESTORE
+                || backupMode == ApplicationThreadConstants.BACKUP_MODE_RESTORE_FULL;
+    }
+
     @VisibleForTesting
     Thread getThreadForCancellation(Runnable operation) {
         return new Thread(operation, /* operationName */ "agent-disconnected");
diff --git a/services/backup/java/com/android/server/backup/KeyValueAdbBackupEngine.java b/services/backup/java/com/android/server/backup/KeyValueAdbBackupEngine.java
index 136bacdd6399..b343ec8e709b 100644
--- a/services/backup/java/com/android/server/backup/KeyValueAdbBackupEngine.java
+++ b/services/backup/java/com/android/server/backup/KeyValueAdbBackupEngine.java
@@ -300,7 +300,8 @@ public class KeyValueAdbBackupEngine {
     }
 
     private void cleanup() {
-        mBackupManagerService.tearDownAgentAndKill(mCurrentPackage.applicationInfo);
+        mBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                mCurrentPackage.applicationInfo, /* allowKill= */ true);
         mBlankStateName.delete();
         mNewStateName.delete();
         mBackupDataName.delete();
diff --git a/services/backup/java/com/android/server/backup/UserBackupManagerService.java b/services/backup/java/com/android/server/backup/UserBackupManagerService.java
index e085f6e63067..a90b693c5a1d 100644
--- a/services/backup/java/com/android/server/backup/UserBackupManagerService.java
+++ b/services/backup/java/com/android/server/backup/UserBackupManagerService.java
@@ -1998,39 +1998,6 @@ public class UserBackupManagerService {
         return mOperationStorage.isBackupOperationInProgress();
     }
 
-    /** Unbind the backup agent and kill the app if it's a non-system app. */
-    public void tearDownAgentAndKill(ApplicationInfo app) {
-        if (app == null) {
-            // Null means the system package, so just quietly move on.  :)
-            return;
-        }
-
-        try {
-            // unbind and tidy up even on timeout or failure, just in case
-            mActivityManager.unbindBackupAgent(app);
-
-            // The agent was running with a stub Application object, so shut it down.
-            // !!! We hardcode the confirmation UI's package name here rather than use a
-            //     manifest flag!  TODO something less direct.
-            if (!UserHandle.isCore(app.uid)
-                    && !app.packageName.equals("com.android.backupconfirm")) {
-                if (MORE_DEBUG) {
-                    Slog.d(TAG, addUserIdToLogMessage(mUserId, "Killing agent host process"));
-                }
-                mActivityManager.killApplicationProcess(app.processName, app.uid);
-            } else {
-                if (MORE_DEBUG) {
-                    Slog.d(
-                            TAG,
-                            addUserIdToLogMessage(
-                                    mUserId, "Not killing after operation: " + app.processName));
-                }
-            }
-        } catch (RemoteException e) {
-            Slog.d(TAG, addUserIdToLogMessage(mUserId, "Lost app trying to shut down"));
-        }
-    }
-
     // ----- Full-data backup scheduling -----
 
     /**
diff --git a/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java b/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java
index b98cb1086680..cf617a523bec 100644
--- a/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java
+++ b/services/backup/java/com/android/server/backup/fullbackup/FullBackupEngine.java
@@ -323,7 +323,8 @@ public class FullBackupEngine {
 
     private void tearDown() {
         if (mPkg != null) {
-            backupManagerService.tearDownAgentAndKill(mPkg.applicationInfo);
+            backupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                    mPkg.applicationInfo, /* allowKill= */ true);
         }
     }
 }
diff --git a/services/backup/java/com/android/server/backup/fullbackup/PerformAdbBackupTask.java b/services/backup/java/com/android/server/backup/fullbackup/PerformAdbBackupTask.java
index dc6709141b25..0ba0d710af38 100644
--- a/services/backup/java/com/android/server/backup/fullbackup/PerformAdbBackupTask.java
+++ b/services/backup/java/com/android/server/backup/fullbackup/PerformAdbBackupTask.java
@@ -503,7 +503,8 @@ public class PerformAdbBackupTask extends FullBackupTask implements BackupRestor
             Slog.w(TAG, "adb backup cancel of " + target);
         }
         if (target != null) {
-            mUserBackupManagerService.tearDownAgentAndKill(mCurrentTarget.applicationInfo);
+            mUserBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                    target.applicationInfo, /* allowKill= */ true);
         }
         mOperationStorage.removeOperation(mCurrentOpToken);
     }
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 65730c9591a8..799494831f19 100644
--- a/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java
+++ b/services/backup/java/com/android/server/backup/fullbackup/PerformFullTransportBackupTask.java
@@ -596,8 +596,8 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba
                     // from the preflight pass.  If we got as far as preflight, we now need
                     // to tear down the target process.
                     if (mBackupRunner != null) {
-                        mUserBackupManagerService.tearDownAgentAndKill(
-                                currentPackage.applicationInfo);
+                        mUserBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                                currentPackage.applicationInfo, /* allowKill= */ true);
                     }
                     // ... and continue looping.
                 } else if (backupPackageStatus == BackupTransport.TRANSPORT_QUOTA_EXCEEDED) {
@@ -609,7 +609,8 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba
                         EventLog.writeEvent(EventLogTags.FULL_BACKUP_QUOTA_EXCEEDED,
                                 packageName);
                     }
-                    mUserBackupManagerService.tearDownAgentAndKill(currentPackage.applicationInfo);
+                    mUserBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                            currentPackage.applicationInfo, /* allowKill= */ true);
                     // Do nothing, clean up, and continue looping.
                 } else if (backupPackageStatus == BackupTransport.AGENT_ERROR) {
                     BackupObserverUtils
@@ -617,7 +618,8 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba
                                     BackupManager.ERROR_AGENT_FAILURE);
                     Slog.w(TAG, "Application failure for package: " + packageName);
                     EventLog.writeEvent(EventLogTags.BACKUP_AGENT_FAILURE, packageName);
-                    mUserBackupManagerService.tearDownAgentAndKill(currentPackage.applicationInfo);
+                    mUserBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                            currentPackage.applicationInfo, /* allowKill= */ true);
                     // Do nothing, clean up, and continue looping.
                 } else if (backupPackageStatus == BackupManager.ERROR_BACKUP_CANCELLED) {
                     BackupObserverUtils
@@ -626,7 +628,8 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba
                     Slog.w(TAG, "Backup cancelled. package=" + packageName +
                             ", cancelAll=" + mCancelAll);
                     EventLog.writeEvent(EventLogTags.FULL_BACKUP_CANCELLED, packageName);
-                    mUserBackupManagerService.tearDownAgentAndKill(currentPackage.applicationInfo);
+                    mUserBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                            currentPackage.applicationInfo, /* allowKill= */ true);
                     // Do nothing, clean up, and continue looping.
                 } else if (backupPackageStatus != BackupTransport.TRANSPORT_OK) {
                     BackupObserverUtils
@@ -636,7 +639,8 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba
                     EventLog.writeEvent(EventLogTags.FULL_BACKUP_TRANSPORT_FAILURE);
                     // Abort entire backup pass.
                     backupRunStatus = BackupManager.ERROR_TRANSPORT_ABORTED;
-                    mUserBackupManagerService.tearDownAgentAndKill(currentPackage.applicationInfo);
+                    mUserBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                            currentPackage.applicationInfo, /* allowKill= */ true);
                     return;
                 } else {
                     // Success!
@@ -1005,7 +1009,8 @@ public class PerformFullTransportBackupTask extends FullBackupTask implements Ba
             mIsCancelled = true;
             // Cancel tasks spun off by this task.
             mUserBackupManagerService.handleCancel(mEphemeralToken, cancelAll);
-            mUserBackupManagerService.tearDownAgentAndKill(mTarget.applicationInfo);
+            mUserBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                    mTarget.applicationInfo, /* allowKill= */ true);
             // Free up everyone waiting on this task and its children.
             mPreflightLatch.countDown();
             mBackupLatch.countDown();
diff --git a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
index 82232a653858..689c43a1cc96 100644
--- a/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
+++ b/services/backup/java/com/android/server/backup/keyvalue/KeyValueBackupTask.java
@@ -1303,7 +1303,7 @@ public class KeyValueBackupTask implements BackupRestoreTask, Runnable {
         // For PM metadata (for which applicationInfo is null) there is no agent-bound state.
         if (mCurrentPackage.applicationInfo != null) {
             mBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
-                    mCurrentPackage.applicationInfo);
+                    mCurrentPackage.applicationInfo, /* allowKill= */ false);
         }
         mAgent = null;
     }
diff --git a/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java b/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java
index b59e860f81fe..237ffa3bdb21 100644
--- a/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java
+++ b/services/backup/java/com/android/server/backup/restore/FullRestoreEngine.java
@@ -727,7 +727,8 @@ public class FullRestoreEngine extends RestoreEngine {
                     latch.await();
                 }
 
-                mBackupManagerService.tearDownAgentAndKill(app);
+                mBackupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                        app, /* allowKill= */ true);
             } catch (RemoteException e) {
                 Slog.d(TAG, "Lost app trying to shut down");
             }
diff --git a/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java b/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java
index e5c7e5cce757..dad84c86deef 100644
--- a/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java
+++ b/services/backup/java/com/android/server/backup/restore/PerformUnifiedRestoreTask.java
@@ -51,7 +51,6 @@ import android.os.ParcelFileDescriptor;
 import android.os.Process;
 import android.os.RemoteException;
 import android.os.SystemClock;
-import android.os.UserHandle;
 import android.provider.Settings;
 import android.util.ArraySet;
 import android.util.EventLog;
@@ -1487,49 +1486,10 @@ public class PerformUnifiedRestoreTask implements BackupRestoreTask {
 
         // If this wasn't the PM pseudopackage, tear down the agent side
         if (mCurrentPackage.applicationInfo != null) {
-            // unbind and tidy up even on timeout or failure
-            try {
-                backupManagerService
-                        .getActivityManager()
-                        .unbindBackupAgent(mCurrentPackage.applicationInfo);
-
-                // The agent was probably running with a stub Application object,
-                // which isn't a valid run mode for the main app logic.  Shut
-                // down the app so that next time it's launched, it gets the
-                // usual full initialization.  Note that this is only done for
-                // full-system restores: when a single app has requested a restore,
-                // it is explicitly not killed following that operation.
-                //
-                // We execute this kill when these conditions hold:
-                //    1. it's not a system-uid process,
-                //    2. the app did not request its own restore (mTargetPackage == null), and
-                // either
-                //    3a. the app is a full-data target (TYPE_FULL_STREAM) or
-                //     b. the app does not state android:killAfterRestore="false" in its manifest
-                final int appFlags = mCurrentPackage.applicationInfo.flags;
-                final boolean killAfterRestore =
-                        !UserHandle.isCore(mCurrentPackage.applicationInfo.uid)
-                                && ((mRestoreDescription.getDataType()
-                                                == RestoreDescription.TYPE_FULL_STREAM)
-                                        || ((appFlags & ApplicationInfo.FLAG_KILL_AFTER_RESTORE)
-                                                != 0));
-
-                if (mTargetPackage == null && killAfterRestore) {
-                    if (DEBUG) {
-                        Slog.d(
-                                TAG,
-                                "Restore complete, killing host process of "
-                                        + mCurrentPackage.applicationInfo.processName);
-                    }
-                    backupManagerService
-                            .getActivityManager()
-                            .killApplicationProcess(
-                                    mCurrentPackage.applicationInfo.processName,
-                                    mCurrentPackage.applicationInfo.uid);
-                }
-            } catch (RemoteException e) {
-                // can't happen; we run in the same process as the activity manager
-            }
+            // If mTargetPackage is not null it means the app requested its own restore, in which
+            // case we won't allow the app to be killed.
+            backupManagerService.getBackupAgentConnectionManager().unbindAgent(
+                    mCurrentPackage.applicationInfo, /* allowKill= */ mTargetPackage == null);
         }
 
         // The caller is responsible for reestablishing the state machine; our