From 3641ff8031c592caa89489597186e8b16b01dfe1 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Wed, 28 Oct 2020 19:58:21 -0700 Subject: [PATCH 1/2] Remove locking inside PermissionRegistry. This change removes the locking inside PermissionRegistry, and makes sure that the PermissionManagerService lock is held when calling into PermissionRegistry. There are still a small number of cases where a Permission instance may be retrieved under the lock, but used outside of the locked block. This may be okay because it doesn't contain any collection classes that absolutely requires synchronized access, and the worst theoretical effect is slightly outdated data (if that's even possible in reality). This has been happening for years in past releases, and permissions almost never change after being registered. Bug: 158736025 Test: manual Change-Id: I8d1c8b64f6db169e8feb23e779db3f326469c5fe --- .../permission/PermissionManagerService.java | 358 ++++++++++-------- .../pm/permission/PermissionRegistry.java | 84 +--- 2 files changed, 208 insertions(+), 234 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java index 6d987aee89956..db4878680a2ed 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -252,7 +252,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { /** Internal storage for permissions and related settings */ @GuardedBy("mLock") - private final PermissionRegistry mRegistry; + private final PermissionRegistry mRegistry = new PermissionRegistry(); /** Injector that can be used to facilitate testing. */ private final Injector mInjector; @@ -387,7 +387,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { mLock = externalLock; mPackageManagerInt = LocalServices.getService(PackageManagerInternal.class); mUserManagerInt = LocalServices.getService(UserManagerInternal.class); - mRegistry = new PermissionRegistry(mLock); mAppOpsManager = context.getSystemService(AppOpsManager.class); mHandlerThread = new ServiceThread(TAG, @@ -409,10 +408,10 @@ public class PermissionManagerService extends IPermissionManager.Stub { synchronized (mLock) { for (int i=0; i pkgs = mRegistry.getAppOpPermissionPackagesLocked(permName); + final ArraySet pkgs = mRegistry.getAppOpPermissionPackages(permName); if (pkgs == null) { return null; } @@ -519,7 +511,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } synchronized (mLock) { final List out = new ArrayList<>(); - for (ParsedPermissionGroup pg : mRegistry.getPermissionGroupsLocked()) { + for (ParsedPermissionGroup pg : mRegistry.getPermissionGroups()) { out.add(PackageInfoUtils.generatePermissionGroupInfo(pg, flags)); } return new ParceledListSlice<>(out); @@ -537,7 +529,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } synchronized (mLock) { return PackageInfoUtils.generatePermissionGroupInfo( - mRegistry.getPermissionGroupLocked(groupName), flags); + mRegistry.getPermissionGroup(groupName), flags); } } @@ -554,7 +546,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { final int targetSdkVersion = getPermissionInfoCallingTargetSdkVersion(opPackage, callingUid); synchronized (mLock) { - final Permission bp = mRegistry.getPermissionLocked(permName); + final Permission bp = mRegistry.getPermission(permName); if (bp == null) { return null; } @@ -584,11 +576,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { return null; } synchronized (mLock) { - if (groupName != null && mRegistry.getPermissionGroupLocked(groupName) == null) { + if (groupName != null && mRegistry.getPermissionGroup(groupName) == null) { return null; } final ArrayList out = new ArrayList(10); - for (Permission bp : mRegistry.getPermissionsLocked()) { + for (Permission bp : mRegistry.getPermissions()) { if (Objects.equals(bp.getGroup(), groupName)) { out.add(bp.generatePermissionInfo(flags)); } @@ -606,24 +598,23 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (info.labelRes == 0 && info.nonLocalizedLabel == null) { throw new SecurityException("Label must be specified in permission"); } - final Permission tree = mRegistry.enforcePermissionTree(info.name, callingUid); final boolean added; final boolean changed; synchronized (mLock) { - Permission bp = mRegistry.getPermissionLocked(info.name); + final Permission tree = mRegistry.enforcePermissionTree(info.name, callingUid); + Permission bp = mRegistry.getPermission(info.name); added = bp == null; int fixedLevel = PermissionInfo.fixProtectionLevel(info.protectionLevel); if (added) { enforcePermissionCapLocked(info, tree); - bp = new Permission(info.name, tree.getPackageName(), - Permission.TYPE_DYNAMIC); + bp = new Permission(info.name, tree.getPackageName(), Permission.TYPE_DYNAMIC); } else if (!bp.isDynamic()) { throw new SecurityException("Not allowed to modify non-dynamic permission " + info.name); } changed = bp.addToTree(fixedLevel, info, tree); if (added) { - mRegistry.addPermissionLocked(bp); + mRegistry.addPermission(bp); } } if (changed) { @@ -638,9 +629,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (mPackageManagerInt.getInstantAppPackageName(callingUid) != null) { throw new SecurityException("Instant applications don't have access to this method"); } - final Permission tree = mRegistry.enforcePermissionTree(permName, callingUid); synchronized (mLock) { - final Permission bp = mRegistry.getPermissionLocked(permName); + mRegistry.enforcePermissionTree(permName, callingUid); + final Permission bp = mRegistry.getPermission(permName); if (bp == null) { return; } @@ -649,7 +640,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { Slog.wtf(TAG, "Not allowed to modify non-dynamic permission " + permName); } - mRegistry.removePermissionLocked(permName); + mRegistry.removePermission(permName); mPackageManagerInt.writeSettings(false); } } @@ -682,7 +673,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } synchronized (mLock) { - if (mRegistry.getPermissionLocked(permName) == null) { + if (mRegistry.getPermission(permName) == null) { return 0; } @@ -807,14 +798,16 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } - final Permission bp; + final boolean isRuntimePermission; final boolean permissionUpdated; synchronized (mLock) { - bp = mRegistry.getPermissionLocked(permName); + final Permission bp = mRegistry.getPermission(permName); if (bp == null) { throw new IllegalArgumentException("Unknown permission: " + permName); } + isRuntimePermission = bp.isRuntime(); + if (bp.isInstallerExemptIgnored()) { flagValues &= ~FLAG_PERMISSION_RESTRICTION_INSTALLER_EXEMPT; } @@ -833,14 +826,14 @@ public class PermissionManagerService extends IPermissionManager.Stub { permissionUpdated = uidState.updatePermissionFlags(bp, flagMask, flagValues); } - if (permissionUpdated && bp.isRuntime()) { + if (permissionUpdated && isRuntimePermission) { notifyRuntimePermissionStateChanged(packageName, userId); } if (permissionUpdated && callback != null) { // Install and runtime permissions are stored in different places, // so figure out what permission changed and persist the change. - if (!bp.isRuntime()) { - int userUid = UserHandle.getUid(userId, UserHandle.getAppId(pkg.getUid())); + if (!isRuntimePermission) { + int userUid = UserHandle.getUid(userId, pkg.getUid()); callback.onInstallPermissionUpdatedNotifyListener(userUid); } else { callback.onPermissionUpdatedNotifyListener(new int[]{userId}, false, pkg.getUid()); @@ -969,7 +962,10 @@ public class PermissionManagerService extends IPermissionManager.Stub { } if (isInstantApp) { - return mRegistry.isPermissionInstant(permissionName); + synchronized (mLock) { + final Permission permission = mRegistry.getPermission(permissionName); + return permission != null && permission.isInstant(); + } } return true; @@ -1231,7 +1227,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { private boolean checkExistsAndEnforceCannotModifyImmutablyRestrictedPermission( @NonNull String permName) { synchronized (mLock) { - final Permission bp = mRegistry.getPermissionLocked(permName); + final Permission bp = mRegistry.getPermission(permName); if (bp == null) { Slog.w(TAG, "No such permissions: " + permName); return false; @@ -1472,36 +1468,33 @@ public class PermissionManagerService extends IPermissionManager.Stub { throw new IllegalArgumentException("Unknown package: " + packageName); } - final Permission bp; + final boolean isSoftRestrictedPermission; synchronized (mLock) { - bp = mRegistry.getPermissionLocked(permName); - } - if (bp == null) { - throw new IllegalArgumentException("Unknown permission: " + permName); - } - - if (!(bp.isRuntime() || bp.isDevelopment())) { - throw new SecurityException("Permission " + permName + " requested by " - + pkg.getPackageName() + " is not a changeable permission type"); - } - - // If a permission review is required for legacy apps we represent - // their permissions as always granted runtime ones since we need - // to keep the review required permission flag per user while an - // install permission's state is shared across all users. - if (pkg.getTargetSdkVersion() < Build.VERSION_CODES.M && bp.isRuntime()) { - return; - } - - if (bp.isSoftRestricted() && !SoftRestrictedPermissionPolicy.forPermission(mContext, - pkg.toAppInfoWithoutState(), pkg, UserHandle.of(userId), permName) - .mayGrantPermission()) { - Log.e(TAG, "Cannot grant soft restricted permission " + permName + " for package " - + packageName); - return; + final Permission permission = mRegistry.getPermission(permName); + isSoftRestrictedPermission = permission != null && permission.isSoftRestricted(); } + final boolean mayGrantSoftRestrictedPermission = isSoftRestrictedPermission + && SoftRestrictedPermissionPolicy.forPermission(mContext, + pkg.toAppInfoWithoutState(), pkg, UserHandle.of(userId), permName) + .mayGrantPermission(); + final boolean isRuntimePermission; + final boolean isDevelopmentPermission; + final boolean permissionHasGids; synchronized (mLock) { + final Permission bp = mRegistry.getPermission(permName); + if (bp == null) { + throw new IllegalArgumentException("Unknown permission: " + permName); + } + + isRuntimePermission = bp.isRuntime(); + isDevelopmentPermission = bp.isDevelopment(); + permissionHasGids = bp.hasGids(); + if (!(isRuntimePermission || isDevelopmentPermission)) { + throw new SecurityException("Permission " + permName + " requested by " + + pkg.getPackageName() + " is not a changeable permission type"); + } + final UidPermissionState uidState = getUidStateLocked(pkg, userId); if (uidState == null) { Slog.e(TAG, "Missing permissions state for " + pkg.getPackageName() + " and user " @@ -1515,6 +1508,14 @@ public class PermissionManagerService extends IPermissionManager.Stub { + " has not requested permission " + permName); } + // If a permission review is required for legacy apps we represent + // their permissions as always granted runtime ones since we need + // to keep the review required permission flag per user while an + // install permission's state is shared across all users. + if (pkg.getTargetSdkVersion() < Build.VERSION_CODES.M && bp.isRuntime()) { + return; + } + final int flags = uidState.getPermissionFlags(permName); if ((flags & PackageManager.FLAG_PERMISSION_SYSTEM_FIXED) != 0) { Log.e(TAG, "Cannot grant system fixed permission " @@ -1534,6 +1535,12 @@ public class PermissionManagerService extends IPermissionManager.Stub { return; } + if (bp.isSoftRestricted() && !mayGrantSoftRestrictedPermission) { + Log.e(TAG, "Cannot grant soft restricted permission " + permName + " for package " + + packageName); + return; + } + if (bp.isDevelopment()) { // Development permissions must be handled specially, since they are not // normal runtime permissions. For now they apply to all users. @@ -1559,23 +1566,23 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } - if (bp.isRuntime()) { + if (isRuntimePermission) { logPermission(MetricsEvent.ACTION_PERMISSION_GRANTED, permName, packageName); } - final int uid = UserHandle.getUid(userId, UserHandle.getAppId(pkg.getUid())); + final int uid = UserHandle.getUid(userId, pkg.getUid()); if (callback != null) { - if (bp.isDevelopment()) { + if (isDevelopmentPermission) { callback.onInstallPermissionGranted(); } else { callback.onPermissionGranted(uid, userId); } - if (bp.hasGids()) { + if (permissionHasGids) { callback.onGidsChanged(UserHandle.getAppId(pkg.getUid()), userId); } } - if (bp.isRuntime()) { + if (isRuntimePermission) { notifyRuntimePermissionStateChanged(packageName, userId); } } @@ -1626,17 +1633,22 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (mPackageManagerInt.filterAppAccess(pkg, callingUid, userId)) { throw new IllegalArgumentException("Unknown package: " + packageName); } - final Permission bp = mRegistry.getPermission(permName); - if (bp == null) { - throw new IllegalArgumentException("Unknown permission: " + permName); - } - - if (!(bp.isRuntime() || bp.isDevelopment())) { - throw new SecurityException("Permission " + permName + " requested by " - + pkg.getPackageName() + " is not a changeable permission type"); - } + final boolean isRuntimePermission; + final boolean isDevelopmentPermission; synchronized (mLock) { + final Permission bp = mRegistry.getPermission(permName); + if (bp == null) { + throw new IllegalArgumentException("Unknown permission: " + permName); + } + + isRuntimePermission = bp.isRuntime(); + isDevelopmentPermission = bp.isDevelopment(); + if (!(isRuntimePermission || isDevelopmentPermission)) { + throw new SecurityException("Permission " + permName + " requested by " + + pkg.getPackageName() + " is not a changeable permission type"); + } + final UidPermissionState uidState = getUidStateLocked(pkg, userId); if (uidState == null) { Slog.e(TAG, "Missing permissions state for " + pkg.getPackageName() + " and user " @@ -1679,20 +1691,20 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } - if (bp.isRuntime()) { + if (isRuntimePermission) { logPermission(MetricsEvent.ACTION_PERMISSION_REVOKED, permName, packageName); } if (callback != null) { - if (bp.isDevelopment()) { + if (isDevelopmentPermission) { mDefaultPermissionCallback.onInstallPermissionRevoked(); } else { - callback.onPermissionRevoked(UserHandle.getUid(userId, - UserHandle.getAppId(pkg.getUid())), userId, reason); + callback.onPermissionRevoked(UserHandle.getUid(userId, pkg.getUid()), userId, + reason); } } - if (bp.isRuntime()) { + if (isRuntimePermission) { notifyRuntimePermissionStateChanged(packageName, userId); } } @@ -1807,16 +1819,18 @@ public class PermissionManagerService extends IPermissionManager.Stub { for (int i = 0; i < permissionCount; i++) { final String permName = pkg.getRequestedPermissions().get(i); - final Permission bp; - synchronized (mLock) { - bp = mRegistry.getPermissionLocked(permName); - } - if (bp == null) { - continue; - } - if (bp.isRemoved()) { - continue; + final boolean isRuntimePermission; + synchronized (mLock) { + final Permission permission = mRegistry.getPermission(permName); + if (permission == null) { + continue; + } + + if (permission.isRemoved()) { + continue; + } + isRuntimePermission = permission.isRuntime(); } // If shared user we just reset the state to which only this app contributed. @@ -1846,7 +1860,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { // permission as requiring a review as this is the initial state. final int uid = mPackageManagerInt.getPackageUid(packageName, 0, userId); final int targetSdk = mPackageManagerInt.getUidTargetSdkVersion(uid); - final int flags = (targetSdk < Build.VERSION_CODES.M && bp.isRuntime()) + final int flags = (targetSdk < Build.VERSION_CODES.M && isRuntimePermission) ? FLAG_PERMISSION_REVIEW_REQUIRED | FLAG_PERMISSION_REVOKED_COMPAT : 0; @@ -1855,7 +1869,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { false, delayingPermCallback); // Below is only runtime permission handling. - if (!bp.isRuntime()) { + if (!isRuntimePermission) { continue; } @@ -2092,13 +2106,15 @@ public class PermissionManagerService extends IPermissionManager.Stub { return false; } - Permission permission = getPermission(permName); - if (permission == null) { - return false; - } - if (permission.isHardRestricted() - && (flags & FLAGS_PERMISSION_RESTRICTION_ANY_EXEMPT) == 0) { - return false; + synchronized (mLock) { + final Permission permission = mRegistry.getPermission(permName); + if (permission == null) { + return false; + } + if (permission.isHardRestricted() + && (flags & FLAGS_PERMISSION_RESTRICTION_ANY_EXEMPT) == 0) { + return false; + } } final long token = Binder.clearCallingIdentity(); @@ -2346,10 +2362,12 @@ public class PermissionManagerService extends IPermissionManager.Stub { final int callingUid = Binder.getCallingUid(); for (int permNum = 0; permNum < numPermissions; permNum++) { - String permName = permissionsToRevoke.get(permNum); - Permission bp = mRegistry.getPermission(permName); - if (bp == null || !bp.isRuntime()) { - continue; + final String permName = permissionsToRevoke.get(permNum); + synchronized (mLock) { + final Permission bp = mRegistry.getPermission(permName); + if (bp == null || !bp.isRuntime()) { + continue; + } } for (int userIdNum = 0; userIdNum < numUserIds; userIdNum++) { final int userId = userIds[userIdNum]; @@ -2387,7 +2405,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } } - bp.setDefinitionChanged(false); } } @@ -2406,7 +2423,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { // permissions for one app being granted to someone just because they happen // to be in a group defined by another app (before this had no implications). if (pkg.getTargetSdkVersion() > Build.VERSION_CODES.LOLLIPOP_MR1) { - p.setParsedPermissionGroup(mRegistry.getPermissionGroupLocked(p.getGroup())); + p.setParsedPermissionGroup(mRegistry.getPermissionGroup(p.getGroup())); // Warn for a permission in an unknown group. if (DEBUG_PERMISSIONS && p.getGroup() != null && p.getParsedPermissionGroup() == null) { @@ -2421,21 +2438,22 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (p.isTree()) { bp = Permission.createOrUpdate( mPackageManagerInt, - mRegistry.getPermissionTreeLocked(p.getName()), permissionInfo, pkg, - mRegistry.getPermissionTreesLocked(), chatty); - mRegistry.addPermissionTreeLocked(bp); + mRegistry.getPermissionTree(p.getName()), permissionInfo, pkg, + mRegistry.getPermissionTrees(), chatty); + mRegistry.addPermissionTree(bp); } else { bp = Permission.createOrUpdate( mPackageManagerInt, - mRegistry.getPermissionLocked(p.getName()), - permissionInfo, pkg, mRegistry.getPermissionTreesLocked(), chatty); - mRegistry.addPermissionLocked(bp); + mRegistry.getPermission(p.getName()), + permissionInfo, pkg, mRegistry.getPermissionTrees(), chatty); + mRegistry.addPermission(bp); } if (bp.isInstalled()) { p.setFlags(p.getFlags() | PermissionInfo.FLAG_INSTALLED); } if (bp.isDefinitionChanged()) { definitionChangedPermissions.add(p.getName()); + bp.setDefinitionChanged(false); } } } @@ -2448,11 +2466,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { StringBuilder r = null; for (int i = 0; i < N; i++) { final ParsedPermissionGroup pg = pkg.getPermissionGroups().get(i); - final ParsedPermissionGroup cur = mRegistry.getPermissionGroupLocked(pg.getName()); + final ParsedPermissionGroup cur = mRegistry.getPermissionGroup(pg.getName()); final String curPackageName = (cur == null) ? null : cur.getPackageName(); final boolean isPackageUpdate = pg.getPackageName().equals(curPackageName); if (cur == null || isPackageUpdate) { - mRegistry.addPermissionGroupLocked(pg); + mRegistry.addPermissionGroup(pg); if (chatty && DEBUG_PACKAGE_SCANNING) { if (r == null) { r = new StringBuilder(256); @@ -2491,9 +2509,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { StringBuilder r = null; for (int i=0; i sourcePerms = newToSplitPerms.get(newPerm); if (sourcePerms != null) { - Permission bp = mRegistry.getPermissionLocked(newPerm); + Permission bp = mRegistry.getPermission(newPerm); if (bp.isRuntime()) { if (!newPerm.equals(Manifest.permission.ACTIVITY_RECOGNITION)) { @@ -3212,7 +3236,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { for (int sourcePermNum = 0; sourcePermNum < sourcePerms.size(); sourcePermNum++) { final String sourcePerm = sourcePerms.valueAt(sourcePermNum); - Permission sourceBp = mRegistry.getPermissionLocked(sourcePerm); + Permission sourceBp = mRegistry.getPermission(sourcePerm); if (!sourceBp.isRuntime()) { inheritsFromInstallPerm = true; break; @@ -3662,15 +3686,16 @@ public class PermissionManagerService extends IPermissionManager.Stub { final boolean instantApp = mPackageManagerInt.isInstantApp(pkg.getPackageName(), userId); for (String permission : pkg.getRequestedPermissions()) { - final Permission bp; + final boolean shouldGrantPermission; synchronized (mLock) { - bp = mRegistry.getPermissionLocked(permission); + final Permission bp = mRegistry.getPermission(permission); + shouldGrantPermission = bp != null && (bp.isRuntime() || bp.isDevelopment()) + && (!instantApp || bp.isInstant()) + && (supportsRuntimePermissions || !bp.isRuntimeOnly()) + && (grantedPermissions == null + || ArrayUtils.contains(grantedPermissions, permission)); } - if (bp != null && (bp.isRuntime() || bp.isDevelopment()) - && (!instantApp || bp.isInstant()) - && (supportsRuntimePermissions || !bp.isRuntimeOnly()) - && (grantedPermissions == null - || ArrayUtils.contains(grantedPermissions, permission))) { + if (shouldGrantPermission) { final int flags = getPermissionFlagsInternal(permission, pkg.getPackageName(), callingUid, userId); if (supportsRuntimePermissions) { @@ -3704,14 +3729,13 @@ public class PermissionManagerService extends IPermissionManager.Stub { for (int j = 0; j < permissionCount; j++) { final String permissionName = pkg.getRequestedPermissions().get(j); - final Permission bp = mRegistry.getPermissionLocked(permissionName); - - if (bp == null || !bp.isHardOrSoftRestricted()) { - continue; - } - final boolean isGranted; synchronized (mLock) { + final Permission bp = mRegistry.getPermission(permissionName); + if (bp == null || !bp.isHardOrSoftRestricted()) { + continue; + } + final UidPermissionState uidState = getUidStateLocked(pkg, userId); if (uidState == null) { Slog.e(TAG, "Missing permissions state for " + pkg.getPackageName() @@ -3865,11 +3889,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { int affectedUserId = UserHandle.USER_NULL; // Update permissions for (String eachPerm : deletedPs.pkg.getRequestedPermissions()) { - Permission bp = mRegistry.getPermission(eachPerm); - if (bp == null) { - continue; - } - // Check if another package in the shared user needs the permission. boolean used = false; final List pkgs = sus.getPackages(); @@ -3913,6 +3932,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { continue; } + Permission bp = mRegistry.getPermission(eachPerm); + if (bp == null) { + continue; + } + // TODO(zhanghai): Why are we only killing the UID when GIDs changed, instead of any // permission change? if (uidState.removePermissionState(bp.getName()) && bp.hasGids()) { @@ -3939,7 +3963,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { final int requestedPermCount = pkg.getRequestedPermissions().size(); for (int j = 0; j < requestedPermCount; j++) { String permission = pkg.getRequestedPermissions().get(j); - Permission bp = mRegistry.getPermissionLocked(permission); + Permission bp = mRegistry.getPermission(permission); if (bp != null) { usedPermissions.add(permission); } @@ -3954,7 +3978,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { for (int i = permissionStatesSize - 1; i >= 0; i--) { PermissionState permissionState = permissionStates.get(i); if (!usedPermissions.contains(permissionState.getName())) { - Permission bp = mRegistry.getPermissionLocked(permissionState.getName()); + Permission bp = mRegistry.getPermission(permissionState.getName()); if (bp != null) { if (uidState.removePermissionState(bp.getName()) && permissionState.isRuntime()) { @@ -4027,7 +4051,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { // Cache background -> foreground permission mapping. // Only system declares background permissions, hence mapping does never change. mBackgroundPermissions = new ArrayMap<>(); - for (Permission bp : mRegistry.getPermissionsLocked()) { + for (Permission bp : mRegistry.getPermissions()) { if (bp.getBackgroundPermission() != null) { String fgPerm = bp.getName(); String bgPerm = bp.getBackgroundPermission(); @@ -4173,9 +4197,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { boolean changed = false; Set needsUpdate = null; synchronized (mLock) { - for (final Permission bp : mRegistry.getPermissionsLocked()) { + for (final Permission bp : mRegistry.getPermissions()) { if (bp.isDynamic()) { - bp.updateDynamicPermission(mRegistry.getPermissionTreesLocked()); + bp.updateDynamicPermission(mRegistry.getPermissionTrees()); } if (!packageName.equals(bp.getPackageName())) { // Not checking sourcePackageSetting because it can be null when @@ -4225,7 +4249,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } }); } - mRegistry.removePermissionLocked(bp.getName()); + mRegistry.removePermission(bp.getName()); continue; } final AndroidPackage sourcePkg = @@ -4239,7 +4263,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } Slog.w(TAG, "Removing dangling permission: " + bp.getName() + " from package " + bp.getPackageName()); - mRegistry.removePermissionLocked(bp.getName()); + mRegistry.removePermission(bp.getName()); } } } @@ -4307,7 +4331,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { Set needsUpdate = null; synchronized (mLock) { - final Iterator it = mRegistry.getPermissionTreesLocked().iterator(); + final Iterator it = mRegistry.getPermissionTrees().iterator(); while (it.hasNext()) { final Permission bp = it.next(); if (!packageName.equals(bp.getPackageName())) { @@ -4343,7 +4367,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { } Slog.w(TAG, "Removing dangling permission tree: " + bp.getName() + " from package " + bp.getPackageName()); - mRegistry.removePermissionLocked(bp.getName()); + mRegistry.removePermission(bp.getName()); } } } @@ -4527,7 +4551,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { @GuardedBy({"mSettings.mLock", "mLock"}) private int calculateCurrentPermissionFootprintLocked(@NonNull Permission permissionTree) { int size = 0; - for (final Permission permission : mRegistry.getPermissionsLocked()) { + for (final Permission permission : mRegistry.getPermissions()) { size += permissionTree.calculateFootprint(permission); } return size; @@ -4686,7 +4710,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { @NonNull Collection permissionStates) { for (final LegacyPermissionState.PermissionState permissionState : permissionStates) { final String permissionName = permissionState.getName(); - final Permission permission = mRegistry.getPermissionLocked(permissionName); + final Permission permission = mRegistry.getPermission(permissionName); if (permission == null) { Slog.w(TAG, "Unknown permission: " + permissionName); continue; @@ -4771,9 +4795,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { permission.setGids(configPermission.getRawGids(), configPermission.areGidsPerUser()); } - mRegistry.addPermissionLocked(permission); + mRegistry.addPermission(permission); } else { - mRegistry.addPermissionTreeLocked(permission); + mRegistry.addPermissionTree(permission); } } } @@ -4787,7 +4811,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { final List legacyPermissions = new ArrayList<>(); synchronized (mLock) { final Collection permissions = writePermissionOrPermissionTree == 0 - ? mRegistry.getPermissionsLocked() : mRegistry.getPermissionTreesLocked(); + ? mRegistry.getPermissions() : mRegistry.getPermissionTrees(); for (final Permission permission : permissions) { // We don't need to provide UID and GIDs, which are only retrieved when dumping. final LegacyPermission legacyPermission = new LegacyPermission( @@ -4807,7 +4831,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { private void transferPermissions(@NonNull String oldPackageName, @NonNull String newPackageName) { synchronized (mLock) { - mRegistry.transferPermissionsLocked(oldPackageName, newPackageName); + mRegistry.transferPermissions(oldPackageName, newPackageName); } } @@ -4822,7 +4846,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { private List getLegacyPermissions() { synchronized (mLock) { final List legacyPermissions = new ArrayList<>(); - for (final Permission permission : mRegistry.getPermissionsLocked()) { + for (final Permission permission : mRegistry.getPermissions()) { final LegacyPermission legacyPermission = new LegacyPermission( permission.getPermissionInfo(), permission.getType(), permission.getUid(), permission.getRawGids()); @@ -4836,7 +4860,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { private Map> getAllAppOpPermissionPackages() { synchronized (mLock) { final ArrayMap> appOpPermissionPackages = - mRegistry.getAllAppOpPermissionPackagesLocked(); + mRegistry.getAllAppOpPermissionPackages(); final Map> deepClone = new ArrayMap<>(); final int appOpPermissionPackagesSize = appOpPermissionPackages.size(); for (int i = 0; i < appOpPermissionPackagesSize; i++) { @@ -5046,7 +5070,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { @Override public Permission getPermissionTEMP(String permName) { synchronized (PermissionManagerService.this.mLock) { - return mRegistry.getPermissionLocked(permName); + return mRegistry.getPermission(permName); } } @@ -5056,7 +5080,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { ArrayList matchingPermissions = new ArrayList<>(); synchronized (mLock) { - for (final Permission permission : mRegistry.getPermissionsLocked()) { + for (final Permission permission : mRegistry.getPermissions()) { if (permission.getProtection() == protection) { matchingPermissions.add(permission.generatePermissionInfo(0)); } @@ -5072,7 +5096,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { ArrayList matchingPermissions = new ArrayList<>(); synchronized (mLock) { - for (final Permission permission : mRegistry.getPermissionsLocked()) { + for (final Permission permission : mRegistry.getPermissions()) { if ((permission.getProtectionFlags() & protectionFlags) == protectionFlags) { matchingPermissions.add(permission.generatePermissionInfo(0)); } @@ -5270,7 +5294,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { Iterator iterator = permissionNames.iterator(); while (iterator.hasNext()) { final String permissionName = iterator.next(); - final Permission permission = mRegistry.getPermissionLocked(permissionName); + final Permission permission = mRegistry.getPermission(permissionName); if (permission == null || !permission.isHardOrSoftRestricted()) { iterator.remove(); } diff --git a/services/core/java/com/android/server/pm/permission/PermissionRegistry.java b/services/core/java/com/android/server/pm/permission/PermissionRegistry.java index 36719203e4b5b..0e3fda7b937a7 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionRegistry.java +++ b/services/core/java/com/android/server/pm/permission/PermissionRegistry.java @@ -22,8 +22,6 @@ import android.content.pm.parsing.component.ParsedPermissionGroup; import android.util.ArrayMap; import android.util.ArraySet; -import com.android.internal.annotations.GuardedBy; - import java.util.Collection; /** @@ -34,87 +32,62 @@ public class PermissionRegistry { * All of the permissions known to the system. The mapping is from permission * name to permission object. */ - @GuardedBy("mLock") private final ArrayMap mPermissions = new ArrayMap<>(); /** * All permission trees known to the system. The mapping is from permission tree * name to permission object. */ - @GuardedBy("mLock") private final ArrayMap mPermissionTrees = new ArrayMap<>(); /** * All permisson groups know to the system. The mapping is from permission group * name to permission group object. */ - @GuardedBy("mLock") private final ArrayMap mPermissionGroups = new ArrayMap<>(); /** * Set of packages that request a particular app op. The mapping is from permission * name to package names. */ - @GuardedBy("mLock") private final ArrayMap> mAppOpPermissionPackages = new ArrayMap<>(); @NonNull - private final Object mLock; - - public PermissionRegistry(@NonNull Object lock) { - mLock = lock; - } - - @GuardedBy("mLock") - @NonNull - public Collection getPermissionsLocked() { + public Collection getPermissions() { return mPermissions.values(); } - @GuardedBy("mLock") - @Nullable - public Permission getPermissionLocked(@NonNull String permName) { - return mPermissions.get(permName); - } - @Nullable public Permission getPermission(@NonNull String permissionName) { - synchronized (mLock) { - return getPermissionLocked(permissionName); - } + return mPermissions.get(permissionName); } - @GuardedBy("mLock") - public void addPermissionLocked(@NonNull Permission permission) { + public void addPermission(@NonNull Permission permission) { mPermissions.put(permission.getName(), permission); } - @GuardedBy("mLock") - public void removePermissionLocked(@NonNull String permissionName) { + public void removePermission(@NonNull String permissionName) { mPermissions.remove(permissionName); } - @GuardedBy("mLock") @NonNull - public Collection getPermissionTreesLocked() { + public Collection getPermissionTrees() { return mPermissionTrees.values(); } - @GuardedBy("mLock") @Nullable - public Permission getPermissionTreeLocked(@NonNull String permissionTreeName) { + public Permission getPermissionTree(@NonNull String permissionTreeName) { return mPermissionTrees.get(permissionTreeName); } - @GuardedBy("mLock") - public void addPermissionTreeLocked(@NonNull Permission permissionTree) { + public void addPermissionTree(@NonNull Permission permissionTree) { mPermissionTrees.put(permissionTree.getName(), permissionTree); } /** * Transfers ownership of permissions from one package to another. */ - public void transferPermissionsLocked(@NonNull String oldPackageName, + public void transferPermissions(@NonNull String oldPackageName, @NonNull String newPackageName) { for (int i = 0; i < 2; i++) { ArrayMap permissions = i == 0 ? mPermissionTrees : mPermissions; @@ -124,37 +97,31 @@ public class PermissionRegistry { } } - @GuardedBy("mLock") @NonNull - public Collection getPermissionGroupsLocked() { + public Collection getPermissionGroups() { return mPermissionGroups.values(); } - @GuardedBy("mLock") @Nullable - public ParsedPermissionGroup getPermissionGroupLocked(@NonNull String permissionGroupName) { + public ParsedPermissionGroup getPermissionGroup(@NonNull String permissionGroupName) { return mPermissionGroups.get(permissionGroupName); } - @GuardedBy("mLock") - public void addPermissionGroupLocked(@NonNull ParsedPermissionGroup permissionGroup) { + public void addPermissionGroup(@NonNull ParsedPermissionGroup permissionGroup) { mPermissionGroups.put(permissionGroup.getName(), permissionGroup); } - @GuardedBy("mLock") @NonNull - public ArrayMap> getAllAppOpPermissionPackagesLocked() { + public ArrayMap> getAllAppOpPermissionPackages() { return mAppOpPermissionPackages; } - @GuardedBy("mLock") @Nullable - public ArraySet getAppOpPermissionPackagesLocked(@NonNull String permissionName) { + public ArraySet getAppOpPermissionPackages(@NonNull String permissionName) { return mAppOpPermissionPackages.get(permissionName); } - @GuardedBy("mLock") - public void addAppOpPermissionPackageLocked(@NonNull String permissionName, + public void addAppOpPermissionPackage(@NonNull String permissionName, @NonNull String packageName) { ArraySet packageNames = mAppOpPermissionPackages.get(permissionName); if (packageNames == null) { @@ -164,8 +131,7 @@ public class PermissionRegistry { packageNames.add(packageName); } - @GuardedBy("mLock") - public void removeAppOpPermissionPackageLocked(@NonNull String permissionName, + public void removeAppOpPermissionPackage(@NonNull String permissionName, @NonNull String packageName) { final ArraySet packageNames = mAppOpPermissionPackages.get(permissionName); if (packageNames == null) { @@ -184,23 +150,7 @@ public class PermissionRegistry { */ @NonNull public Permission enforcePermissionTree(@NonNull String permissionName, int callingUid) { - synchronized (mLock) { - return Permission.enforcePermissionTree(mPermissionTrees.values(), permissionName, - callingUid); - } - } - - public boolean isPermissionInstant(@NonNull String permissionName) { - synchronized (mLock) { - final Permission permission = mPermissions.get(permissionName); - return permission != null && permission.isInstant(); - } - } - - boolean isPermissionAppOp(@NonNull String permissionName) { - synchronized (mLock) { - final Permission permission = mPermissions.get(permissionName); - return permission != null && permission.isAppOp(); - } + return Permission.enforcePermissionTree(mPermissionTrees.values(), permissionName, + callingUid); } } From a73d98002ebf301b08c9cf1a5495efe78a934a5f Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Thu, 29 Oct 2020 17:40:51 -0700 Subject: [PATCH 2/2] Finish cleaning up locking inside PermissionManagerService. Bug: 158736025 Test: LockFinder Test: Errorprone @GuardedBy Test: presubmit Change-Id: I75290c1c7d634fa448ee254c2b10494ea2ad8cc6 --- .../server/pm/permission/Permission.java | 44 ++++---- .../permission/PermissionManagerService.java | 103 +++++++++--------- 2 files changed, 76 insertions(+), 71 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/Permission.java b/services/core/java/com/android/server/pm/permission/Permission.java index c121e6b4a7635..4e8ddac885297 100644 --- a/services/core/java/com/android/server/pm/permission/Permission.java +++ b/services/core/java/com/android/server/pm/permission/Permission.java @@ -378,28 +378,33 @@ public final class Permission { } } + public static boolean isOverridingSystemPermission(@Nullable Permission permission, + @NonNull PermissionInfo permissionInfo, + @NonNull PackageManagerInternal packageManagerInternal) { + if (permission == null || Objects.equals(permission.mPermissionInfo.packageName, + permissionInfo.packageName)) { + return false; + } + if (!permission.mReconciled) { + return false; + } + final AndroidPackage currentPackage = packageManagerInternal.getPackage( + permission.mPermissionInfo.packageName); + if (currentPackage == null) { + return false; + } + return currentPackage.isSystem(); + } + @NonNull - static Permission createOrUpdate(PackageManagerInternal packageManagerInternal, - @Nullable Permission permission, @NonNull PermissionInfo permissionInfo, - @NonNull AndroidPackage pkg, @NonNull Collection permissionTrees, + public static Permission createOrUpdate(@Nullable Permission permission, + @NonNull PermissionInfo permissionInfo, @NonNull AndroidPackage pkg, + @NonNull Collection permissionTrees, boolean isOverridingSystemPermission, boolean chatty) { // Allow system apps to redefine non-system permissions boolean ownerChanged = false; if (permission != null && !Objects.equals(permission.mPermissionInfo.packageName, permissionInfo.packageName)) { - final boolean currentOwnerIsSystem; - if (!permission.mReconciled) { - currentOwnerIsSystem = false; - } else { - AndroidPackage currentPackage = packageManagerInternal.getPackage( - permission.mPermissionInfo.packageName); - if (currentPackage == null) { - currentOwnerIsSystem = false; - } else { - currentOwnerIsSystem = currentPackage.isSystem(); - } - } - if (pkg.isSystem()) { if (permission.mType == Permission.TYPE_CONFIG && !permission.mReconciled) { // It's a built-in permission and no owner, take ownership now @@ -407,11 +412,10 @@ public final class Permission { permission.mPermissionInfo = permissionInfo; permission.mReconciled = true; permission.mUid = pkg.getUid(); - } else if (!currentOwnerIsSystem) { - String msg = "New decl " + pkg + " of permission " + } else if (!isOverridingSystemPermission) { + Slog.w(TAG, "New decl " + pkg + " of permission " + permissionInfo.name + " is system; overriding " - + permission.mPermissionInfo.packageName; - PackageManagerService.reportSettingsProblem(Log.WARN, msg); + + permission.mPermissionInfo.packageName); ownerChanged = true; permission = null; } diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java index db4878680a2ed..300206a679a28 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -224,6 +224,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { private PermissionControllerManager mPermissionControllerManager; /** Map of OneTimePermissionUserManagers keyed by userId */ + @GuardedBy("mLock") + @NonNull private final SparseArray mOneTimePermissionUserManagers = new SparseArray<>(); @@ -252,12 +254,14 @@ public class PermissionManagerService extends IPermissionManager.Stub { /** Internal storage for permissions and related settings */ @GuardedBy("mLock") + @NonNull private final PermissionRegistry mRegistry = new PermissionRegistry(); /** Injector that can be used to facilitate testing. */ private final Injector mInjector; @GuardedBy("mLock") + @Nullable private ArraySet mPrivappPermissionsViolations; @GuardedBy("mLock") @@ -309,7 +313,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { // purposes. It may make sense to keep as an abstraction, but, the methods // necessary to be overridden may be different than what was initially needed // for the split. - private PermissionCallback mDefaultPermissionCallback = new PermissionCallback() { + private final PermissionCallback mDefaultPermissionCallback = new PermissionCallback() { @Override public void onGidsChanged(int appId, int userId) { mHandler.post(() -> killUid(appId, userId, KILL_APP_REASON_GIDS_CHANGED)); @@ -641,8 +645,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { + permName); } mRegistry.removePermission(permName); - mPackageManagerInt.writeSettings(false); } + mPackageManagerInt.writeSettings(false); } @Override @@ -962,10 +966,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { } if (isInstantApp) { - synchronized (mLock) { - final Permission permission = mRegistry.getPermission(permissionName); - return permission != null && permission.isInstant(); - } + final Permission permission = mRegistry.getPermission(permissionName); + return permission != null && permission.isInstant(); } return true; @@ -1226,21 +1228,23 @@ public class PermissionManagerService extends IPermissionManager.Stub { private boolean checkExistsAndEnforceCannotModifyImmutablyRestrictedPermission( @NonNull String permName) { + final boolean isImmutablyRestrictedPermission; synchronized (mLock) { final Permission bp = mRegistry.getPermission(permName); if (bp == null) { Slog.w(TAG, "No such permissions: " + permName); return false; } - if (bp.isHardOrSoftRestricted() && bp.isImmutablyRestricted() - && mContext.checkCallingOrSelfPermission( - Manifest.permission.WHITELIST_RESTRICTED_PERMISSIONS) - != PackageManager.PERMISSION_GRANTED) { - throw new SecurityException("Cannot modify whitelisting of an immutably " - + "restricted permission: " + permName); - } - return true; + isImmutablyRestrictedPermission = bp.isHardOrSoftRestricted() + && bp.isImmutablyRestricted(); } + if (isImmutablyRestrictedPermission && mContext.checkCallingOrSelfPermission( + Manifest.permission.WHITELIST_RESTRICTED_PERMISSIONS) + != PackageManager.PERMISSION_GRANTED) { + throw new SecurityException("Cannot modify whitelisting of an immutably " + + "restricted permission: " + permName); + } + return true; } @Override @@ -2193,8 +2197,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { private void restoreRuntimePermissions(@NonNull byte[] backup, @NonNull UserHandle user) { synchronized (mLock) { mHasNoDelayedPermBackup.delete(user.getIdentifier()); - mPermissionControllerManager.stageAndApplyRuntimePermissionsBackup(backup, user); } + mPermissionControllerManager.stageAndApplyRuntimePermissionsBackup(backup, user); } /** @@ -2213,18 +2217,16 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (mHasNoDelayedPermBackup.get(user.getIdentifier(), false)) { return; } - - mPermissionControllerManager.applyStagedRuntimePermissionBackup(packageName, user, - mContext.getMainExecutor(), (hasMoreBackup) -> { - if (hasMoreBackup) { - return; - } - - synchronized (mLock) { - mHasNoDelayedPermBackup.put(user.getIdentifier(), true); - } - }); } + mPermissionControllerManager.applyStagedRuntimePermissionBackup(packageName, user, + mContext.getMainExecutor(), (hasMoreBackup) -> { + if (hasMoreBackup) { + return; + } + synchronized (mLock) { + mHasNoDelayedPermBackup.put(user.getIdentifier(), true); + } + }); } private void addOnRuntimePermissionStateChangedListener(@NonNull @@ -2417,6 +2419,8 @@ public class PermissionManagerService extends IPermissionManager.Stub { // Assume by default that we did not install this permission into the system. p.setFlags(p.getFlags() & ~PermissionInfo.FLAG_INSTALLED); + final PermissionInfo permissionInfo; + final Permission oldPermission; synchronized (mLock) { // Now that permission groups have a special meaning, we ignore permission // groups for legacy apps to prevent unexpected behavior. In particular, @@ -2432,28 +2436,30 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } - final PermissionInfo permissionInfo = PackageInfoUtils.generatePermissionInfo(p, + permissionInfo = PackageInfoUtils.generatePermissionInfo(p, PackageManager.GET_META_DATA); - final Permission bp; + oldPermission = p.isTree() ? mRegistry.getPermissionTree(p.getName()) + : mRegistry.getPermission(p.getName()); + } + // TODO(zhanghai): Maybe we should store whether a permission is owned by system inside + // itself. + final boolean isOverridingSystemPermission = Permission.isOverridingSystemPermission( + oldPermission, permissionInfo, mPackageManagerInt); + synchronized (mLock) { + final Permission permission = Permission.createOrUpdate(oldPermission, + permissionInfo, pkg, mRegistry.getPermissionTrees(), + isOverridingSystemPermission, chatty); if (p.isTree()) { - bp = Permission.createOrUpdate( - mPackageManagerInt, - mRegistry.getPermissionTree(p.getName()), permissionInfo, pkg, - mRegistry.getPermissionTrees(), chatty); - mRegistry.addPermissionTree(bp); + mRegistry.addPermissionTree(permission); } else { - bp = Permission.createOrUpdate( - mPackageManagerInt, - mRegistry.getPermission(p.getName()), - permissionInfo, pkg, mRegistry.getPermissionTrees(), chatty); - mRegistry.addPermission(bp); + mRegistry.addPermission(permission); } - if (bp.isInstalled()) { + if (permission.isInstalled()) { p.setFlags(p.getFlags() | PermissionInfo.FLAG_INSTALLED); } - if (bp.isDefinitionChanged()) { + if (permission.isDefinitionChanged()) { definitionChangedPermissions.add(p.getName()); - bp.setDefinitionChanged(false); + permission.setDefinitionChanged(false); } } } @@ -2740,12 +2746,10 @@ public class PermissionManagerService extends IPermissionManager.Stub { // the runtime ones are written only if changed. The only cases of // changed runtime permissions here are promotion of an install to // runtime and revocation of a runtime from a shared user. - synchronized (mLock) { - if (revokeUnusedSharedUserPermissionsLocked( - ps.getSharedUser().getPackages(), uidState)) { - updatedUserIds = ArrayUtils.appendInt(updatedUserIds, userId); - runtimePermissionsRevoked = true; - } + if (revokeUnusedSharedUserPermissionsLocked( + ps.getSharedUser().getPackages(), uidState)) { + updatedUserIds = ArrayUtils.appendInt(updatedUserIds, userId); + runtimePermissionsRevoked = true; } } } @@ -3948,7 +3952,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { return affectedUserId; } - @GuardedBy("mLock") private boolean revokeUnusedSharedUserPermissionsLocked( List pkgList, UidPermissionState uidState) { // Collect all used permissions in the UID @@ -4548,7 +4551,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { return builder.toString(); } - @GuardedBy({"mSettings.mLock", "mLock"}) private int calculateCurrentPermissionFootprintLocked(@NonNull Permission permissionTree) { int size = 0; for (final Permission permission : mRegistry.getPermissions()) { @@ -4557,7 +4559,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { return size; } - @GuardedBy({"mSettings.mLock", "mLock"}) private void enforcePermissionCapLocked(PermissionInfo info, Permission tree) { // We calculate the max size of permissions defined by this uid and throw // if that plus the size of 'info' would exceed our stated maximum. @@ -5069,7 +5070,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { @Override public Permission getPermissionTEMP(String permName) { - synchronized (PermissionManagerService.this.mLock) { + synchronized (mLock) { return mRegistry.getPermission(permName); } }