Merge changes from topic "abandon-race" into rvc-dev

* changes:
  Fix racing condition between session abandonment and its verification
  Persist destroyed staged sessions until they are cleaned up

services/core/java/com/android/server/pm/PackageInstallerService.java

@@ -676,7 +676,7 @@ public class PackageInstallerService extends IPackageInstaller.Stub implements
         session = new PackageInstallerSession(mInternalCallback, mContext, mPm, this,
                 mInstallThread.getLooper(), mStagingManager, sessionId, userId, callingUid,
                 installSource, params, createdMillis,
-                stageDir, stageCid, null, false, false, false, null, SessionInfo.INVALID_ID,
+                stageDir, stageCid, null, false, false, false, false, null, SessionInfo.INVALID_ID,
                 false, false, false, SessionInfo.STAGED_SESSION_NO_ERROR, "");
 
         synchronized (mSessions) {
@@ -830,7 +830,7 @@ public class PackageInstallerService extends IPackageInstaller.Stub implements
         synchronized (mSessions) {
             final PackageInstallerSession session = mSessions.get(sessionId);
 
-            return session != null
+            return (session != null && !(session.isStaged() && session.isDestroyed()))
                     ? session.generateInfoForCaller(true /*withIcon*/, Binder.getCallingUid())
                     : null;
         }
@@ -851,7 +851,8 @@ public class PackageInstallerService extends IPackageInstaller.Stub implements
         synchronized (mSessions) {
             for (int i = 0; i < mSessions.size(); i++) {
                 final PackageInstallerSession session = mSessions.valueAt(i);
-                if (session.userId == userId && !session.hasParentSessionId()) {
+                if (session.userId == userId && !session.hasParentSessionId()
+                        && !(session.isStaged() && session.isDestroyed())) {
                     result.add(session.generateInfoForCaller(false, callingUid));
                 }
             }
@@ -1269,7 +1270,7 @@ public class PackageInstallerService extends IPackageInstaller.Stub implements
         public void onStagedSessionChanged(PackageInstallerSession session) {
             session.markUpdated();
             writeSessionsAsync();
-            if (mOkToSendBroadcasts) {
+            if (mOkToSendBroadcasts && !session.isDestroyed()) {
                 // we don't scrub the data here as this is sent only to the installer several
                 // privileged system packages
                 mPm.sendSessionUpdatedBroadcast(

services/core/java/com/android/server/pm/PackageInstallerSession.java

@@ -190,6 +190,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     private static final String ATTR_SESSION_STAGE_CID = "sessionStageCid";
     private static final String ATTR_PREPARED = "prepared";
     private static final String ATTR_COMMITTED = "committed";
+    private static final String ATTR_DESTROYED = "destroyed";
     private static final String ATTR_SEALED = "sealed";
     private static final String ATTR_MULTI_PACKAGE = "multiPackage";
     private static final String ATTR_PARENT_SESSION_ID = "parentSessionId";
@@ -533,7 +534,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             int sessionId, int userId, int installerUid, @NonNull InstallSource installSource,
             SessionParams params, long createdMillis,
             File stageDir, String stageCid, InstallationFile[] files, boolean prepared,
-            boolean committed, boolean sealed,
+            boolean committed, boolean destroyed, boolean sealed,
             @Nullable int[] childSessionIds, int parentSessionId, boolean isReady,
             boolean isFailed, boolean isApplied, int stagedSessionErrorCode,
             String stagedSessionErrorMessage) {
@@ -579,6 +580,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
 
         mPrepared = prepared;
         mCommitted = committed;
+        mDestroyed = destroyed;
         mStagedSessionReady = isReady;
         mStagedSessionFailed = isFailed;
         mStagedSessionApplied = isApplied;
@@ -713,6 +715,13 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         }
     }
 
+    /** {@hide} */
+    boolean isDestroyed() {
+        synchronized (mLock) {
+            return mDestroyed;
+        }
+    }
+
     /** Returns true if a staged session has reached a final state and can be forgotten about  */
     public boolean isStagedAndInTerminalState() {
         synchronized (mLock) {
@@ -1067,6 +1076,19 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         }
     }
 
+    /**
+     * Check if the caller is the owner of this session. Otherwise throw a
+     * {@link SecurityException}.
+     */
+    @GuardedBy("mLock")
+    private void assertCallerIsOwnerOrRootOrSystemLocked() {
+        final int callingUid = Binder.getCallingUid();
+        if (callingUid != Process.ROOT_UID && callingUid != mInstallerUid
+                && callingUid != Process.SYSTEM_UID) {
+            throw new SecurityException("Session does not belong to uid " + callingUid);
+        }
+    }
+
     /**
      * If anybody is reading or writing data of the session, throw an {@link SecurityException}.
      */
@@ -2552,7 +2574,13 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                             + mParentSessionId +  " and may not be abandoned directly.");
         }
         synchronized (mLock) {
+            if (params.isStaged && mDestroyed) {
+                // If a user abandons staged session in an unsafe state, then system will try to
+                // abandon the destroyed staged session when it is safe on behalf of the user.
+                assertCallerIsOwnerOrRootOrSystemLocked();
+            } else {
                 assertCallerIsOwnerOrRootLocked();
+            }
 
             if (isStagedAndInTerminalState()) {
                 // We keep the session in the database if it's in a finalized state. It will be
@@ -2562,11 +2590,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
                 return;
             }
             if (mCommitted && params.isStaged) {
-                synchronized (mLock) {
                 mDestroyed = true;
+                if (!mStagingManager.abortCommittedSessionLocked(this)) {
+                    // Do not clean up the staged session from system. It is not safe yet.
+                    mCallback.onStagedSessionChanged(this);
+                    return;
                 }
-                mStagingManager.abortCommittedSession(this);
-
                 cleanStageDir();
             }
 
@@ -2926,6 +2955,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     /** {@hide} */
     void setStagedSessionReady() {
         synchronized (mLock) {
+            if (mDestroyed) return; // Do not allow destroyed staged session to change state
             mStagedSessionReady = true;
             mStagedSessionApplied = false;
             mStagedSessionFailed = false;
@@ -2939,6 +2969,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     void setStagedSessionFailed(@StagedSessionErrorCode int errorCode,
                                 String errorMessage) {
         synchronized (mLock) {
+            if (mDestroyed) return; // Do not allow destroyed staged session to change state
             mStagedSessionReady = false;
             mStagedSessionApplied = false;
             mStagedSessionFailed = true;
@@ -2953,6 +2984,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
     /** {@hide} */
     void setStagedSessionApplied() {
         synchronized (mLock) {
+            if (mDestroyed) return; // Do not allow destroyed staged session to change state
             mStagedSessionReady = false;
             mStagedSessionApplied = true;
             mStagedSessionFailed = false;
@@ -3197,7 +3229,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
      */
     void write(@NonNull XmlSerializer out, @NonNull File sessionsDir) throws IOException {
         synchronized (mLock) {
-            if (mDestroyed) {
+            if (mDestroyed && !params.isStaged) {
                 return;
             }
 
@@ -3223,6 +3255,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
             }
             writeBooleanAttribute(out, ATTR_PREPARED, isPrepared());
             writeBooleanAttribute(out, ATTR_COMMITTED, isCommitted());
+            writeBooleanAttribute(out, ATTR_DESTROYED, isDestroyed());
             writeBooleanAttribute(out, ATTR_SEALED, isSealed());
 
             writeBooleanAttribute(out, ATTR_MULTI_PACKAGE, params.isMultiPackage);
@@ -3352,6 +3385,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         final String stageCid = readStringAttribute(in, ATTR_SESSION_STAGE_CID);
         final boolean prepared = readBooleanAttribute(in, ATTR_PREPARED, true);
         final boolean committed = readBooleanAttribute(in, ATTR_COMMITTED);
+        final boolean destroyed = readBooleanAttribute(in, ATTR_DESTROYED);
         final boolean sealed = readBooleanAttribute(in, ATTR_SEALED);
         final int parentSessionId = readIntAttribute(in, ATTR_PARENT_SESSION_ID,
                 SessionInfo.INVALID_ID);
@@ -3473,7 +3507,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub {
         return new PackageInstallerSession(callback, context, pm, sessionProvider,
                 installerThread, stagingManager, sessionId, userId, installerUid,
                 installSource, params, createdMillis, stageDir, stageCid, fileArray,
-                prepared, committed, sealed, childSessionIdsArray, parentSessionId,
+                prepared, committed, destroyed, sealed, childSessionIdsArray, parentSessionId,
                 isReady, isFailed, isApplied, stagedSessionErrorCode, stagedSessionErrorMessage);
     }
 }

services/core/java/com/android/server/pm/StagingManager.java

@@ -61,6 +61,7 @@ import android.text.TextUtils;
 import android.util.IntArray;
 import android.util.Slog;
 import android.util.SparseArray;
+import android.util.SparseBooleanArray;
 import android.util.SparseIntArray;
 import android.util.apk.ApkSignatureVerifier;
 
@@ -137,6 +138,9 @@ public class StagingManager {
         synchronized (mStagedSessions) {
             for (int i = 0; i < mStagedSessions.size(); i++) {
                 final PackageInstallerSession stagedSession = mStagedSessions.valueAt(i);
+                if (stagedSession.isDestroyed()) {
+                    continue;
+                }
                 result.add(stagedSession.generateInfoForCaller(false /*icon*/, callingUid));
             }
         }
@@ -202,7 +206,7 @@ public class StagingManager {
         final IntArray childSessionIds = new IntArray();
         if (session.isMultiPackage()) {
             for (int id : session.getChildSessionIds()) {
-                if (isApexSession(mStagedSessions.get(id))) {
+                if (isApexSession(getStagedSession(id))) {
                     childSessionIds.add(id);
                 }
             }
@@ -797,6 +801,8 @@ public class StagingManager {
                                 + session.sessionId + " [" + errorMessage + "]");
                         session.setStagedSessionFailed(
                                 SessionInfo.STAGED_SESSION_VERIFICATION_FAILED, errorMessage);
+                        mPreRebootVerificationHandler.onPreRebootVerificationComplete(
+                                session.sessionId);
                         return;
                     }
                     mPreRebootVerificationHandler.notifyPreRebootVerification_Apk_Complete(
@@ -880,7 +886,8 @@ public class StagingManager {
         synchronized (mStagedSessions) {
             for (int i = 0; i < mStagedSessions.size(); i++) {
                 final PackageInstallerSession stagedSession = mStagedSessions.valueAt(i);
-                if (!stagedSession.isCommitted() || stagedSession.isStagedAndInTerminalState()) {
+                if (!stagedSession.isCommitted() || stagedSession.isStagedAndInTerminalState()
+                        || stagedSession.isDestroyed()) {
                     continue;
                 }
                 if (stagedSession.isMultiPackage()) {
@@ -943,29 +950,70 @@ public class StagingManager {
         }
     }
 
-    void abortCommittedSession(@NonNull PackageInstallerSession session) {
+    /**
+     * <p>Abort committed staged session
+     *
+     * <p>This method must be called while holding {@link PackageInstallerSession.mLock}.
+     *
+     * <p>The method returns {@code false} to indicate it is not safe to clean up the session from
+     * system yet. When it is safe, the method returns {@code true}.
+     *
+     * <p> When it is safe to clean up, {@link StagingManager} will call
+     * {@link PackageInstallerSession#abandon()} on the session again.
+     *
+     * @return {@code true} if it is safe to cleanup the session resources, otherwise {@code false}.
+     */
+    boolean abortCommittedSessionLocked(@NonNull PackageInstallerSession session) {
+        int sessionId = session.sessionId;
         if (session.isStagedSessionApplied()) {
-            Slog.w(TAG, "Cannot abort applied session : " + session.sessionId);
+            Slog.w(TAG, "Cannot abort applied session : " + sessionId);
-            return;
+            return false;
+        }
+        if (!session.isDestroyed()) {
+            throw new IllegalStateException("Committed session must be destroyed before aborting it"
+                    + " from StagingManager");
+        }
+        if (getStagedSession(sessionId) == null) {
+            Slog.w(TAG, "Session " + sessionId + " has been abandoned already");
+            return false;
         }
-        abortSession(session);
 
-        boolean hasApex = sessionContainsApex(session);
+        // If pre-reboot verification is running, then return false. StagingManager will call
-        if (hasApex) {
+        // abandon again when pre-reboot verification ends.
-            ApexSessionInfo apexSession = mApexManager.getStagedSessionInfo(session.sessionId);
+        if (mPreRebootVerificationHandler.isVerificationRunning(sessionId)) {
+            Slog.w(TAG, "Session " + sessionId + " aborted before pre-reboot "
+                    + "verification completed.");
+            return false;
+        }
+
+        // A session could be marked ready once its pre-reboot verification ends
+        if (session.isStagedSessionReady()) {
+            if (sessionContainsApex(session)) {
+                try {
+                    ApexSessionInfo apexSession =
+                            mApexManager.getStagedSessionInfo(session.sessionId);
                     if (apexSession == null || isApexSessionFinalized(apexSession)) {
                         Slog.w(TAG,
                                 "Cannot abort session " + session.sessionId
-                                + " because it is not active or APEXD is not reachable");
+                                        + " because it is not active.");
-                return;
+                    } else {
-            }
-            try {
                         mApexManager.abortStagedSession(session.sessionId);
-            } catch (Exception ignore) {
+                    }
+                } catch (Exception e) {
+                    // Failed to contact apexd service. The apex might still be staged. We can still
+                    // safely cleanup the staged session since pre-reboot verification is complete.
+                    // Also, cleaning up the stageDir prevents the apex from being activated.
+                    Slog.w(TAG, "Could not contact apexd to abort staged session " + sessionId);
                 }
             }
         }
 
+        // Session was successfully aborted from apexd (if required) and pre-reboot verification
+        // is also complete. It is now safe to clean up the session from system.
+        abortSession(session);
+        return true;
+    }
+
     private boolean isApexSessionFinalized(ApexSessionInfo session) {
         /* checking if the session is in a final state, i.e., not active anymore */
         return session.isUnknown || session.isActivationFailed || session.isSuccess
@@ -1042,6 +1090,11 @@ public class StagingManager {
             // Final states, nothing to do.
             return;
         }
+        if (session.isDestroyed()) {
+            // Device rebooted before abandoned session was cleaned up.
+            session.abandon();
+            return;
+        }
         if (!session.isStagedSessionReady()) {
             // The framework got restarted before the pre-reboot verification could complete,
             // restart the verification.
@@ -1124,10 +1177,20 @@ public class StagingManager {
         }
     }
 
+    private PackageInstallerSession getStagedSession(int sessionId) {
+        PackageInstallerSession session;
+        synchronized (mStagedSessions) {
+            session = mStagedSessions.get(sessionId);
+        }
+        return session;
+    }
+
     private final class PreRebootVerificationHandler extends Handler {
         // Hold session ids before handler gets ready to do the verification.
         private IntArray mPendingSessionIds;
         private boolean mIsReady;
+        @GuardedBy("mVerificationRunning")
+        private final SparseBooleanArray mVerificationRunning = new SparseBooleanArray();
 
         PreRebootVerificationHandler(Looper looper) {
             super(looper);
@@ -1155,13 +1218,15 @@ public class StagingManager {
         @Override
         public void handleMessage(Message msg) {
             final int sessionId = msg.arg1;
-            final PackageInstallerSession session;
+            final PackageInstallerSession session = getStagedSession(sessionId);
-            synchronized (mStagedSessions) {
-                session = mStagedSessions.get(sessionId);
-            }
-            // Maybe session was aborted before pre-reboot verification was complete
             if (session == null) {
-                Slog.d(TAG, "Stopping pre-reboot verification for sessionId: " + sessionId);
+                Slog.wtf(TAG, "Session disappeared in the middle of pre-reboot verification: "
+                        + sessionId);
+                return;
+            }
+            if (session.isDestroyed()) {
+                // No point in running verification on a destroyed session
+                onPreRebootVerificationComplete(sessionId);
                 return;
             }
             switch (msg.what) {
@@ -1200,9 +1265,40 @@ public class StagingManager {
                 mPendingSessionIds.add(sessionId);
                 return;
             }
+
+            PackageInstallerSession session = getStagedSession(sessionId);
+            synchronized (mVerificationRunning) {
+                // Do not start verification on a session that has been abandoned
+                if (session == null || session.isDestroyed()) {
+                    return;
+                }
+                Slog.d(TAG, "Starting preRebootVerification for session " + sessionId);
+                mVerificationRunning.put(sessionId, true);
+            }
             obtainMessage(MSG_PRE_REBOOT_VERIFICATION_START, sessionId, 0).sendToTarget();
         }
 
+        // Things to do when pre-reboot verification completes for a particular sessionId
+        private void onPreRebootVerificationComplete(int sessionId) {
+            // Remove it from mVerificationRunning so that verification is considered complete
+            synchronized (mVerificationRunning) {
+                Slog.d(TAG, "Stopping preRebootVerification for session " + sessionId);
+                mVerificationRunning.delete(sessionId);
+            }
+            // Check if the session was destroyed while pre-reboot verification was running. If so,
+            // abandon it again.
+            PackageInstallerSession session = getStagedSession(sessionId);
+            if (session != null && session.isDestroyed()) {
+                session.abandon();
+            }
+        }
+
+        private boolean isVerificationRunning(int sessionId) {
+            synchronized (mVerificationRunning) {
+                return mVerificationRunning.get(sessionId);
+            }
+        }
+
         private void notifyPreRebootVerification_Start_Complete(int sessionId) {
             obtainMessage(MSG_PRE_REBOOT_VERIFICATION_APEX, sessionId, 0).sendToTarget();
         }
@@ -1221,8 +1317,6 @@ public class StagingManager {
          * See {@link PreRebootVerificationHandler} to see all nodes of pre reboot verification
          */
         private void handlePreRebootVerification_Start(@NonNull PackageInstallerSession session) {
-            Slog.d(TAG, "Starting preRebootVerification for session " + session.sessionId);
-
             if ((session.params.installFlags & PackageManager.INSTALL_ENABLE_ROLLBACK) != 0) {
                 // If rollback is enabled for this session, we call through to the RollbackManager
                 // with the list of sessions it must enable rollback for. Note that
@@ -1269,6 +1363,7 @@ public class StagingManager {
                     }
                 } catch (PackageManagerException e) {
                     session.setStagedSessionFailed(e.error, e.getMessage());
+                    onPreRebootVerificationComplete(session.sessionId);
                     return;
                 }
 
@@ -1301,6 +1396,7 @@ public class StagingManager {
                 // TODO(b/118865310): abort the session on apexd.
             } catch (PackageManagerException e) {
                 session.setStagedSessionFailed(e.error, e.getMessage());
+                onPreRebootVerificationComplete(session.sessionId);
             }
         }
 
@@ -1323,9 +1419,18 @@ public class StagingManager {
                 Slog.e(TAG, "Failed to get hold of StorageManager", e);
                 session.setStagedSessionFailed(SessionInfo.STAGED_SESSION_UNKNOWN,
                         "Failed to get hold of StorageManager");
+                onPreRebootVerificationComplete(session.sessionId);
                 return;
             }
 
+            // Stop pre-reboot verification before marking session ready. From this point on, if we
+            // abandon the session then it will be cleaned up immediately. If session is abandoned
+            // after this point, then even if for some reason system tries to install the session
+            // or activate its apex, there won't be any files to work with as they will be cleaned
+            // up by the system as part of abandonment. If session is abandoned before this point,
+            // then the session is already destroyed and cannot be marked ready anymore.
+            onPreRebootVerificationComplete(session.sessionId);
+
             // Proactively mark session as ready before calling apexd. Although this call order
             // looks counter-intuitive, this is the easiest way to ensure that session won't end up
             // in the inconsistent state:
@@ -1337,15 +1442,16 @@ public class StagingManager {
             // only apex part of the train will be applied, leaving device in an inconsistent state.
             Slog.d(TAG, "Marking session " + session.sessionId + " as ready");
             session.setStagedSessionReady();
+            if (session.isStagedSessionReady()) {
                 final boolean hasApex = sessionContainsApex(session);
-            if (!hasApex) {
+                if (hasApex) {
-                // Session doesn't contain apex, nothing to do.
-                return;
-            }
                     try {
                         mApexManager.markStagedSessionReady(session.sessionId);
                     } catch (PackageManagerException e) {
                         session.setStagedSessionFailed(e.error, e.getMessage());
+                        return;
+                    }
+                }
             }
         }
     }

services/tests/servicestests/src/com/android/server/pm/PackageInstallerSessionTest.java

@@ -178,6 +178,7 @@ public class PackageInstallerSessionTest {
                 /* files */ null,
                 /* prepared */ true,
                 /* committed */ true,
+                /* destroyed */ staged ? true : false,
                 /* sealed */ false,  // Setting to true would trigger some PM logic.
                 /* childSessionIds */ childSessionIds != null ? childSessionIds : new int[0],
                 /* parentSessionId */ parentSessionId,