From 02c4a81ef859cf636e42bb76ff48bd448e49ea63 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Fri, 20 May 2022 21:29:52 +0000 Subject: [PATCH] Revert "Revert "Persist permission state for updated apps synchronously."" This reverts commit cebec6c3e83dae71305f82580a1ed963babc94ad. The root cause for the previous regression is that when a new user is added, the replace parameter will be true for every package (despite they aren't being actually replaced), thus a synchronous write will be done for every one of them and the PM lock will be blocked and main thread will be held for a very long time, causing ANR in apps calling into system server at the same time. The proper fix would be renaming the replace parameter and the relevant flags like UPDATE_PERMISSIONS_REPLACE(_ALL) into something like regrantInstallPermissions that better describes its use, and then introduce another parameter that actually represents whether the package is being replaced. But since we are late in T now and naming is hard, this double revert simply re-purposes the existing changing package name to indicate whether there is actually a changing package, and only write synchronously in that case. Fixes: 221899913 Test: android.platform.test.scenario.sysui.multiuser.AddGuest Change-Id: Ifa40b435c2ad8d04050fd5c5972408e61aeea64d --- .../PermissionManagerServiceImpl.java | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java index bd3fc50473de4..3924a5421531e 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java @@ -2493,13 +2493,13 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt * @param pkg the package the permissions belong to * @param replace if the package is getting replaced (this might change the requested * permissions of this package) - * @param packageOfInterest If this is the name of {@code pkg} add extra logging + * @param changingPackageName the name of the package that is changing * @param callback Result call back * @param filterUserId If not {@link UserHandle.USER_ALL}, only restore the permission state for * this particular user */ private void restorePermissionState(@NonNull AndroidPackage pkg, boolean replace, - @Nullable String packageOfInterest, @Nullable PermissionCallback callback, + @Nullable String changingPackageName, @Nullable PermissionCallback callback, @UserIdInt int filterUserId) { // IMPORTANT: There are two types of permissions: install and runtime. // Install time permissions are granted when the app is installed to @@ -2521,6 +2521,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt final int[] userIds = filterUserId == UserHandle.USER_ALL ? getAllUserIds() : new int[] { filterUserId }; + boolean installPermissionsChanged = false; boolean runtimePermissionsRevoked = false; int[] updatedUserIds = EMPTY_INT_ARRAY; @@ -2639,7 +2640,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt UidPermissionState origState = uidState; - boolean changedInstallPermission = false; + boolean installPermissionsChangedForUser = false; if (replace) { userState.setInstallPermissionsFixed(ps.getPackageName(), false); @@ -2680,7 +2681,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt // permission is present, because otherwise the permission should have been // removed. if (bp == null /*|| getSourcePackageSetting(bp) == null*/) { - if (packageOfInterest == null || packageOfInterest.equals( + if (changingPackageName == null || changingPackageName.equals( pkg.getPackageName())) { if (DEBUG_PERMISSIONS) { Slog.i(TAG, "Unknown permission " + permName @@ -2805,7 +2806,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt && origState.isPermissionGranted(permName))))) { // Grant an install permission. if (uidState.grantPermission(bp)) { - changedInstallPermission = true; + installPermissionsChangedForUser = true; } } else if (bp.isRuntime()) { boolean hardRestricted = bp.isHardRestricted(); @@ -2945,12 +2946,12 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt } } if (uidState.removePermissionState(bp.getName())) { - changedInstallPermission = true; + installPermissionsChangedForUser = true; } } } - if ((changedInstallPermission || replace) + if ((installPermissionsChangedForUser || replace) && !userState.areInstallPermissionsFixed(ps.getPackageName()) && !ps.isSystem() || ps.getTransientState().isUpdatedSystemApp()) { // This is the first that we have heard about this package, so the @@ -2959,6 +2960,12 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt userState.setInstallPermissionsFixed(ps.getPackageName(), true); } + if (installPermissionsChangedForUser) { + installPermissionsChanged = true; + if (changingPackageName != null && replace) { + updatedUserIds = ArrayUtils.appendInt(updatedUserIds, userId); + } + } updatedUserIds = revokePermissionsNoLongerImplicitLocked(uidState, pkg.getPackageName(), uidImplicitPermissions, uidTargetSdkVersion, userId, updatedUserIds); @@ -2975,8 +2982,13 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt // Persist the runtime permissions state for users with changes. If permissions // were revoked because no app in the shared user declares them we have to // write synchronously to avoid losing runtime permissions state. + // Also write synchronously if we changed any install permission for an updated app, because + // the install permission state is likely already fixed before update, and if we lose the + // changes here the app won't be reconsidered for newly-added install permissions. if (callback != null) { - callback.onPermissionUpdated(updatedUserIds, runtimePermissionsRevoked); + callback.onPermissionUpdated(updatedUserIds, + (changingPackageName != null && replace && installPermissionsChanged) + || runtimePermissionsRevoked); } for (int userId : updatedUserIds) {