From 4756970d3f868d64eb8de3cd352e17e66dc31fe9 Mon Sep 17 00:00:00 2001 From: Richard Uhler Date: Thu, 2 May 2019 12:36:39 +0100 Subject: [PATCH] Fix handling of multi-package enable rollback failures. Failure to enable rollback for one of the packages in a multi-package install should fail to enable rollback for all packages in the multi-package install. That was not the case prior to this CL, which could lead to partial rollbacks. Keep track of how many child sessions we expect to enable rollback for and double check that we succeeded before making the rollback available. This involves some cleanup of how we track parent vs child sessions when enabling rollback. Bug: 128656191 Test: atest RollbackTest, with new test added. Change-Id: I737896cdc1915396748c5c1959b5397af793258a --- .../rollback/RollbackManagerServiceImpl.java | 210 +++++++++++------- .../server/rollback/RollbackStore.java | 5 +- .../android/tests/rollback/RollbackTest.java | 36 +++ 3 files changed, 170 insertions(+), 81 deletions(-) diff --git a/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java b/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java index db2c742b8d4ea..b3e25c68dddb3 100644 --- a/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java +++ b/services/core/java/com/android/server/rollback/RollbackManagerServiceImpl.java @@ -50,6 +50,7 @@ import android.os.UserManager; // out of order to avoid merge conflict import android.os.SystemClock; import android.os.UserHandle; import android.provider.DeviceConfig; +import android.util.ArraySet; import android.util.IntArray; import android.util.Log; import android.util.SparseBooleanArray; @@ -69,11 +70,9 @@ import java.security.SecureRandom; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; -import java.util.Map; import java.util.Random; import java.util.Set; import java.util.concurrent.LinkedBlockingQueue; @@ -107,15 +106,9 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { @GuardedBy("mLock") private final SparseBooleanArray mAllocatedRollbackIds = new SparseBooleanArray(); - // Package rollback data for rollback-enabled installs that have not yet - // been committed. Maps from sessionId to rollback data. + // Package rollback data for rollbacks we are in the process of enabling. @GuardedBy("mLock") - private final Map mPendingRollbacks = new HashMap<>(); - - // Map from child session id's for enabled rollbacks to their - // corresponding parent session ids. - @GuardedBy("mLock") - private final Map mChildSessions = new HashMap<>(); + private final Set mNewRollbacks = new ArraySet<>(); // The list of all rollbacks, including available and committed rollbacks. // This list is null until the rollback data has been loaded. @@ -136,7 +129,6 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { // No need for guarding with lock because value is only accessed in handler thread. private long mRelativeBootTime = calculateRelativeBootTime(); - RollbackManagerServiceImpl(Context context) { mContext = context; // Note that we're calling onStart here because this object is only constructed on @@ -848,7 +840,6 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { // TODO: It would be nice if package manager or package installer told // us the session directly, rather than have to search for it // ourselves. - PackageInstaller.SessionInfo session = null; // getAllSessions only returns sessions for the associated user. // Create a context with the right user so we can find the matching @@ -859,7 +850,8 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { return false; } - int parentSessionId = -1; + PackageInstaller.SessionInfo parentSession = null; + PackageInstaller.SessionInfo packageSession = null; PackageInstaller installer = context.getPackageManager().getPackageInstaller(); for (PackageInstaller.SessionInfo info : installer.getAllSessions()) { if (info.isMultiPackage()) { @@ -867,21 +859,21 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { PackageInstaller.SessionInfo child = installer.getSessionInfo(childId); if (sessionMatchesForEnableRollback(child, installFlags, newPackageCodePath)) { // TODO: Check we only have one matching session? - parentSessionId = info.getSessionId(); - session = child; + parentSession = info; + packageSession = child; break; } } } else if (sessionMatchesForEnableRollback(info, installFlags, newPackageCodePath)) { // TODO: Check we only have one matching session? - parentSessionId = info.getSessionId(); - session = info; + parentSession = info; + packageSession = info; break; } } - if (session == null) { - Log.e(TAG, "Unable to find session id for enabled rollback."); + if (parentSession == null || packageSession == null) { + Log.e(TAG, "Unable to find session for enabled rollback."); return false; } @@ -893,7 +885,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { ensureRollbackDataLoadedLocked(); for (int i = 0; i < mRollbacks.size(); ++i) { RollbackData data = mRollbacks.get(i); - if (data.apkSessionId == parentSessionId) { + if (data.apkSessionId == parentSession.getSessionId()) { rd = data; break; } @@ -906,7 +898,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { PackageParser.PackageLite newPackage = null; try { newPackage = PackageParser.parsePackageLite( - new File(session.resolvedBaseCodePath), 0); + new File(packageSession.resolvedBaseCodePath), 0); } catch (PackageParser.PackageParserException e) { Log.e(TAG, "Unable to parse new package", e); return false; @@ -924,16 +916,32 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { return false; } - return enableRollbackForSession(session, installedUsers, true); + NewRollback newRollback; + synchronized (mLock) { + // See if we already have a NewRollback that contains this package + // session. If not, create a NewRollback for the parent session + // that we will use for all the packages in the session. + newRollback = getNewRollbackForPackageSessionLocked(packageSession.getSessionId()); + if (newRollback == null) { + newRollback = createNewRollbackLocked(parentSession); + mNewRollbacks.add(newRollback); + } + } + + return enableRollbackForPackageSession(newRollback.data, packageSession, + installedUsers, /* snapshotUserData*/ true); } /** * Do code and userdata backups to enable rollback of the given session. * In case of multiPackage sessions, session should be one of * the child sessions, not the parent session. + * + * @return true on success, false on failure. */ - private boolean enableRollbackForSession(PackageInstaller.SessionInfo session, - @NonNull int[] installedUsers, boolean snapshotUserData) { + private boolean enableRollbackForPackageSession(RollbackData data, + PackageInstaller.SessionInfo session, @NonNull int[] installedUsers, + boolean snapshotUserData) { // TODO: Don't attempt to enable rollback for split installs. final int installFlags = session.installFlags; if ((installFlags & PackageManager.INSTALL_ENABLE_ROLLBACK) == 0) { @@ -988,41 +996,14 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { VersionedPackage installedVersion = new VersionedPackage(packageName, pkgInfo.getLongVersionCode()); - PackageRollbackInfo info = new PackageRollbackInfo(newVersion, installedVersion, + PackageRollbackInfo packageRollbackInfo = new PackageRollbackInfo( + newVersion, installedVersion, new IntArray() /* pendingBackups */, new ArrayList<>() /* pendingRestores */, isApex, IntArray.wrap(installedUsers), new SparseLongArray() /* ceSnapshotInodes */); - RollbackData data; - try { - int childSessionId = session.getSessionId(); - int parentSessionId = session.getParentSessionId(); - if (parentSessionId == PackageInstaller.SessionInfo.INVALID_ID) { - parentSessionId = childSessionId; - } - - synchronized (mLock) { - // TODO: no need to add to mChildSessions if childSessionId is - // the same as parentSessionId. - mChildSessions.put(childSessionId, parentSessionId); - data = mPendingRollbacks.get(parentSessionId); - if (data == null) { - int rollbackId = allocateRollbackIdLocked(); - if (session.isStaged()) { - data = mRollbackStore.createStagedRollback(rollbackId, parentSessionId); - } else { - data = mRollbackStore.createNonStagedRollback(rollbackId); - } - mPendingRollbacks.put(parentSessionId, data); - } - data.info.getPackages().add(info); - } - } catch (IOException e) { - Log.e(TAG, "Unable to create rollback for " + packageName, e); - return false; - } if (snapshotUserData && !isApex) { - mAppDataRollbackHelper.snapshotAppData(data.info.getRollbackId(), info); + mAppDataRollbackHelper.snapshotAppData(data.info.getRollbackId(), packageRollbackInfo); } try { @@ -1037,6 +1018,10 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { Log.e(TAG, "Unable to copy package for rollback for " + packageName, e); return false; } + + synchronized (mLock) { + data.info.getPackages().add(packageRollbackInfo); + } return true; } @@ -1107,8 +1092,14 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { return; } + NewRollback newRollback; + synchronized (mLock) { + newRollback = createNewRollbackLocked(session); + } + if (!session.isMultiPackage()) { - if (!enableRollbackForSession(session, new int[0], false)) { + if (!enableRollbackForPackageSession(newRollback.data, session, + new int[0], /* snapshotUserData */ false)) { Log.e(TAG, "Unable to enable rollback for session: " + sessionId); result.offer(false); return; @@ -1122,7 +1113,8 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { result.offer(false); return; } - if (!enableRollbackForSession(childSession, new int[0], false)) { + if (!enableRollbackForPackageSession(newRollback.data, childSession, + new int[0], /* snapshotUserData */ false)) { Log.e(TAG, "Unable to enable rollback for session: " + sessionId); result.offer(false); return; @@ -1130,8 +1122,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { } } - completeEnableRollback(sessionId, true); - result.offer(true); + result.offer(completeEnableRollback(newRollback, true) != null); }); try { @@ -1262,9 +1253,19 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { @Override public void onFinished(int sessionId, boolean success) { - RollbackData rollback = completeEnableRollback(sessionId, success); - if (rollback != null && !rollback.isStaged()) { - makeRollbackAvailable(rollback); + NewRollback newRollback; + synchronized (mLock) { + newRollback = getNewRollbackForPackageSessionLocked(sessionId); + if (newRollback != null) { + mNewRollbacks.remove(newRollback); + } + } + + if (newRollback != null) { + RollbackData rollback = completeEnableRollback(newRollback, success); + if (rollback != null && !rollback.isStaged()) { + makeRollbackAvailable(rollback); + } } } } @@ -1274,25 +1275,22 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { * This should be called after rollback has been enabled for all packages * in the rollback. It does not make the rollback available yet. * - * @return the rollback data for a successfully enable-completed rollback. + * @return the rollback data for a successfully enable-completed rollback, + * or null on error. */ - private RollbackData completeEnableRollback(int sessionId, boolean success) { - RollbackData data = null; - synchronized (mLock) { - Integer parentSessionId = mChildSessions.remove(sessionId); - if (parentSessionId != null) { - sessionId = parentSessionId; - } - - data = mPendingRollbacks.remove(sessionId); - } - - if (data == null) { + private RollbackData completeEnableRollback(NewRollback newRollback, boolean success) { + RollbackData data = newRollback.data; + if (!success) { + // The install session was aborted, clean up the pending install. + deleteRollback(data); return null; } - if (!success) { - // The install session was aborted, clean up the pending install. + // It's safe to access data.info outside a synchronized block because + // this is running on the handler thread and all changes to the + // data.info occur on the handler thread. + if (data.info.getPackages().size() != newRollback.packageSessionIds.length) { + Log.e(TAG, "Failed to enable rollback for all packages in session."); deleteRollback(data); return null; } @@ -1376,7 +1374,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { } @GuardedBy("mLock") - private int allocateRollbackIdLocked() throws IOException { + private int allocateRollbackIdLocked() { int n = 0; int rollbackId; do { @@ -1387,7 +1385,7 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { } } while (n++ < 32); - throw new IOException("Failed to allocate rollback ID"); + throw new IllegalStateException("Failed to allocate rollback ID"); } private void deleteRollback(RollbackData rollbackData) { @@ -1462,4 +1460,60 @@ class RollbackManagerServiceImpl extends IRollbackManager.Stub { + Manifest.permission.TEST_MANAGE_ROLLBACKS); } } + + private static class NewRollback { + public final RollbackData data; + + /** + * Session ids for all packages in the install. + * For multi-package sessions, this is the list of child session ids. + * For normal sessions, this list is a single element with the normal + * session id. + */ + public final int[] packageSessionIds; + + NewRollback(RollbackData data, int[] packageSessionIds) { + this.data = data; + this.packageSessionIds = packageSessionIds; + } + } + + NewRollback createNewRollbackLocked(PackageInstaller.SessionInfo parentSession) { + int rollbackId = allocateRollbackIdLocked(); + final RollbackData data; + int parentSessionId = parentSession.getSessionId(); + + if (parentSession.isStaged()) { + data = mRollbackStore.createStagedRollback(rollbackId, parentSessionId); + } else { + data = mRollbackStore.createNonStagedRollback(rollbackId); + } + + int[] packageSessionIds; + if (parentSession.isMultiPackage()) { + packageSessionIds = parentSession.getChildSessionIds(); + } else { + packageSessionIds = new int[]{parentSessionId}; + } + + return new NewRollback(data, packageSessionIds); + } + + /** + * Returns the NewRollback associated with the given package session. + * Returns null if no NewRollback is found for the given package + * session. + */ + NewRollback getNewRollbackForPackageSessionLocked(int packageSessionId) { + // We expect mNewRollbacks to be a very small list; linear search + // should be plenty fast. + for (NewRollback newRollbackData : mNewRollbacks) { + for (int id : newRollbackData.packageSessionIds) { + if (id == packageSessionId) { + return newRollbackData; + } + } + } + return null; + } } diff --git a/services/core/java/com/android/server/rollback/RollbackStore.java b/services/core/java/com/android/server/rollback/RollbackStore.java index 2cfa465328344..8a26368c3e133 100644 --- a/services/core/java/com/android/server/rollback/RollbackStore.java +++ b/services/core/java/com/android/server/rollback/RollbackStore.java @@ -194,7 +194,7 @@ class RollbackStore { * Creates a new RollbackData instance for a non-staged rollback with * backupDir assigned. */ - RollbackData createNonStagedRollback(int rollbackId) throws IOException { + RollbackData createNonStagedRollback(int rollbackId) { File backupDir = new File(mRollbackDataDir, Integer.toString(rollbackId)); return new RollbackData(rollbackId, backupDir, -1); } @@ -203,8 +203,7 @@ class RollbackStore { * Creates a new RollbackData instance for a staged rollback with * backupDir assigned. */ - RollbackData createStagedRollback(int rollbackId, int stagedSessionId) - throws IOException { + RollbackData createStagedRollback(int rollbackId, int stagedSessionId) { File backupDir = new File(mRollbackDataDir, Integer.toString(rollbackId)); return new RollbackData(rollbackId, backupDir, stagedSessionId); } diff --git a/tests/RollbackTest/RollbackTest/src/com/android/tests/rollback/RollbackTest.java b/tests/RollbackTest/RollbackTest/src/com/android/tests/rollback/RollbackTest.java index f9304f242ef36..7d36ac9458f0c 100644 --- a/tests/RollbackTest/RollbackTest/src/com/android/tests/rollback/RollbackTest.java +++ b/tests/RollbackTest/RollbackTest/src/com/android/tests/rollback/RollbackTest.java @@ -775,6 +775,42 @@ public class RollbackTest { } } + /** + * Test failure to enable rollback for multi-package installs. + * If any one of the packages fail to enable rollback, we shouldn't enable + * rollback for any package. + */ + @Test + public void testMultiPackageEnableFail() throws Exception { + try { + RollbackTestUtils.adoptShellPermissionIdentity( + Manifest.permission.INSTALL_PACKAGES, + Manifest.permission.DELETE_PACKAGES, + Manifest.permission.TEST_MANAGE_ROLLBACKS); + RollbackManager rm = RollbackTestUtils.getRollbackManager(); + + RollbackTestUtils.uninstall(TEST_APP_A); + RollbackTestUtils.uninstall(TEST_APP_B); + RollbackTestUtils.install("RollbackTestAppAv1.apk", false); + + // We should fail to enable rollback here because TestApp B is not + // already installed. + RollbackTestUtils.installMultiPackage(true, + "RollbackTestAppAv2.apk", + "RollbackTestAppBv2.apk"); + + assertEquals(2, RollbackTestUtils.getInstalledVersion(TEST_APP_A)); + assertEquals(2, RollbackTestUtils.getInstalledVersion(TEST_APP_B)); + + assertNull(getUniqueRollbackInfoForPackage( + rm.getAvailableRollbacks(), TEST_APP_A)); + assertNull(getUniqueRollbackInfoForPackage( + rm.getAvailableRollbacks(), TEST_APP_B)); + } finally { + RollbackTestUtils.dropShellPermissionIdentity(); + } + } + @Test @Ignore("b/120200473") /**