From e6542d041b4f0ddf558373d264ce881107372e6e Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 21 Aug 2020 15:02:56 +0800 Subject: [PATCH 1/4] Rewrite how we abandon stages sessions (1/n) 1. if abandon() is called before or after pre-reboot verification, it is safe to clean up the session immediately. 2. otherwise, it wait until notifyEndPreRebootVerification() is called to do the clean-up. This is the 1st step to move all calls to StagingManager outside of the lock. The goal is make it safe for StagingManager to call into PackageInstallerSession methods without incurring deadlock. Bug: 161765186 Test: atest StagedInstallTest AtomicInstallTest StagedInstallInternalTest Change-Id: Ic37b58e621ba8fd0ba61c78b9ac44871fe0cefcb --- .../server/pm/PackageInstallerSession.java | 93 +++++++++++++++---- .../com/android/server/pm/StagingManager.java | 61 ++++-------- 2 files changed, 95 insertions(+), 59 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index ed62362b04fb0..dea840a6e155f 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -388,6 +388,20 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @GuardedBy("mLock") private String mStagedSessionErrorMessage; + /** + * The callback to run when pre-reboot verification has ended. Used by {@link #abandonStaged()} + * to delay session clean-up until it is safe to do so. + */ + @GuardedBy("mLock") + @Nullable + private Runnable mPendingAbandonCallback; + /** + * {@code true} if pre-reboot verification is ongoing which means it is not safe for + * {@link #abandon()} to clean up staging directories. + */ + @GuardedBy("mLock") + private boolean mInPreRebootVerification; + /** * Path to the validated base APK for this session, which may point at an * APK inside the session (when the session defines the base), or it may @@ -1570,7 +1584,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { SessionInfo.STAGED_SESSION_VERIFICATION_FAILED, detailMessage); // TODO(b/136257624): Remove this once all verification logic has been transferred out // of StagingManager. - mStagingManager.notifyVerificationComplete(sessionId); + mStagingManager.notifyVerificationComplete(this); } else { // Dispatch message to remove session from PackageInstallerService. dispatchSessionFinished(error, detailMessage, null); @@ -2786,14 +2800,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } private void abandonStaged() { + final Runnable r; synchronized (mLock) { - if (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(); - } + assertCallerIsOwnerOrRootLocked(); if (isStagedAndInTerminalState()) { // We keep the session in the database if it's in a finalized state. It will be // removed by PackageInstallerService when the last update time is old enough. @@ -2802,17 +2811,25 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return; } mDestroyed = true; - if (mCommitted) { - if (!mStagingManager.abortCommittedSessionLocked(this)) { - // Do not clean up the staged session from system. It is not safe yet. - mCallback.onStagedSessionChanged(this); - return; + boolean isCommitted = mCommitted; + List childSessions = getChildSessionsLocked(); + r = () -> { + assertNotLocked("abandonStaged"); + if (isCommitted) { + mStagingManager.abortCommittedSession(this); } + cleanStageDir(childSessions); + destroyInternal(); + dispatchSessionFinished(INSTALL_FAILED_ABORTED, "Session was abandoned", null); + }; + if (mInPreRebootVerification) { + // Pre-reboot verification is ongoing. It is not safe to clean up the session yet. + mPendingAbandonCallback = r; + mCallback.onStagedSessionChanged(this); + return; } - cleanStageDir(getChildSessionsLocked()); - destroyInternal(); } - dispatchSessionFinished(INSTALL_FAILED_ABORTED, "Session was abandoned", null); + r.run(); } @Override @@ -2829,6 +2846,50 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } + /** + * Notified by the staging manager that pre-reboot verification is about to start. The return + * value should be checked to decide whether it is OK to start pre-reboot verification. In + * the case of a destroyed session, {@code false} is returned and there is no need to start + * pre-reboot verification. + */ + boolean notifyStagedStartPreRebootVerification() { + synchronized (mLock) { + if (mInPreRebootVerification) { + throw new IllegalStateException("Pre-reboot verification has started"); + } + if (mDestroyed) { + return false; + } + mInPreRebootVerification = true; + return true; + } + } + + private void dispatchPendingAbandonCallback() { + final Runnable callback; + synchronized (mLock) { + callback = mPendingAbandonCallback; + mPendingAbandonCallback = null; + } + if (callback != null) { + callback.run(); + } + } + + /** + * Notified by the staging manager that pre-reboot verification has ended. Now it is safe to + * clean up the session if {@link #abandon()} has been called previously. + */ + void notifyStagedEndPreRebootVerification() { + synchronized (mLock) { + if (!mInPreRebootVerification) { + throw new IllegalStateException("Pre-reboot verification not started"); + } + mInPreRebootVerification = false; + } + dispatchPendingAbandonCallback(); + } + @Override public boolean isMultiPackage() { return params.isMultiPackage; diff --git a/services/core/java/com/android/server/pm/StagingManager.java b/services/core/java/com/android/server/pm/StagingManager.java index 0c4eaec32ba52..353459c248e5f 100644 --- a/services/core/java/com/android/server/pm/StagingManager.java +++ b/services/core/java/com/android/server/pm/StagingManager.java @@ -1028,22 +1028,12 @@ public class StagingManager { /** *

Abort committed staged session - * - *

This method must be called while holding {@link PackageInstallerSession#mLock}. - * - *

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}. - * - *

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) { + void abortCommittedSession(@NonNull PackageInstallerSession session) { int sessionId = session.sessionId; - if (session.isStagedSessionApplied()) { - Slog.w(TAG, "Cannot abort applied session : " + sessionId); - return false; + if (session.isStagedAndInTerminalState()) { + Slog.w(TAG, "Cannot abort session in final state: " + sessionId); + return; } if (!session.isDestroyed()) { throw new IllegalStateException("Committed session must be destroyed before aborting it" @@ -1051,15 +1041,7 @@ public class StagingManager { } if (getStagedSession(sessionId) == null) { Slog.w(TAG, "Session " + sessionId + " has been abandoned already"); - return false; - } - - // If pre-reboot verification is running, then return false. StagingManager will call - // abandon again when pre-reboot verification ends. - if (mPreRebootVerificationHandler.isVerificationRunning(sessionId)) { - Slog.w(TAG, "Session " + sessionId + " aborted before pre-reboot " - + "verification completed."); - return false; + return; } // A session could be marked ready once its pre-reboot verification ends @@ -1075,7 +1057,6 @@ public class StagingManager { // 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; } /** @@ -1264,8 +1245,8 @@ public class StagingManager { // TODO(b/136257624): Temporary API to let PMS communicate with StagingManager. When all // verification logic is extracted out of StagingManager into PMS, we can remove // this. - void notifyVerificationComplete(int sessionId) { - mPreRebootVerificationHandler.onPreRebootVerificationComplete(sessionId); + void notifyVerificationComplete(PackageInstallerSession session) { + mPreRebootVerificationHandler.onPreRebootVerificationComplete(session); } // TODO(b/136257624): Temporary API to let PMS communicate with StagingManager. When all @@ -1316,7 +1297,7 @@ public class StagingManager { } if (session.isDestroyed() || session.isStagedSessionFailed()) { // No point in running verification on a destroyed/failed session - onPreRebootVerificationComplete(sessionId); + onPreRebootVerificationComplete(session); return; } switch (msg.what) { @@ -1357,15 +1338,13 @@ public class StagingManager { } PackageInstallerSession session = getStagedSession(sessionId); - synchronized (mVerificationRunning) { - // Do not start verification on a session that has been abandoned - if (session == null || session.isDestroyed()) { - return; + if (session != null && session.notifyStagedStartPreRebootVerification()) { + synchronized (mVerificationRunning) { + Slog.d(TAG, "Starting preRebootVerification for session " + sessionId); + mVerificationRunning.put(sessionId, true); } - Slog.d(TAG, "Starting preRebootVerification for session " + sessionId); - mVerificationRunning.put(sessionId, true); + obtainMessage(MSG_PRE_REBOOT_VERIFICATION_START, sessionId, 0).sendToTarget(); } - obtainMessage(MSG_PRE_REBOOT_VERIFICATION_START, sessionId, 0).sendToTarget(); } private void onPreRebootVerificationFailure(PackageInstallerSession session, @@ -1376,22 +1355,18 @@ public class StagingManager { // failed on next step and staging directory for session will be deleted. } session.setStagedSessionFailed(errorCode, errorMessage); - onPreRebootVerificationComplete(session.sessionId); + onPreRebootVerificationComplete(session); } // Things to do when pre-reboot verification completes for a particular sessionId - private void onPreRebootVerificationComplete(int sessionId) { + private void onPreRebootVerificationComplete(PackageInstallerSession session) { + int sessionId = session.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(); - } + session.notifyStagedEndPreRebootVerification(); } private boolean isVerificationRunning(int sessionId) { @@ -1516,7 +1491,7 @@ public class StagingManager { // 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); + onPreRebootVerificationComplete(session); // 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 From 40af11f8d9b37e45adc5b57152ed20d8b2b93b46 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Mon, 24 Aug 2020 10:46:39 +0800 Subject: [PATCH 2/4] Merge streamValidateAndCommit() and streamAndValidateLocked() (2/n) Also move mStagingManager.checkNonOverlappingWithStagedSessions() outside the lock. It is safe to do so since it accesses members which are immutable after calls to validate[Apex|Apk]InstallLocked(). Now all calls to StagingManager are done outside the lock. Bug: 161765186 Test: atest StagedInstallTest AtomicInstallTest StagedInstallInternalTest Change-Id: I0d7cbe83a117a2d01eedc089a34778739d5fee36 --- .../server/pm/PackageInstallerSession.java | 91 ++++++++----------- 1 file changed, 40 insertions(+), 51 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index dea840a6e155f..fe0d251b056dd 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -249,6 +249,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private final PackageManagerService mPm; private final Handler mHandler; private final PackageSessionProvider mSessionProvider; + /** + * Note all calls must be done outside {@link #mLock} to prevent lock inversion. + */ private final StagingManager mStagingManager; final int sessionId; @@ -1468,26 +1471,50 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // TODO(patb): since the work done here for a parent session in a multi-package install is // mostly superficial, consider splitting this method for the parent and // single / child sessions. - synchronized (mLock) { - if (mCommitted) { - return true; + try { + synchronized (mLock) { + if (mCommitted) { + return true; + } + // Read transfers from the original owner stay open, but as the session's data + // cannot be modified anymore, there is no leak of information. For staged sessions, + // further validation is performed by the staging manager. + if (!params.isMultiPackage) { + if (!prepareDataLoaderLocked()) { + return false; + } + + if (isApexInstallation()) { + validateApexInstallLocked(); + } else { + validateApkInstallLocked(); + } + } } - if (!streamAndValidateLocked()) { - return false; + if (params.isStaged) { + mStagingManager.checkNonOverlappingWithStagedSessions(this); } - // Client staging is fully done at this point - mClientProgress = 1f; - computeProgressLocked(true); + synchronized (mLock) { + // Client staging is fully done at this point + mClientProgress = 1f; + computeProgressLocked(true); - // This ongoing commit should keep session active, even though client - // will probably close their end. - mActiveCount.incrementAndGet(); + // This ongoing commit should keep session active, even though client + // will probably close their end. + mActiveCount.incrementAndGet(); - mCommitted = true; + mCommitted = true; + } + return true; + } catch (PackageManagerException e) { + throw onSessionValidationFailure(e); + } catch (Throwable e) { + // Convert all exceptions into package manager exceptions as only those are handled + // in the code above. + throw onSessionValidationFailure(new PackageManagerException(e)); } - return true; } @GuardedBy("mLock") @@ -1525,44 +1552,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } - /** - * Prepare DataLoader and stream content for DataLoader sessions. - * Validate the contents of all session. - * - * @return false if the data loader could not be prepared. - * @throws PackageManagerException when an unrecoverable exception is encountered - */ - @GuardedBy("mLock") - private boolean streamAndValidateLocked() throws PackageManagerException { - try { - // Read transfers from the original owner stay open, but as the session's data cannot - // be modified anymore, there is no leak of information. For staged sessions, further - // validation is performed by the staging manager. - if (!params.isMultiPackage) { - if (!prepareDataLoaderLocked()) { - return false; - } - - if (isApexInstallation()) { - validateApexInstallLocked(); - } else { - validateApkInstallLocked(); - } - } - - if (params.isStaged) { - mStagingManager.checkNonOverlappingWithStagedSessions(this); - } - return true; - } catch (PackageManagerException e) { - throw onSessionValidationFailure(e); - } catch (Throwable e) { - // Convert all exceptions into package manager exceptions as only those are handled - // in the code above. - throw onSessionValidationFailure(new PackageManagerException(e)); - } - } - private PackageManagerException onSessionValidationFailure(PackageManagerException e) { onSessionValidationFailure(e.error, ExceptionUtils.getCompleteMessage(e)); return e; From 061a28c7a8a6d46a7af52b56cdb81939a748c07d Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 21 Aug 2020 15:47:38 +0800 Subject: [PATCH 3/4] Remove unused code (3/n) Bug: 161765186 Test: m Change-Id: I66eda02e797857a133bc023014b9f2aea1fc8da6 --- .../com/android/server/pm/StagingManager.java | 20 ++----------------- 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/services/core/java/com/android/server/pm/StagingManager.java b/services/core/java/com/android/server/pm/StagingManager.java index 353459c248e5f..462b215353717 100644 --- a/services/core/java/com/android/server/pm/StagingManager.java +++ b/services/core/java/com/android/server/pm/StagingManager.java @@ -60,7 +60,6 @@ import android.util.ArraySet; 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; @@ -1260,8 +1259,6 @@ public class StagingManager { // 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); @@ -1339,10 +1336,7 @@ public class StagingManager { PackageInstallerSession session = getStagedSession(sessionId); if (session != null && session.notifyStagedStartPreRebootVerification()) { - synchronized (mVerificationRunning) { - Slog.d(TAG, "Starting preRebootVerification for session " + sessionId); - mVerificationRunning.put(sessionId, true); - } + Slog.d(TAG, "Starting preRebootVerification for session " + sessionId); obtainMessage(MSG_PRE_REBOOT_VERIFICATION_START, sessionId, 0).sendToTarget(); } } @@ -1361,20 +1355,10 @@ public class StagingManager { // Things to do when pre-reboot verification completes for a particular sessionId private void onPreRebootVerificationComplete(PackageInstallerSession session) { int sessionId = session.sessionId; - // Remove it from mVerificationRunning so that verification is considered complete - synchronized (mVerificationRunning) { - Slog.d(TAG, "Stopping preRebootVerification for session " + sessionId); - mVerificationRunning.delete(sessionId); - } + Slog.d(TAG, "Stopping preRebootVerification for session " + sessionId); session.notifyStagedEndPreRebootVerification(); } - 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(); } From 6acd2bda7f1ce403f6a8b28878831c8bfd8f7974 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 21 Aug 2020 18:12:53 +0800 Subject: [PATCH 4/4] Ensure no state changes after mDestroyed=true (4/n) mDestroyed=true is a terminal state. No more state changes should happen after reaching the final state. Since abandon() can be called at any moment, checking mDestroyed and setting mCommitted/mRelinquished must be done in the same synchronization block to prevent state changes after mDestroyed=true. Bug: 161765186 Test: atest StagedInstallTest AtomicInstallTest StagedInstallInternalTest Change-Id: I301ab7d89332cfbf14bd69b7b2601fe26bd0eaf3 --- .../server/pm/PackageInstallerSession.java | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index fe0d251b056dd..f7e9604724c4b 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -1497,6 +1497,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } synchronized (mLock) { + if (mDestroyed) { + throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, + "Session destroyed"); + } // Client staging is fully done at this point mClientProgress = 1f; computeProgressLocked(true); @@ -1839,21 +1843,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throws PackageManagerException { assertNotLocked("makeSessionActive"); - synchronized (mLock) { - if (mRelinquished) { - throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, - "Session relinquished"); - } - if (mDestroyed) { - throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, - "Session destroyed"); - } - if (!mSealed) { - throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, - "Session not sealed"); - } - } - // TODO(b/159331446): Move this to makeSessionActiveForInstall and update javadoc if (!params.isMultiPackage && needToAskForPermissions()) { // User needs to confirm installation; @@ -1883,6 +1872,19 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @GuardedBy("mLock") private PackageManagerService.VerificationParams makeVerificationParamsLocked() throws PackageManagerException { + if (mRelinquished) { + throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, + "Session relinquished"); + } + if (mDestroyed) { + throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, + "Session destroyed"); + } + if (!mSealed) { + throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, + "Session not sealed"); + } + // TODO(b/136257624): Some logic in this if block probably belongs in // makeInstallParams(). if (!params.isMultiPackage && !isApexInstallation()) {