From bf2dd8b6f392b38d1d2402226c130b93ffb60f0c Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 7 Jul 2020 14:38:08 +0800 Subject: [PATCH 1/7] Protect #handleInstall with mLock (3/n) Some code inside #handleInstall needs to be locked. Bug: 159663586 Test: atest StagedInstallTest AtomicInstallTest Change-Id: I597d46a707cb482f95e8f3d2000820fa6ca5d5c1 --- .../java/com/android/server/pm/PackageInstallerSession.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 9eef336929d8d..cc4b22112982f 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -1687,7 +1687,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } private void handleInstall() { - if (isInstallerDeviceOwnerOrAffiliatedProfileOwnerLocked()) { + final boolean needsLogging; + synchronized (mLock) { + needsLogging = isInstallerDeviceOwnerOrAffiliatedProfileOwnerLocked(); + } + if (needsLogging) { DevicePolicyEventLogger .createEvent(DevicePolicyEnums.INSTALL_PACKAGE) .setAdmin(mInstallSource.installerPackageName) From 39fae7be5279f0798ad4711c1ab1b0e5bce3259c Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 7 Jul 2020 14:43:59 +0800 Subject: [PATCH 2/7] mSealed is not protected inside #setPermissionsResult (4/n) Call isSealed() instead. Bug: 159663586 Test: atest StagedInstallTest AtomicInstallTest Change-Id: Ifcbeeee4a8ea4e904b18dd6c8e0c1260d2c84765 --- .../java/com/android/server/pm/PackageInstallerSession.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index cc4b22112982f..c18ae3d999034 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -2544,7 +2544,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } void setPermissionsResult(boolean accepted) { - if (!mSealed) { + if (!isSealed()) { throw new SecurityException("Must be sealed to accept permissions"); } From 4c053b8278139b76c4d5c4cc76002c28d129ee63 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 7 Jul 2020 15:11:37 +0800 Subject: [PATCH 3/7] Protect accesses to members of childSession (5/n) Add a method to ensure access to mCommitted and mDestroyed are properly protected. Bug: 159663586 Test: atest StagedInstallTest AtomicInstallTest Change-Id: I704d972f7ec68eaa310da0a80f6f8acd4e1cc928 --- .../android/server/pm/PackageInstallerSession.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index c18ae3d999034..8531da463372d 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -2951,13 +2951,17 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return EMPTY_CHILD_SESSION_ARRAY; } + private boolean canBeAddedAsChild(int parentCandidate) { + synchronized (mLock) { + return (!hasParentSessionId() || mParentSessionId == parentCandidate) + && !mCommitted && !mDestroyed; + } + } + @Override public void addChildSessionId(int childSessionId) { final PackageInstallerSession childSession = mSessionProvider.getSession(childSessionId); - if (childSession == null - || (childSession.hasParentSessionId() && childSession.mParentSessionId != sessionId) - || childSession.mCommitted - || childSession.mDestroyed) { + if (childSession == null || !childSession.canBeAddedAsChild(sessionId)) { throw new IllegalStateException("Unable to add child session " + childSessionId + " as it does not exist or is in an invalid state."); } From 7f344ac524ac40fd7f94d268f9ad627c7a0a3119 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 7 Jul 2020 15:33:12 +0800 Subject: [PATCH 4/7] Protect accesses to mDestroyed and mDataLoaderFinished (6/n) Bug: 159663586 Test: atest StagedInstallTest AtomicInstallTest Change-Id: If9479152e0340ce7597b7846656b45ef3cd9974a --- .../server/pm/PackageInstallerSession.java | 33 +++++++++++++++---- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 8531da463372d..0044e3cb3929b 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -400,6 +400,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { @GuardedBy("mLock") private boolean mVerityFound; + @GuardedBy("mLock") private boolean mDataLoaderFinished = false; @GuardedBy("mLock") @@ -2790,7 +2791,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { return; } - if (mDestroyed || mDataLoaderFinished) { + final boolean isDestroyedOrDataLoaderFinished; + synchronized (mLock) { + isDestroyedOrDataLoaderFinished = mDestroyed || mDataLoaderFinished; + } + if (isDestroyedOrDataLoaderFinished) { switch (status) { case IDataLoaderStatusListener.DATA_LOADER_UNRECOVERABLE: onStorageUnhealthy(); @@ -2802,7 +2807,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { try { IDataLoader dataLoader = dataLoaderManager.getDataLoader(dataLoaderId); if (dataLoader == null) { - mDataLoaderFinished = true; + synchronized (mLock) { + mDataLoaderFinished = true; + } dispatchSessionVerificationFailure(INSTALL_FAILED_MEDIA_UNAVAILABLE, "Failure to obtain data loader"); return; @@ -2835,7 +2842,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { break; } case IDataLoaderStatusListener.DATA_LOADER_IMAGE_READY: { - mDataLoaderFinished = true; + synchronized (mLock) { + mDataLoaderFinished = true; + } if (hasParentSessionId()) { mSessionProvider.getSession( mParentSessionId).dispatchStreamValidateAndCommit(); @@ -2848,7 +2857,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { break; } case IDataLoaderStatusListener.DATA_LOADER_IMAGE_NOT_READY: { - mDataLoaderFinished = true; + synchronized (mLock) { + mDataLoaderFinished = true; + } dispatchSessionVerificationFailure(INSTALL_FAILED_MEDIA_UNAVAILABLE, "Failed to prepare image."); if (manualStartAndDestroy) { @@ -2862,7 +2873,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { break; } case IDataLoaderStatusListener.DATA_LOADER_UNRECOVERABLE: - mDataLoaderFinished = true; + synchronized (mLock) { + mDataLoaderFinished = true; + } dispatchSessionVerificationFailure(INSTALL_FAILED_MEDIA_UNAVAILABLE, "DataLoader reported unrecoverable failure."); break; @@ -2886,7 +2899,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { final IStorageHealthListener healthListener = new IStorageHealthListener.Stub() { @Override public void onHealthStatus(int storageId, int status) { - if (mDestroyed || mDataLoaderFinished) { + final boolean isDestroyedOrDataLoaderFinished; + synchronized (mLock) { + isDestroyedOrDataLoaderFinished = mDestroyed || mDataLoaderFinished; + } + if (isDestroyedOrDataLoaderFinished) { // App's installed. switch (status) { case IStorageHealthListener.HEALTH_STATUS_UNHEALTHY: @@ -2908,7 +2925,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // fallthrough case IStorageHealthListener.HEALTH_STATUS_UNHEALTHY: // Even ADB installation can't wait for missing pages for too long. - mDataLoaderFinished = true; + synchronized (mLock) { + mDataLoaderFinished = true; + } dispatchSessionVerificationFailure(INSTALL_FAILED_MEDIA_UNAVAILABLE, "Image is missing pages required for installation."); break; From 807f59ebb7555c64175e2822e04b8328338e98fe Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 7 Jul 2020 15:51:14 +0800 Subject: [PATCH 5/7] Protect accesses to mPackageName (7/n) Bug: 159663586 Test: atest StagedInstallTest AtomicInstallTest Change-Id: I8a2f1b0cfe7b2f4b9ff0356ab598801603d22830 --- .../android/server/pm/PackageInstallerSession.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 0044e3cb3929b..c6a7845677c3c 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -1586,12 +1586,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } private void onStorageUnhealthy() { - if (TextUtils.isEmpty(mPackageName)) { + final String packageName = getPackageName(); + if (TextUtils.isEmpty(packageName)) { // The package has not been installed. return; } final PackageManagerService packageManagerService = mPm; - final String packageName = mPackageName; mHandler.post(() -> { if (packageManagerService.deletePackageX(packageName, PackageManager.VERSION_CODE_HIGHEST, UserHandle.USER_SYSTEM, @@ -1906,19 +1906,20 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // Skip logging the side-loaded app installations, as those are private and aren't reported // anywhere; app stores already have a record of the installation and that's why reporting // it here is fine + final String packageName = getPackageName(); final String packageNameToLog = - (params.installFlags & PackageManager.INSTALL_FROM_ADB) == 0 ? mPackageName : ""; + (params.installFlags & PackageManager.INSTALL_FROM_ADB) == 0 ? packageName : ""; final long currentTimestamp = System.currentTimeMillis(); FrameworkStatsLog.write(FrameworkStatsLog.PACKAGE_INSTALLER_V2_REPORTED, isIncrementalInstallation(), packageNameToLog, currentTimestamp - createdMillis, returnCode, - getApksSize()); + getApksSize(packageName)); } - private long getApksSize() { - final PackageSetting ps = mPm.getPackageSetting(mPackageName); + private long getApksSize(String packageName) { + final PackageSetting ps = mPm.getPackageSetting(packageName); if (ps == null) { return 0; } From 631f6a1b9449fcaccf3b8db5a31b679a512ac803 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 7 Jul 2020 16:11:36 +0800 Subject: [PATCH 6/7] Protect accesses to mParentSessionId (8/n) Bug: 159663586 Test: atest StagedInstallTest AtomicInstallTest Change-Id: I89acee96601d9baf7ec7efd83d63b2d6c67bb8c3 --- .../android/server/pm/PackageInstallerSession.java | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index c6a7845677c3c..09d2da646f234 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -1115,7 +1115,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { if (hasParentSessionId()) { throw new IllegalStateException( "Session " + sessionId + " is a child of multi-package session " - + mParentSessionId + " and may not be committed directly."); + + getParentSessionId() + " and may not be committed directly."); } if (!markAsSealed(statusReceiver, forTransfer)) { @@ -2622,7 +2622,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { if (hasParentSessionId()) { throw new IllegalStateException( "Session " + sessionId + " is a child of multi-package session " - + mParentSessionId + " and may not be abandoned directly."); + + getParentSessionId() + " and may not be abandoned directly."); } List childSessions = getChildSessionsNotLocked(); @@ -2848,7 +2848,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } if (hasParentSessionId()) { mSessionProvider.getSession( - mParentSessionId).dispatchStreamValidateAndCommit(); + getParentSessionId()).dispatchStreamValidateAndCommit(); } else { dispatchStreamValidateAndCommit(); } @@ -3030,12 +3030,16 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } boolean hasParentSessionId() { - return mParentSessionId != SessionInfo.INVALID_ID; + synchronized (mLock) { + return mParentSessionId != SessionInfo.INVALID_ID; + } } @Override public int getParentSessionId() { - return mParentSessionId; + synchronized (mLock) { + return mParentSessionId; + } } private void dispatchSessionFinished(int returnCode, String msg, Bundle extras) { From 2ddce8af7e143b142fc38673e10cfc7afc80a8d1 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Tue, 7 Jul 2020 16:17:48 +0800 Subject: [PATCH 7/7] Protect accesses to mStagedSessionXXX (9/n) Bug: 159663586 Test: atest StagedInstallTest AtomicInstallTest Change-Id: I6c91d7f5b39ed3aa1627f82ba1d4317a24c2debd --- .../server/pm/PackageInstallerSession.java | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 09d2da646f234..79d258e28d293 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -3129,27 +3129,37 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { /** {@hide} */ boolean isStagedSessionReady() { - return mStagedSessionReady; + synchronized (mLock) { + return mStagedSessionReady; + } } /** {@hide} */ boolean isStagedSessionApplied() { - return mStagedSessionApplied; + synchronized (mLock) { + return mStagedSessionApplied; + } } /** {@hide} */ boolean isStagedSessionFailed() { - return mStagedSessionFailed; + synchronized (mLock) { + return mStagedSessionFailed; + } } /** {@hide} */ @StagedSessionErrorCode int getStagedSessionErrorCode() { - return mStagedSessionErrorCode; + synchronized (mLock) { + return mStagedSessionErrorCode; + } } /** {@hide} */ String getStagedSessionErrorMessage() { - return mStagedSessionErrorMessage; + synchronized (mLock) { + return mStagedSessionErrorMessage; + } } private void destroyInternal() {