From 731bd965fb5e0f62dc703d05983baef8e0a0f4e7 Mon Sep 17 00:00:00 2001 From: Mohammad Samiul Islam Date: Thu, 23 Apr 2020 16:23:21 +0100 Subject: [PATCH 1/2] Persist destroyed staged sessions until they are cleaned up Currently, when we abandon a staged session we mark it as destroyed and then immediately clean it up. Cleaning up a staged session immediately causes racing condition with pre-reboot verification. In order to avoid the racing condition, we want to delay cleanup of staged session until it is safe to do so. This means, the system will be carrying around destroyed staged sessions internally. Since there is now a gap between when a session is destroyed and when it is cleaned up, the user can reboot in this window. As such, we need to persist the mDestroyed field of session so that we know session is destroyed after reboot and act accordingly. Also, once a session is destroyed, theoretically it doesn't exist. Carrying it around internally is an implementation details which shouldn't be exposed externally. As such, we filter out destroyed sessions before surfacing them to users. Bug: 145925842 Test: atest PackageInstallerSessionTest Test: atest StagedInstallTest Change-Id: I4ede6b7a4b5d861e5c73f13884c7aa86cf7633a2 --- .../server/pm/PackageInstallerService.java | 7 ++++--- .../server/pm/PackageInstallerSession.java | 17 ++++++++++++++--- .../com/android/server/pm/StagingManager.java | 3 +++ .../server/pm/PackageInstallerSessionTest.java | 1 + 4 files changed, 22 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerService.java b/services/core/java/com/android/server/pm/PackageInstallerService.java index 57908f3a4dacd..d5b73cd72f3af 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerService.java +++ b/services/core/java/com/android/server/pm/PackageInstallerService.java @@ -666,7 +666,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) { @@ -816,7 +816,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; } @@ -837,7 +837,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)); } } diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index f07fa501d3a8e..3a81b54305318 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -187,6 +187,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"; @@ -525,7 +526,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) { @@ -571,6 +572,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mPrepared = prepared; mCommitted = committed; + mDestroyed = destroyed; mStagedSessionReady = isReady; mStagedSessionFailed = isFailed; mStagedSessionApplied = isApplied; @@ -705,6 +707,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) { @@ -3065,7 +3074,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; } @@ -3091,6 +3100,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); @@ -3220,6 +3230,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); @@ -3341,7 +3352,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); } } diff --git a/services/core/java/com/android/server/pm/StagingManager.java b/services/core/java/com/android/server/pm/StagingManager.java index 8f6bd026a9bda..cf2c2db2f3d19 100644 --- a/services/core/java/com/android/server/pm/StagingManager.java +++ b/services/core/java/com/android/server/pm/StagingManager.java @@ -137,6 +137,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)); } } diff --git a/services/tests/servicestests/src/com/android/server/pm/PackageInstallerSessionTest.java b/services/tests/servicestests/src/com/android/server/pm/PackageInstallerSessionTest.java index d4edab44bae30..63d797e9b95c9 100644 --- a/services/tests/servicestests/src/com/android/server/pm/PackageInstallerSessionTest.java +++ b/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, From a0623e290e239dfb04f0956d76839193a0bb0a81 Mon Sep 17 00:00:00 2001 From: Mohammad Samiul Islam Date: Fri, 24 Apr 2020 09:26:19 +0100 Subject: [PATCH 2/2] Fix racing condition between session abandonment and its verification This CL prevents a committed staged session from being cleaned up if it unsafe for some reason. The session is marked destroyed and StagingManager ensures destroyed session do not get installed anymore. Meanwhile, when it becomes safe for session to be cleaned up again, StagingManager will call abandon again to clean it up. Bug: 145925842 Test: atest StagedInstallTest Test: atest StagedInstallTest#testAbandonStagedApkBeforeReady Test: atest StagedInstallTest#testNoSessionUpdatedBroadcastSentForStagedSessionAbandon Change-Id: Idf0adfb54afcd411c5ea1c49d7fcf5c0cddfcb32 --- .../server/pm/PackageInstallerService.java | 2 +- .../server/pm/PackageInstallerSession.java | 33 +++- .../com/android/server/pm/StagingManager.java | 173 ++++++++++++++---- 3 files changed, 167 insertions(+), 41 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerService.java b/services/core/java/com/android/server/pm/PackageInstallerService.java index d5b73cd72f3af..630342c75864c 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerService.java +++ b/services/core/java/com/android/server/pm/PackageInstallerService.java @@ -1248,7 +1248,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( diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 3a81b54305318..d9612ebddbcf2 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -1068,6 +1068,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}. */ @@ -2453,7 +2466,13 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { + mParentSessionId + " and may not be abandoned directly."); } synchronized (mLock) { - assertCallerIsOwnerOrRootLocked(); + 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 @@ -2463,11 +2482,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return; } if (mCommitted && params.isStaged) { - synchronized (mLock) { - mDestroyed = true; + 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(); } @@ -2803,6 +2823,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; @@ -2816,6 +2837,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; @@ -2830,6 +2852,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; diff --git a/services/core/java/com/android/server/pm/StagingManager.java b/services/core/java/com/android/server/pm/StagingManager.java index cf2c2db2f3d19..fdb75ffeaf800 100644 --- a/services/core/java/com/android/server/pm/StagingManager.java +++ b/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; @@ -205,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); } } @@ -757,6 +758,8 @@ public class StagingManager { + session.sessionId + " [" + errorMessage + "]"); session.setStagedSessionFailed( SessionInfo.STAGED_SESSION_VERIFICATION_FAILED, errorMessage); + mPreRebootVerificationHandler.onPreRebootVerificationComplete( + session.sessionId); return; } mPreRebootVerificationHandler.notifyPreRebootVerification_Apk_Complete( @@ -840,7 +843,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()) { @@ -903,27 +907,68 @@ public class StagingManager { } } - void abortCommittedSession(@NonNull PackageInstallerSession session) { + /** + *

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) { + int sessionId = session.sessionId; if (session.isStagedSessionApplied()) { - Slog.w(TAG, "Cannot abort applied session : " + session.sessionId); - return; + Slog.w(TAG, "Cannot abort applied session : " + sessionId); + 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 (hasApex) { - 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"); - return; - } - try { - mApexManager.abortStagedSession(session.sessionId); - } catch (Exception ignore) { + // 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; + } + + // 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."); + } else { + mApexManager.abortStagedSession(session.sessionId); + } + } 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) { @@ -1002,6 +1047,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. @@ -1084,10 +1134,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); @@ -1115,13 +1175,15 @@ public class StagingManager { @Override public void handleMessage(Message msg) { final int sessionId = msg.arg1; - final PackageInstallerSession session; - synchronized (mStagedSessions) { - session = mStagedSessions.get(sessionId); - } - // Maybe session was aborted before pre-reboot verification was complete + final PackageInstallerSession session = getStagedSession(sessionId); 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) { @@ -1160,9 +1222,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(); } @@ -1181,8 +1274,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 @@ -1228,6 +1319,7 @@ public class StagingManager { } } catch (PackageManagerException e) { session.setStagedSessionFailed(e.error, e.getMessage()); + onPreRebootVerificationComplete(session.sessionId); return; } } @@ -1256,6 +1348,7 @@ public class StagingManager { // TODO(b/118865310): abort the session on apexd. } catch (PackageManagerException e) { session.setStagedSessionFailed(e.error, e.getMessage()); + onPreRebootVerificationComplete(session.sessionId); } } @@ -1278,9 +1371,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: @@ -1292,15 +1394,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(); - final boolean hasApex = sessionContainsApex(session); - 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()); + if (session.isStagedSessionReady()) { + final boolean hasApex = sessionContainsApex(session); + if (hasApex) { + try { + mApexManager.markStagedSessionReady(session.sessionId); + } catch (PackageManagerException e) { + session.setStagedSessionFailed(e.error, e.getMessage()); + return; + } + } } } }