From ebd86f6267228ddc44e28a5d57892f162b118a83 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Thu, 16 Jul 2020 17:06:12 +0800 Subject: [PATCH 1/3] Some code cleanup (1/n) 1. make #isLinkPossible a static method which doesn't access members. 2. access members directly when already in the lock. Bug: 161199857 Test: m Change-Id: Ice3041861b5b07368f650f09ab5a05db82388fcf --- .../server/pm/PackageInstallerSession.java | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 00cb22e9d4b56..25bce32a63d8a 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -2438,7 +2438,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * Determine if creating hard links between source and destination is * possible. That is, do they all live on the same underlying device. */ - private boolean isLinkPossible(List fromFiles, File toDir) { + private static boolean isLinkPossible(List fromFiles, File toDir) { try { final StructStat toStat = Os.stat(toDir.getAbsolutePath()); for (File fromFile : fromFiles) { @@ -3004,16 +3004,17 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { detailMessage).sendToTarget(); } + @GuardedBy("mLock") + private int[] getChildSessionIdsLocked() { + final int[] childSessionIds = mChildSessionIds.copyKeys(); + return childSessionIds != null ? childSessionIds : EMPTY_CHILD_SESSION_ARRAY; + } + @Override public int[] getChildSessionIds() { - final int[] childSessionIds; synchronized (mLock) { - childSessionIds = mChildSessionIds.copyKeys(); + return getChildSessionIdsLocked(); } - if (childSessionIds != null) { - return childSessionIds; - } - return EMPTY_CHILD_SESSION_ARRAY; } private boolean canBeAddedAsChild(int parentCandidate) { @@ -3470,10 +3471,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { if (stageCid != null) { writeStringAttribute(out, ATTR_SESSION_STAGE_CID, stageCid); } - writeBooleanAttribute(out, ATTR_PREPARED, isPrepared()); - writeBooleanAttribute(out, ATTR_COMMITTED, isCommitted()); - writeBooleanAttribute(out, ATTR_DESTROYED, isDestroyed()); - writeBooleanAttribute(out, ATTR_SEALED, isSealed()); + writeBooleanAttribute(out, ATTR_PREPARED, mPrepared); + writeBooleanAttribute(out, ATTR_COMMITTED, mCommitted); + writeBooleanAttribute(out, ATTR_DESTROYED, mDestroyed); + writeBooleanAttribute(out, ATTR_SEALED, mSealed); writeBooleanAttribute(out, ATTR_MULTI_PACKAGE, params.isMultiPackage); writeBooleanAttribute(out, ATTR_STAGED_SESSION, params.isStaged); @@ -3535,7 +3536,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { params.appIconLastModified = appIconFile.lastModified(); } - final int[] childSessionIds = getChildSessionIds(); + final int[] childSessionIds = getChildSessionIdsLocked(); for (int childSessionId : childSessionIds) { out.startTag(null, TAG_CHILD_SESSION); writeIntAttribute(out, ATTR_SESSION_ID, childSessionId); @@ -3543,7 +3544,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } final InstallationFile[] files = getInstallationFilesLocked(); - for (InstallationFile file : getInstallationFilesLocked()) { + for (InstallationFile file : files) { out.startTag(null, TAG_SESSION_FILE); writeIntAttribute(out, ATTR_LOCATION, file.getLocation()); writeStringAttribute(out, ATTR_NAME, file.getName()); From a39f84ee3504b2404e526a67b69a340738a87f22 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Thu, 16 Jul 2020 17:12:54 +0800 Subject: [PATCH 2/3] Make #sendPendingStreaming a static method (2/n) 1. to be consistent with #sendOnPackageInstalled. 2. document these static methods don't change the internal states and are safe to call outside the lock. Bug: 161199857 Test: m Change-Id: Ie8fe5e988b06c66648150d0ec1e285a38409ac18 --- .../server/pm/PackageInstallerSession.java | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 25bce32a63d8a..0f58b10f52b8c 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -2915,7 +2915,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } case IDataLoaderStatusListener.DATA_LOADER_UNAVAILABLE: { // Don't fail or commit the session. Allow caller to commit again. - sendPendingStreaming("DataLoader unavailable"); + final IntentSender statusReceiver; + synchronized (mLock) { + statusReceiver = mRemoteStatusReceiver; + } + sendPendingStreaming(mContext, statusReceiver, sessionId, + "DataLoader unavailable"); break; } case IDataLoaderStatusListener.DATA_LOADER_UNRECOVERABLE: @@ -2929,7 +2934,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } catch (RemoteException e) { // In case of streaming failure we don't want to fail or commit the session. // Just return from this method and allow caller to commit again. - sendPendingStreaming(e.getMessage()); + final IntentSender statusReceiver; + synchronized (mLock) { + statusReceiver = mRemoteStatusReceiver; + } + sendPendingStreaming(mContext, statusReceiver, sessionId, e.getMessage()); } } }; @@ -3324,6 +3333,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { pw.decreaseIndent(); } + /** + * This method doesn't change internal states and is safe to call outside the lock. + */ private static void sendOnUserActionRequired(Context context, IntentSender target, int sessionId, Intent intent) { final Intent fillIn = new Intent(); @@ -3336,6 +3348,9 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } + /** + * This method doesn't change internal states and is safe to call outside the lock. + */ private static void sendOnPackageInstalled(Context context, IntentSender target, int sessionId, boolean showNotification, int userId, String basePackageName, int returnCode, String msg, Bundle extras) { @@ -3376,13 +3391,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } - private void sendPendingStreaming(@Nullable String cause) { - final IntentSender statusReceiver; - synchronized (mLock) { - statusReceiver = mRemoteStatusReceiver; - } - - if (statusReceiver == null) { + /** + * This method doesn't change internal states and is safe to call outside the lock. + */ + private static void sendPendingStreaming(Context context, IntentSender target, int sessionId, + @Nullable String cause) { + if (target == null) { Slog.e(TAG, "Missing receiver for pending streaming status."); return; } @@ -3397,7 +3411,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { intent.putExtra(PackageInstaller.EXTRA_STATUS_MESSAGE, "Staging Image Not Ready"); } try { - statusReceiver.sendIntent(mContext, 0, intent, null, null); + target.sendIntent(context, 0, intent, null, null); } catch (IntentSender.SendIntentException ignored) { } } From 82278d78b782b8aefa1121b61de2f7e9bd500511 Mon Sep 17 00:00:00 2001 From: JW Wang Date: Fri, 17 Jul 2020 10:12:51 +0800 Subject: [PATCH 3/3] Rewrite some methods (3/n) 1. Now #isInstallerDeviceOwnerOrAffiliatedProfileOwner should be called outside the lock because it accesses external entities. 2. Now #needToAskForPermissions should be called outside the lock for the same reason above. 3. #sendOnUserActionRequired, #sendOnPackageInstalled and #sendPendingStreaming are now called outside the lock. 4. Rewrite #installNonStagedLocked so mPm.installStage() is called outside the lock. 5. Split #makeSessionActive into 2 parts so permission checks can be done outside the lock. Note the original code is flawed in that session.makeSessionActiveLocked() [1] is called without session.mLock held. This is also fixed by this CL. [1] https://source.corp.google.com/android/frameworks/base/services/core/java/com/android/server/pm/PackageInstallerSession.java;rcl=d07f2fc343d37100303696fcfeb603232b5087ce;l=1755 Bug: 161199857 Test: atest StagedInstallTest AtomicInstallTest Change-Id: I1f4a28259f185d8792bac5a9c8a974df53e7d3fb --- .../server/pm/PackageInstallerSession.java | 138 +++++++++++------- 1 file changed, 84 insertions(+), 54 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageInstallerSession.java b/services/core/java/com/android/server/pm/PackageInstallerSession.java index 0f58b10f52b8c..391a08db6716a 100644 --- a/services/core/java/com/android/server/pm/PackageInstallerSession.java +++ b/services/core/java/com/android/server/pm/PackageInstallerSession.java @@ -459,12 +459,8 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { final int returnCode = args.argi1; args.recycle(); - final boolean showNotification; - synchronized (mLock) { - showNotification = isInstallerDeviceOwnerOrAffiliatedProfileOwnerLocked(); - } sendOnPackageInstalled(mContext, statusReceiver, sessionId, - showNotification, userId, + isInstallerDeviceOwnerOrAffiliatedProfileOwner(), userId, packageName, returnCode, message, extras); break; @@ -494,8 +490,11 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { /** * @return {@code true} iff the installing is app an device owner or affiliated profile owner. */ - @GuardedBy("mLock") - private boolean isInstallerDeviceOwnerOrAffiliatedProfileOwnerLocked() { + private boolean isInstallerDeviceOwnerOrAffiliatedProfileOwner() { + assertNotLocked("isInstallerDeviceOwnerOrAffiliatedProfileOwner"); + // It is safe to access mInstallerUid and mInstallSource without lock + // because they are immutable after sealing. + assertSealed("isInstallerDeviceOwnerOrAffiliatedProfileOwner"); if (userId != UserHandle.getUserId(mInstallerUid)) { return false; } @@ -513,12 +512,17 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * * @return {@code true} iff we need to ask to confirm the permissions? */ - @GuardedBy("mLock") - private boolean needToAskForPermissionsLocked() { - if (mPermissionsManuallyAccepted) { - return false; + private boolean needToAskForPermissions() { + final String packageName; + synchronized (mLock) { + if (mPermissionsManuallyAccepted) { + return false; + } + packageName = mPackageName; } + // It is safe to access mInstallerUid and mInstallSource without lock + // because they are immutable after sealing. final boolean isInstallPermissionGranted = (mPm.checkUidPermission(android.Manifest.permission.INSTALL_PACKAGES, mInstallerUid) == PackageManager.PERMISSION_GRANTED); @@ -528,7 +532,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { final boolean isUpdatePermissionGranted = (mPm.checkUidPermission(android.Manifest.permission.INSTALL_PACKAGE_UPDATES, mInstallerUid) == PackageManager.PERMISSION_GRANTED); - final int targetPackageUid = mPm.getPackageUid(mPackageName, 0, userId); + final int targetPackageUid = mPm.getPackageUid(packageName, 0, userId); final boolean isPermissionGranted = isInstallPermissionGranted || (isUpdatePermissionGranted && targetPackageUid != -1) || (isSelfUpdatePermissionGranted && targetPackageUid == mInstallerUid); @@ -540,7 +544,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { // Device owners and affiliated profile owners are allowed to silently install packages, so // the permission check is waived if the installer is the device owner. return forcePermissionPrompt || !(isPermissionGranted || isInstallerRoot - || isInstallerSystem || isInstallerDeviceOwnerOrAffiliatedProfileOwnerLocked()); + || isInstallerSystem || isInstallerDeviceOwnerOrAffiliatedProfileOwner()); } public PackageInstallerSession(PackageInstallerService.InternalCallback callback, @@ -740,6 +744,18 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } + private void assertNotLocked(String cookie) { + if (Thread.holdsLock(mLock)) { + throw new IllegalStateException(cookie + " is holding mLock"); + } + } + + private void assertSealed(String cookie) { + if (!isSealed()) { + throw new IllegalStateException(cookie + " before sealing"); + } + } + @GuardedBy("mLock") private void assertPreparedAndNotSealedLocked(String cookie) { assertPreparedAndNotCommittedOrDestroyedLocked(cookie); @@ -1693,11 +1709,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } private void handleInstall() { - final boolean needsLogging; - synchronized (mLock) { - needsLogging = isInstallerDeviceOwnerOrAffiliatedProfileOwnerLocked(); - } - if (needsLogging) { + if (isInstallerDeviceOwnerOrAffiliatedProfileOwner()) { DevicePolicyEventLogger .createEvent(DevicePolicyEnums.INSTALL_PACKAGE) .setAdmin(mInstallSource.installerPackageName) @@ -1724,9 +1736,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { List childSessions = getChildSessionsNotLocked(); try { - synchronized (mLock) { - installNonStagedLocked(childSessions); - } + installNonStaged(childSessions); } catch (PackageManagerException e) { final String completeMsg = ExceptionUtils.getCompleteMessage(e); Slog.e(TAG, "Commit of session " + sessionId + " failed: " + completeMsg); @@ -1735,11 +1745,10 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } - @GuardedBy("mLock") - private void installNonStagedLocked(List childSessions) + private void installNonStaged(List childSessions) throws PackageManagerException { final PackageManagerService.ActiveInstallSession installingSession = - makeSessionActiveLocked(); + makeSessionActive(); if (installingSession == null) { return; } @@ -1752,7 +1761,7 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { final PackageInstallerSession session = childSessions.get(i); try { final PackageManagerService.ActiveInstallSession installingChildSession = - session.makeSessionActiveLocked(); + session.makeSessionActive(); if (installingChildSession != null) { installingChildSessions.add(installingChildSession); } @@ -1762,8 +1771,12 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { } } if (!success) { - sendOnPackageInstalled(mContext, mRemoteStatusReceiver, sessionId, - isInstallerDeviceOwnerOrAffiliatedProfileOwnerLocked(), userId, null, + final IntentSender statusReceiver; + synchronized (mLock) { + statusReceiver = mRemoteStatusReceiver; + } + sendOnPackageInstalled(mContext, statusReceiver, sessionId, + isInstallerDeviceOwnerOrAffiliatedProfileOwner(), userId, null, failure.error, failure.getLocalizedMessage(), null); return; } @@ -1778,41 +1791,58 @@ public class PackageInstallerSession extends IPackageInstallerSession.Stub { * {@link PackageManagerService.ActiveInstallSession} representing this new staged state or null * in case permissions need to be requested before install can proceed. */ + private PackageManagerService.ActiveInstallSession makeSessionActive() + throws PackageManagerException { + assertNotLocked("makeSessionActive"); + + synchronized (mLock) { + if (mRelinquished) { + throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, + "Session relinquished"); + } + if (mDestroyed) { + throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, + "Session destroyed"); + } + if (!mSealed) { + throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, + "Session not sealed"); + } + } + + if (!params.isMultiPackage && needToAskForPermissions()) { + // User needs to confirm installation; + // give installer an intent they can use to involve + // user. + final Intent intent = new Intent(PackageInstaller.ACTION_CONFIRM_INSTALL); + intent.setPackage(mPm.getPackageInstallerPackageName()); + intent.putExtra(PackageInstaller.EXTRA_SESSION_ID, sessionId); + + final IntentSender statusReceiver; + synchronized (mLock) { + statusReceiver = mRemoteStatusReceiver; + } + sendOnUserActionRequired(mContext, statusReceiver, sessionId, intent); + + // Commit was keeping session marked as active until now; release + // that extra refcount so session appears idle. + closeInternal(false); + return null; + } + + synchronized (mLock) { + return makeSessionActiveLocked(); + } + } + @GuardedBy("mLock") private PackageManagerService.ActiveInstallSession makeSessionActiveLocked() throws PackageManagerException { - if (mRelinquished) { - throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, - "Session relinquished"); - } - if (mDestroyed) { - throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, "Session destroyed"); - } - if (!mSealed) { - throw new PackageManagerException(INSTALL_FAILED_INTERNAL_ERROR, "Session not sealed"); - } - if (!params.isMultiPackage) { Objects.requireNonNull(mPackageName); Objects.requireNonNull(mSigningDetails); Objects.requireNonNull(mResolvedBaseFile); - if (needToAskForPermissionsLocked()) { - // User needs to confirm installation; - // give installer an intent they can use to involve - // user. - final Intent intent = new Intent(PackageInstaller.ACTION_CONFIRM_INSTALL); - intent.setPackage(mPm.getPackageInstallerPackageName()); - intent.putExtra(PackageInstaller.EXTRA_SESSION_ID, sessionId); - - sendOnUserActionRequired(mContext, mRemoteStatusReceiver, sessionId, intent); - - // Commit was keeping session marked as active until now; release - // that extra refcount so session appears idle. - closeInternal(false); - return null; - } - // Inherit any packages and native libraries from existing install that // haven't been overridden. if (params.mode == SessionParams.MODE_INHERIT_EXISTING) {