Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix more locking issues identified by @GuardedBy.

Browse Source 
- Add one missing locking for mRegistry.removePermission().
- Add @GuardedBy annotations for *Locked methods.
- Remove unused mBackgroundPermissions.
- Don't lock access to mOnPermissionChangeListeners because
  RemoteCallbackList has its own locking.
- Lock access to mPrivappPermissionsViolations.

Bug: 158736025
Test: presubmit
Test: errorprone build logs/warnings.html
Change-Id: I356b0d3100fa555b163e6c40dad78f4b516c521b
This commit is contained in:
Hai Zhang
2020-10-30 20:20:39 -07:00
parent 87e1f782f4
commit 27154a1372
1 changed files with 32 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

99
services/core/java/com/android/server/pm/permission/PermissionManagerService.java
View File

@@ -270,13 +270,6 @@ public class PermissionManagerService extends IPermissionManager.Stub {
@GuardedBy("mLock")
private PermissionPolicyInternal mPermissionPolicyInternal;
/**
* For each foreground/background permission the mapping:
* Background permission -> foreground permissions
*/
@GuardedBy("mLock")
private ArrayMap<String, List<String>> mBackgroundPermissions;
/**
* A permission backup might contain apps that are not installed. In this case we delay the
* restoration until the app is installed.
@@ -295,7 +288,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
@GuardedBy("mLock")
private CheckPermissionDelegate mCheckPermissionDelegate;
@GuardedBy("mLock")
@NonNull
private final OnPermissionChangeListeners mOnPermissionChangeListeners;
@GuardedBy("mLock")
@@ -959,6 +952,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
return PackageManager.PERMISSION_DENIED;
}
@GuardedBy("mLock")
private boolean checkSinglePermissionInternalLocked(@NonNull UidPermissionState uidState,
@NonNull String permissionName, boolean isInstantApp) {
if (!uidState.isPermissionGranted(permissionName)) {
@@ -1029,6 +1023,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
return PackageManager.PERMISSION_DENIED;
}
@GuardedBy("mLock")
private boolean checkSingleUidPermissionInternalLocked(int uid,
@NonNull String permissionName) {
ArraySet<String> permissions = mSystemPermissions.get(uid);
@@ -1094,9 +1089,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
Manifest.permission.OBSERVE_GRANT_REVOKE_PERMISSIONS,
"addOnPermissionsChangeListener");
synchronized (mLock) {
mOnPermissionChangeListeners.addListenerLocked(listener);
}
mOnPermissionChangeListeners.addListener(listener);
}
@Override
@@ -1104,9 +1097,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
if (mPackageManagerInt.getInstantAppPackageName(Binder.getCallingUid()) != null) {
throw new SecurityException("Instant applications don't have access to this method");
}
synchronized (mLock) {
mOnPermissionChangeListeners.removeListenerLocked(listener);
}
mOnPermissionChangeListeners.removeListener(listener);
}
@Override
@@ -3068,6 +3059,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
*
* @return The updated value of the {@code updatedUserIds} parameter
*/
@GuardedBy("mLock")
private @NonNull int[] revokePermissionsNoLongerImplicitLocked(@NonNull UidPermissionState ps,
@NonNull AndroidPackage pkg, int userId, @NonNull int[] updatedUserIds) {
String pkgName = pkg.getPackageName();
@@ -3120,6 +3112,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
* @param ps The permission state of the package
* @param pkg The package requesting the permissions
*/
@GuardedBy("mLock")
private void inheritPermissionStateToNewImplicitPermissionLocked(
@NonNull ArraySet<String> sourcePerms, @NonNull String newPerm,
@NonNull UidPermissionState ps, @NonNull AndroidPackage pkg) {
@@ -3191,6 +3184,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
*
* @return List of users for which the permission state has been changed
*/
@GuardedBy("mLock")
private @NonNull int[] setInitialGrantForNewImplicitPermissionsLocked(
@NonNull UidPermissionState origPs, @NonNull UidPermissionState ps,
@NonNull AndroidPackage pkg, @NonNull ArraySet<String> newImplicitPermissions,
@@ -3582,11 +3576,13 @@ public class PermissionManagerService extends IPermissionManager.Stub {
+ packageName + " (" + pkg.getPath()
+ ") not in privapp-permissions whitelist");
if (RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_ENFORCE) {
if (mPrivappPermissionsViolations == null) {
mPrivappPermissionsViolations = new ArraySet<>();
synchronized (mLock) {
if (mPrivappPermissionsViolations == null) {
mPrivappPermissionsViolations = new ArraySet<>();
}
mPrivappPermissionsViolations.add(packageName + " (" + pkg.getPath() + "): "
+ permissionName);
}
mPrivappPermissionsViolations.add(packageName + " (" + pkg.getPath() + "): "
+ permissionName);
}
}
}
@@ -3952,6 +3948,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
return affectedUserId;
}
@GuardedBy("mLock")
private boolean revokeUnusedSharedUserPermissionsLocked(
List<AndroidPackage> pkgList, UidPermissionState uidState) {
// Collect all used permissions in the UID
@@ -4043,35 +4040,6 @@ public class PermissionManagerService extends IPermissionManager.Stub {
}
}
/**
* Cache background->foreground permission mapping.
*
* <p>This is only run once.
*/
private void cacheBackgroundToForegoundPermissionMapping() {
synchronized (mLock) {
if (mBackgroundPermissions == null) {
// Cache background -> foreground permission mapping.
// Only system declares background permissions, hence mapping does never change.
mBackgroundPermissions = new ArrayMap<>();
for (Permission bp : mRegistry.getPermissions()) {
if (bp.getBackgroundPermission() != null) {
String fgPerm = bp.getName();
String bgPerm = bp.getBackgroundPermission();
List<String> fgPerms = mBackgroundPermissions.get(bgPerm);
if (fgPerms == null) {
fgPerms = new ArrayList<>();
mBackgroundPermissions.put(bgPerm, fgPerms);
}
fgPerms.add(fgPerm);
}
}
}
}
}
/**
* Update all packages on the volume, <u>beside</u> the changing package. If the changing
* package is set too, all packages are updated.
@@ -4145,8 +4113,6 @@ public class PermissionManagerService extends IPermissionManager.Stub {
flags |= UPDATE_PERMISSIONS_ALL;
}
cacheBackgroundToForegoundPermissionMapping();
Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "restorePermissionState");
// Now update the permissions for all packages.
if ((flags & UPDATE_PERMISSIONS_ALL) != 0) {
@@ -4252,7 +4218,9 @@ public class PermissionManagerService extends IPermissionManager.Stub {
}
});
}
mRegistry.removePermission(bp.getName());
synchronized (mLock) {
mRegistry.removePermission(bp.getName());
}
continue;
}
final AndroidPackage sourcePkg =
@@ -4551,6 +4519,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
return builder.toString();
}
@GuardedBy("mLock")
private int calculateCurrentPermissionFootprintLocked(@NonNull Permission permissionTree) {
int size = 0;
for (final Permission permission : mRegistry.getPermissions()) {
@@ -4559,6 +4528,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
return size;
}
@GuardedBy("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.
@@ -4572,9 +4542,12 @@ public class PermissionManagerService extends IPermissionManager.Stub {
private void systemReady() {
mSystemReady = true;
if (mPrivappPermissionsViolations != null) {
throw new IllegalStateException("Signature|privileged permissions not in "
+ "privapp-permissions whitelist: " + mPrivappPermissionsViolations);
synchronized (mLock) {
if (mPrivappPermissionsViolations != null) {
throw new IllegalStateException("Signature|privileged permissions not in "
+ "privapp-permissions whitelist: " + mPrivappPermissionsViolations);
}
}
mPermissionControllerManager = mContext.getSystemService(PermissionControllerManager.class);
@@ -4642,29 +4615,21 @@ public class PermissionManagerService extends IPermissionManager.Stub {
mMetricsLogger.write(log);
}
/**
* Get the mapping of background permissions to their foreground permissions.
*
* <p>Only initialized in the system server.
*
* @return the map &lt;bg permission -> list&lt;fg perm&gt;&gt;
*/
public @Nullable ArrayMap<String, List<String>> getBackgroundPermissions() {
return mBackgroundPermissions;
}
@GuardedBy("mLock")
@Nullable
private UidPermissionState getUidStateLocked(@NonNull PackageSetting ps,
@UserIdInt int userId) {
return getUidStateLocked(ps.getAppId(), userId);
}
@GuardedBy("mLock")
@Nullable
private UidPermissionState getUidStateLocked(@NonNull AndroidPackage pkg,
@UserIdInt int userId) {
return getUidStateLocked(pkg.getUid(), userId);
}
@GuardedBy("mLock")
@Nullable
private UidPermissionState getUidStateLocked(@AppIdInt int appId, @UserIdInt int userId) {
final UserPermissionState userState = mState.getUserState(userId);
@@ -4707,6 +4672,7 @@ public class PermissionManagerService extends IPermissionManager.Stub {
});
}
@GuardedBy("mLock")
private void readLegacyPermissionStatesLocked(@NonNull UidPermissionState uidState,
@NonNull Collection<LegacyPermissionState.PermissionState> permissionStates) {
for (final LegacyPermissionState.PermissionState permissionState : permissionStates) {
@@ -5371,12 +5337,11 @@ public class PermissionManagerService extends IPermissionManager.Stub {
}
}
public void addListenerLocked(IOnPermissionsChangeListener listener) {
public void addListener(IOnPermissionsChangeListener listener) {
mPermissionListeners.register(listener);
}
public void removeListenerLocked(IOnPermissionsChangeListener listener) {
public void removeListener(IOnPermissionsChangeListener listener) {
mPermissionListeners.unregister(listener);
}