From 73c1071428755692e55b89835e0c51e9add8c176 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Wed, 17 Nov 2021 15:12:10 +0800 Subject: [PATCH 1/2] A destroyed session shouldn't fail other sessions Bug: 206733704 Test: atest PackageSessionVerifierTest Change-Id: I0ea1a663879bb2f6736d3a758caab8c7c9894ced --- .../server/pm/PackageSessionVerifier.java | 6 ++++++ .../server/pm/PackageSessionVerifierTest.java | 20 ++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageSessionVerifier.java b/services/core/java/com/android/server/pm/PackageSessionVerifier.java index 866712c43a96b..4f21d0e8d1c02 100644 --- a/services/core/java/com/android/server/pm/PackageSessionVerifier.java +++ b/services/core/java/com/android/server/pm/PackageSessionVerifier.java @@ -527,6 +527,9 @@ final class PackageSessionVerifier { @VisibleForTesting void checkRollbacks(StagingManager.StagedSession session) throws PackageManagerException { + if (session.isDestroyed() || session.isInTerminalState()) { + return; + } for (StagingManager.StagedSession stagedSession : mStagedSessions) { if (stagedSession.isDestroyed() || stagedSession.isInTerminalState()) { continue; @@ -565,6 +568,9 @@ final class PackageSessionVerifier { @VisibleForTesting void checkOverlaps(StagingManager.StagedSession parent, StagingManager.StagedSession child) throws PackageManagerException { + if (parent.isDestroyed() || parent.isInTerminalState()) { + return; + } final String packageName = child.getPackageName(); if (packageName == null) { throw new PackageManagerException( diff --git a/services/tests/mockingservicestests/src/com/android/server/pm/PackageSessionVerifierTest.java b/services/tests/mockingservicestests/src/com/android/server/pm/PackageSessionVerifierTest.java index 1e399ff33dece..cf5967e2764c3 100644 --- a/services/tests/mockingservicestests/src/com/android/server/pm/PackageSessionVerifierTest.java +++ b/services/tests/mockingservicestests/src/com/android/server/pm/PackageSessionVerifierTest.java @@ -21,6 +21,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -77,18 +78,26 @@ public class PackageSessionVerifierTest { public void checkRollbacks() throws Exception { StagingManager.StagedSession session1 = createStagedSession(111, "com.foo", 1); StagingManager.StagedSession session2 = createStagedSession(222, "com.bar", 2); + StagingManager.StagedSession session3 = createStagedSession(333, "com.baz", 3); session2.sessionParams().setInstallReason(PackageManager.INSTALL_REASON_ROLLBACK); + session3.sessionParams().setInstallReason(PackageManager.INSTALL_REASON_ROLLBACK); + when(session2.isDestroyed()).thenReturn(true); mSessionVerifier.storeSession(session1); mSessionVerifier.storeSession(session2); + mSessionVerifier.storeSession(session3); + + // Non-rollback session shouldn't be failed by a destroyed session + mSessionVerifier.checkRollbacks(session2); + verify(session1, never()).setSessionFailed(anyInt(), anyString()); // Non-rollback session should fail - mSessionVerifier.checkRollbacks(session2); + mSessionVerifier.checkRollbacks(session3); verify(session1, times(1)).setSessionFailed(anyInt(), anyString()); // Yet another non-rollback session should fail - StagingManager.StagedSession session3 = createStagedSession(333, "com.baz", 3); + StagingManager.StagedSession session4 = createStagedSession(444, "com.fur", 4); assertThrows(PackageManagerException.class, - () -> mSessionVerifier.checkRollbacks(session3)); + () -> mSessionVerifier.checkRollbacks(session4)); } @Test @@ -105,6 +114,11 @@ public class PackageSessionVerifierTest { StagingManager.StagedSession session3 = createStagedSession(333, "com.foo", 3); assertThrows(PackageManagerException.class, () -> mSessionVerifier.checkOverlaps(session3, session3)); + // session4 is earlier than session1, but it shouldn't fail session1 + StagingManager.StagedSession session4 = createStagedSession(444, "com.foo", 0); + when(session4.isDestroyed()).thenReturn(true); + mSessionVerifier.checkOverlaps(session4, session4); + verify(session1, never()).setSessionFailed(anyInt(), anyString()); } private PackageInstallerSession createSession(boolean isStaged, boolean isApex, From b69eacf5ccc897132a18fec6b7c6e0c77372ad5c Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 16 Nov 2021 16:49:07 +0800 Subject: [PATCH 2/2] Add session cleanup tests for multi-package sessions (2/n) * Tests session cleanup in the case of multi-package sessions * Adds destroy() which cleans up storage for all children Bug: 173194203 Test: atest StagedInstallInternalTest Change-Id: I726dd15155f65a7f92ed4d0aaa66c83ceb586d7b --- .../server/pm/PackageInstallerSession.java | 38 +++++++++++-------- .../StagedInstallInternalTest.java | 16 ++++++++ .../host/StagedInstallInternalTest.java | 12 ++++++ 3 files changed, 51 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 c47ca4b45ab37..f997d67864f93 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -558,10 +558,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { Slog.d(TAG, "Marking session " + sessionId + " as failed: " + errorMessage); childSessions = getChildSessionsLocked(); } - destroyInternal(); - for (PackageInstallerSession child : childSessions) { - child.destroyInternal(); - } + destroy(); mCallback.onStagedSessionChanged(PackageInstallerSession.this); } @@ -579,10 +576,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { Slog.d(TAG, "Marking session " + sessionId + " as applied"); childSessions = getChildSessionsLocked(); } - destroyInternal(); - for (PackageInstallerSession child : childSessions) { - child.destroyInternal(); - } + destroy(); mCallback.onStagedSessionChanged(PackageInstallerSession.this); } @@ -2111,18 +2105,14 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { private void onSessionVerificationFailure(int error, String msg) { final String msgWithErrorCode = PackageManager.installStatusToString(error, msg); Slog.e(TAG, "Failed to verify session " + sessionId + " [" + msgWithErrorCode + "]"); - // Session is sealed and committed but could not be verified, we need to destroy it. - destroyInternal(); - if (isMultiPackage()) { - for (PackageInstallerSession childSession : getChildSessions()) { - childSession.destroyInternal(); - } - } if (isStaged()) { + // This will clean up the session when it reaches the terminal state mStagedSession.setSessionFailed( SessionInfo.STAGED_SESSION_VERIFICATION_FAILED, msgWithErrorCode); mStagedSession.notifyEndPreRebootVerification(); } else { + // Session is sealed and committed but could not be verified, we need to destroy it. + destroy(); // Dispatch message to remove session from PackageInstallerService. dispatchSessionFinished(error, msg, null); } @@ -4265,6 +4255,24 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return params.isStaged ? mStagedSession.getSessionErrorMessage() : ""; } + /** + * Free up storage used by this session and its children. + * Must not be called on a child session. + */ + private void destroy() { + // TODO(b/173194203): destroyInternal() should be used by destroy() only. + // For the sake of consistency, a session should be destroyed as a whole. The caller + // should always call destroy() for cleanup without knowing it has child sessions or not. + assertNotChild("destroy"); + destroyInternal(); + for (PackageInstallerSession child : getChildSessions()) { + child.destroyInternal(); + } + } + + /** + * Free up storage used by this session. + */ private void destroyInternal() { final IncrementalFileStorages incrementalFileStorages; synchronized (mLock) { diff --git a/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java b/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java index e4e535d3240d2..da5468e48f194 100644 --- a/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java +++ b/tests/StagedInstallTest/app/src/com/android/tests/stagedinstallinternal/StagedInstallInternalTest.java @@ -157,8 +157,18 @@ public class StagedInstallInternalTest { @Test public void testStagedSessionShouldCleanUpOnVerificationFailure() throws Exception { + // APEX verification InstallUtils.commitExpectingFailure(AssertionError.class, "apexd verification failed", Install.single(APEX_WRONG_SHA_V2).setStaged()); + InstallUtils.commitExpectingFailure(AssertionError.class, "apexd verification failed", + Install.multi(APEX_WRONG_SHA_V2, TestApp.A1).setStaged()); + // APK verification + Install.single(TestApp.A2).commit(); + assertThat(InstallUtils.getInstalledVersion(TestApp.A)).isEqualTo(2); + InstallUtils.commitExpectingFailure(AssertionError.class, "Downgrade detected", + Install.single(TestApp.A1).setStaged()); + InstallUtils.commitExpectingFailure(AssertionError.class, "Downgrade detected", + Install.multi(TestApp.A1, TestApp.B1).setStaged()); } @Test @@ -175,6 +185,12 @@ public class StagedInstallInternalTest { assertThat(info.isStagedSessionApplied()).isTrue(); } + @Test + public void testStagedSessionShouldCleanUpOnOnSuccessMultiPackage_Commit() throws Exception { + int sessionId = Install.multi(TestApp.A1, TestApp.Apex2).setStaged().commit(); + storeSessionId(sessionId); + } + @Test public void testStagedInstallationShouldCleanUpOnValidationFailure() throws Exception { InstallUtils.commitExpectingFailure(AssertionError.class, "INSTALL_FAILED_INVALID_APK", diff --git a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java index 78cf9aca66c09..926bf1b88fee3 100644 --- a/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java +++ b/tests/StagedInstallTest/src/com/android/tests/stagedinstallinternal/host/StagedInstallInternalTest.java @@ -300,6 +300,18 @@ public class StagedInstallInternalTest extends BaseHostJUnit4Test { assertThat(after).isEqualTo(before); } + @Test + @LargeTest + public void testStagedSessionShouldCleanUpOnOnSuccessMultiPackage() throws Exception { + List before = getStagingDirectories(); + runPhase("testStagedSessionShouldCleanUpOnOnSuccessMultiPackage_Commit"); + assertThat(getStagingDirectories()).isNotEqualTo(before); + getDevice().reboot(); + runPhase("testStagedSessionShouldCleanUpOnOnSuccess_Verify"); + List after = getStagingDirectories(); + assertThat(after).isEqualTo(before); + } + @Test public void testStagedInstallationShouldCleanUpOnValidationFailure() throws Exception { List before = getStagingDirectories();