From 43f1af9dd9ce7873d7ede94cc72fc59c7c71283d Mon Sep 17 00:00:00 2001 From: shafik Date: Thu, 14 Mar 2019 15:14:00 +0000 Subject: [PATCH] Enforce multipackage installs have consistent rollback params PackageInstallerSession: * Add multipackage consistency check to sealAndValidateLocked. The check asserts all child sessions match parent session in respect to: * Staged parameter * Rollback enabled parameter * Remove the staged conistency check from addChildSessionId * Remove sealAndValidateLocked from constructor because it was used only when sessions where loaded (from XML file) and there's no guarantee that child sessions will be loaded before parent. Instead, mark the session as ShouldBeSealed. PackageInstallerService: * To make sure relevant sessions are sealed and validated after they are loaded from an XML file, iterate through the loaded seasions and seal and validate those marked as ShouldBeSealed. Sessions that do not pass the validation are destroyed. Test: atest StagedRollbackTest Test: atest CtsAtomicInstallTestCases Test: atest PackageInstallerSessionTest Fixes: 127765168 Fixes: 124215984 Change-Id: I8f152332cadb0f6c9063264f27668821fad1cec7 --- .../android/content/pm/PackageInstaller.java | 14 ++ .../android/content/pm/PackageManager.java | 8 + .../server/pm/PackageInstallerService.java | 5 + .../server/pm/PackageInstallerSession.java | 143 ++++++++++++++---- 4 files changed, 140 insertions(+), 30 deletions(-) diff --git a/core/java/android/content/pm/PackageInstaller.java b/core/java/android/content/pm/PackageInstaller.java index 1e6cea39b44dd..5d6867e143938 100644 --- a/core/java/android/content/pm/PackageInstaller.java +++ b/core/java/android/content/pm/PackageInstaller.java @@ -1211,6 +1211,9 @@ public class PackageInstaller { * Adds a session ID to the set of sessions that will be committed atomically * when this session is committed. * + *

If the parent is staged or has rollback enabled, all children must have + * the same properties. + * * @param sessionId the session ID to add to this multi-package session. */ public void addChildSessionId(int sessionId) { @@ -1480,6 +1483,9 @@ public class PackageInstaller { /** * Request that rollbacks be enabled or disabled for the given upgrade. * + *

If the parent session is staged or has rollback enabled, all children sessions + * must have the same properties. + * * @param enable set to {@code true} to enable, {@code false} to disable * @hide */ @@ -1607,6 +1613,9 @@ public class PackageInstaller { * multi-package. In that case, if any of the children sessions fail to install at reboot, * all the other children sessions are aborted as well. * + *

If the parent session is staged or has rollback enabled, all children sessions + * must have the same properties. + * * {@hide} */ @SystemApi @TestApi @@ -1626,6 +1635,11 @@ public class PackageInstaller { installFlags |= PackageManager.INSTALL_APEX; } + /** @hide */ + public boolean getEnableRollback() { + return (installFlags & PackageManager.INSTALL_ENABLE_ROLLBACK) != 0; + } + /** {@hide} */ public void dump(IndentingPrintWriter pw) { pw.printPair("mode", mode); diff --git a/core/java/android/content/pm/PackageManager.java b/core/java/android/content/pm/PackageManager.java index b190b34aa03fd..254238f68afe3 100644 --- a/core/java/android/content/pm/PackageManager.java +++ b/core/java/android/content/pm/PackageManager.java @@ -1397,6 +1397,14 @@ public abstract class PackageManager { */ public static final int INSTALL_FAILED_OTHER_STAGED_SESSION_IN_PROGRESS = -119; + /** + * Installation failed return code: one of the child sessions does not match the parent session + * in respect to staged or rollback enabled parameters. + * + * @hide + */ + public static final int INSTALL_FAILED_MULTIPACKAGE_INCONSISTENCY = -120; + /** @hide */ @IntDef(flag = true, prefix = { "DELETE_" }, value = { DELETE_KEEP_DATA, diff --git a/services/core/java/com/android/server/pm/PackageInstallerService.java b/services/core/java/com/android/server/pm/PackageInstallerService.java index ad17549d7448e..7c04cddcaad2c 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerService.java +++ b/services/core/java/com/android/server/pm/PackageInstallerService.java @@ -396,6 +396,11 @@ public class PackageInstallerService extends IPackageInstaller.Stub implements } finally { IoUtils.closeQuietly(fis); } + // After all of the sessions were loaded, they are ready to be sealed and validated + for (int i = 0; i < mSessions.size(); ++i) { + PackageInstallerSession session = mSessions.valueAt(i); + session.sealAndValidateIfNecessary(); + } } @GuardedBy("mSessions") diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index f1d4524cccac3..5d539a4b1dda7 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -231,6 +231,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @GuardedBy("mLock") private boolean mSealed = false; @GuardedBy("mLock") + private boolean mShouldBeSealed = false; + @GuardedBy("mLock") private boolean mCommitted = false; @GuardedBy("mLock") private boolean mRelinquished = false; @@ -430,6 +432,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { this.updatedMillis = createdMillis; this.stageDir = stageDir; this.stageCid = stageCid; + this.mShouldBeSealed = sealed; if (childSessionIds != null) { for (int childSessionId : childSessionIds) { mChildSessionIds.put(childSessionId, 0); @@ -450,16 +453,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { mStagedSessionErrorCode = stagedSessionErrorCode; mStagedSessionErrorMessage = stagedSessionErrorMessage != null ? stagedSessionErrorMessage : ""; - if (sealed) { - synchronized (mLock) { - try { - sealAndValidateLocked(); - } catch (PackageManagerException | IOException e) { - destroyInternal(); - throw new IllegalArgumentException(e); - } - } - } } public SessionInfo generateInfo() { @@ -932,6 +925,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @NonNull IntentSender statusReceiver, boolean forTransfer) { Preconditions.checkNotNull(statusReceiver); + List childSessions = getChildSessions(); + final boolean wasSealed; synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); @@ -963,7 +958,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { wasSealed = mSealed; if (!mSealed) { try { - sealAndValidateLocked(); + sealAndValidateLocked(childSessions); } catch (IOException e) { throw new IllegalArgumentException(e); } catch (PackageManagerException e) { @@ -994,21 +989,91 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return true; } + /** Return a list of child sessions or null if the session is not multipackage + * + *

This method is handy to prevent potential deadlocks (b/123391593) + */ + private @Nullable List getChildSessions() { + List childSessions = null; + if (isMultiPackage()) { + final int[] childSessionIds = getChildSessionIds(); + childSessions = new ArrayList<>(childSessionIds.length); + for (int childSessionId : childSessionIds) { + childSessions.add(mSessionProvider.getSession(childSessionId)); + } + } + return childSessions; + } + + /** + * Assert multipackage install has consistent sessions. + * + * @throws PackageManagerException if child sessions don't match parent session + * in respect to staged and enable rollback parameters. + */ + @GuardedBy("mLock") + private void assertMultiPackageConsistencyLocked( + @NonNull List childSessions) throws PackageManagerException { + for (PackageInstallerSession childSession : childSessions) { + // It might be that the parent session is loaded before all of it's child sessions are, + // e.g. when reading sessions from XML. Those sessions will be null here, and their + // conformance with the multipackage params will be checked when they're loaded. + if (childSession == null) { + continue; + } + assertConsistencyWithLocked(childSession); + } + } + + /** + * Assert consistency with the given session. + * + * @throws PackageManagerException if other sessions doesn't match this session + * in respect to staged and enable rollback parameters. + */ + @GuardedBy("mLock") + private void assertConsistencyWithLocked(PackageInstallerSession other) + throws PackageManagerException { + // Session groups must be consistent wrt to isStaged parameter. Non-staging session + // cannot be grouped with staging sessions. + if (this.params.isStaged != other.params.isStaged) { + throw new PackageManagerException( + PackageManager.INSTALL_FAILED_MULTIPACKAGE_INCONSISTENCY, + "Multipackage Inconsistency: session " + other.sessionId + + " and session " + sessionId + + " have inconsistent staged settings"); + } + if (this.params.getEnableRollback() != other.params.getEnableRollback()) { + throw new PackageManagerException( + PackageManager.INSTALL_FAILED_MULTIPACKAGE_INCONSISTENCY, + "Multipackage Inconsistency: session " + other.sessionId + + " and session " + sessionId + + " have inconsistent rollback settings"); + } + } + /** * Seal the session to prevent further modification and validate the contents of it. * *

The session will be sealed after calling this method even if it failed. * + * @param childSessions the child sessions of a multipackage that will be checked for + * consistency. Can be null if session is not multipackage. * @throws PackageManagerException if the session was sealed but something went wrong. If the * session was sealed this is the only possible exception. */ @GuardedBy("mLock") - private void sealAndValidateLocked() throws PackageManagerException, IOException { + private void sealAndValidateLocked(List childSessions) + throws PackageManagerException, IOException { assertNoWriteFileTransfersOpenLocked(); assertPreparedAndNotDestroyedLocked("sealing of session"); mSealed = true; + if (childSessions != null) { + assertMultiPackageConsistencyLocked(childSessions); + } + if (params.isStaged) { final PackageInstallerSession activeSession = mStagingManager.getActiveSession(); final boolean anotherSessionAlreadyInProgress = @@ -1048,6 +1113,38 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } + /** + * If session should be sealed, then it's sealed to prevent further modification + * and then it's validated. + * + * If the session was sealed but something went wrong then it's destroyed. + * + *

This is meant to be called after all of the sessions are loaded and added to + * PackageInstallerService + */ + void sealAndValidateIfNecessary() { + synchronized (mLock) { + if (!mShouldBeSealed) { + return; + } + } + List childSessions = getChildSessions(); + synchronized (mLock) { + try { + sealAndValidateLocked(childSessions); + } catch (IOException e) { + throw new IllegalStateException(e); + } catch (PackageManagerException e) { + Slog.e(TAG, "Package not valid", e); + // Session is sealed but could not be verified, we need to destroy it. + destroyInternal(); + // Dispatch message to remove session from PackageInstallerService + dispatchSessionFinished( + e.error, ExceptionUtils.getCompleteMessage(e), null); + } + } + } + /** Update the timestamp of when the staged session last changed state */ public void markUpdated() { synchronized (mLock) { @@ -1076,12 +1173,14 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { throw new SecurityException("Can only transfer sessions that use public options"); } + List childSessions = getChildSessions(); + synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotSealedLocked("transfer"); try { - sealAndValidateLocked(); + sealAndValidateLocked(childSessions); } catch (IOException e) { throw new IllegalStateException(e); } catch (PackageManagerException e) { @@ -1132,14 +1231,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // outside of the lock, because reading the child // sessions with the lock held could lead to deadlock // (b/123391593). - List childSessions = null; - if (isMultiPackage()) { - final int[] childSessionIds = getChildSessionIds(); - childSessions = new ArrayList<>(childSessionIds.length); - for (int childSessionId : childSessionIds) { - childSessions.add(mSessionProvider.getSession(childSessionId)); - } - } + List childSessions = getChildSessions(); try { synchronized (mLock) { @@ -1965,15 +2057,6 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { + " does not exist"), false, true).rethrowAsRuntimeException(); } - // Session groups must be consistent wrt to isStaged parameter. Non-staging session - // cannot be grouped with staging sessions. - if (this.params.isStaged ^ childSession.params.isStaged) { - throw new RemoteException("Unable to add child.", - new PackageManagerException("Child session " + childSessionId - + " and parent session " + this.sessionId + " do not have consistent" - + " staging session settings."), - false, true); - } synchronized (mLock) { assertCallerIsOwnerOrRootLocked(); assertPreparedAndNotSealedLocked("addChildSessionId");