From c3f20ed79972709768b3ab0ad0763211a85f08a4 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Wed, 10 May 2023 20:03:03 -0700 Subject: [PATCH] Fix deadlock in app op listeners. The onStateMutated() callback is called while holding our state lock, and the listener implementation are supposed to schedule callbacks to another thread, without grabbing any external lock. However, the current AppOpsService listener implementation has to either grab the AppOpsService.this lock for reading listeners (which may be fixed by creating a granular lock), or call into package manager for package UID. We are already posting notifyOpChangedForAllPkgsInUid() for UID state changes, so it should be fine to post here as well. Also added dumping package name in new dumpsys, and dropped unknown permission when reading from disk. Fixes: 281919202 Test: presubmit Change-Id: If5f1f31dfe35dad12c6f1c03108dfad827906279 --- .../android/server/appop/AppOpsService.java | 90 ++++++++++--------- .../permission/AppIdPermissionPersistence.kt | 27 +++++- .../access/permission/PermissionService.kt | 31 +++++-- 3 files changed, 96 insertions(+), 52 deletions(-) diff --git a/services/core/java/com/android/server/appop/AppOpsService.java b/services/core/java/com/android/server/appop/AppOpsService.java index 111cbfdf7467d..0a44da37d3377 100644 --- a/services/core/java/com/android/server/appop/AppOpsService.java +++ b/services/core/java/com/android/server/appop/AppOpsService.java @@ -931,53 +931,17 @@ public class AppOpsService extends IAppOpsService.Stub { new AppOpsCheckingServiceInterface.AppOpsModeChangedListener() { @Override public void onUidModeChanged(int uid, int code, int mode) { - notifyOpChangedForAllPkgsInUid(code, uid, false); + mHandler.sendMessage(PooledLambda.obtainMessage( + AppOpsService::notifyOpChangedForAllPkgsInUid, AppOpsService.this, + code, uid, false)); } @Override public void onPackageModeChanged(String packageName, int userId, int code, int mode) { - ArraySet repCbs = null; - int uid = -1; - synchronized (AppOpsService.this) { - ArraySet cbs = - mOpModeWatchers.get(code); - if (cbs != null) { - if (repCbs == null) { - repCbs = new ArraySet<>(); - } - repCbs.addAll(cbs); - } - cbs = mPackageModeWatchers.get(packageName); - if (cbs != null) { - if (repCbs == null) { - repCbs = new ArraySet<>(); - } - repCbs.addAll(cbs); - } - if (repCbs != null && mIgnoredCallback != null) { - repCbs.remove(mModeWatchers.get(mIgnoredCallback.asBinder())); - } - uid = getPackageManagerInternal().getPackageUid(packageName, - PackageManager.MATCH_KNOWN_PACKAGES, userId); - Op op = getOpLocked(code, uid, packageName, null, false, null, - /* edit */ false); - if (op != null && mode == AppOpsManager.opToDefaultMode(op.op)) { - // If going into the default mode, prune this op - // if there is nothing else interesting in it. - pruneOpLocked(op, uid, packageName); - } - scheduleFastWriteLocked(); - if (mode != MODE_ERRORED) { - updateStartedOpModeForUidLocked(code, mode == MODE_IGNORED, uid); - } - } - - if (repCbs != null && uid != -1) { - mHandler.sendMessage(PooledLambda.obtainMessage( - AppOpsService::notifyOpChanged, - AppOpsService.this, repCbs, code, uid, packageName)); - } + mHandler.sendMessage(PooledLambda.obtainMessage( + AppOpsService::notifyOpChangedForPkg, AppOpsService.this, + packageName, code, mode, userId)); } }); //mAppOpsCheckingService = new AppOpsCheckingServiceLoggingDecorator( @@ -1988,6 +1952,48 @@ public class AppOpsService extends IAppOpsService.Stub { } } + private void notifyOpChangedForPkg(@NonNull String packageName, int code, int mode, + @UserIdInt int userId) { + ArraySet repCbs = null; + int uid = -1; + synchronized (AppOpsService.this) { + ArraySet cbs = mOpModeWatchers.get(code); + if (cbs != null) { + if (repCbs == null) { + repCbs = new ArraySet<>(); + } + repCbs.addAll(cbs); + } + cbs = mPackageModeWatchers.get(packageName); + if (cbs != null) { + if (repCbs == null) { + repCbs = new ArraySet<>(); + } + repCbs.addAll(cbs); + } + if (repCbs != null && mIgnoredCallback != null) { + repCbs.remove(mModeWatchers.get(mIgnoredCallback.asBinder())); + } + uid = getPackageManagerInternal().getPackageUid(packageName, + PackageManager.MATCH_KNOWN_PACKAGES, userId); + Op op = getOpLocked(code, uid, packageName, null, false, null, /* edit */ false); + if (op != null && mode == AppOpsManager.opToDefaultMode(op.op)) { + // If going into the default mode, prune this op + // if there is nothing else interesting in it. + pruneOpLocked(op, uid, packageName); + } + scheduleFastWriteLocked(); + if (mode != MODE_ERRORED) { + updateStartedOpModeForUidLocked(code, mode == MODE_IGNORED, uid); + } + } + + if (repCbs != null && uid != -1) { + mHandler.sendMessage(PooledLambda.obtainMessage(AppOpsService::notifyOpChanged, this, + repCbs, code, uid, packageName)); + } + } + private void updatePermissionRevokedCompat(int uid, int switchCode, int mode) { PackageManager packageManager = mContext.getPackageManager(); if (packageManager == null) { diff --git a/services/permission/java/com/android/server/permission/access/permission/AppIdPermissionPersistence.kt b/services/permission/java/com/android/server/permission/access/permission/AppIdPermissionPersistence.kt index 8fbe513d919ce..615bbdc71f854 100644 --- a/services/permission/java/com/android/server/permission/access/permission/AppIdPermissionPersistence.kt +++ b/services/permission/java/com/android/server/permission/access/permission/AppIdPermissionPersistence.kt @@ -43,23 +43,42 @@ import com.android.server.permission.access.util.tagName class AppIdPermissionPersistence { fun BinaryXmlPullParser.parseSystemState(state: MutableAccessState) { - val systemState = state.mutateSystemState(WriteMode.NONE) when (tagName) { - TAG_PERMISSION_TREES -> parsePermissions(systemState.mutatePermissionTrees()) - TAG_PERMISSIONS -> parsePermissions(systemState.mutatePermissions()) + TAG_PERMISSION_TREES -> parsePermissions(state, true) + TAG_PERMISSIONS -> parsePermissions(state, false) else -> {} } } private fun BinaryXmlPullParser.parsePermissions( - permissions: MutableIndexedMap + state: MutableAccessState, + isPermissionTree: Boolean ) { + val systemState = state.mutateSystemState(WriteMode.NONE) + val permissions = if (isPermissionTree) { + systemState.mutatePermissionTrees() + } else { + systemState.mutatePermissions() + } forEachTag { when (val tagName = tagName) { TAG_PERMISSION -> parsePermission(permissions) else -> Log.w(LOG_TAG, "Ignoring unknown tag $tagName when parsing permissions") } } + permissions.forEachReversedIndexed { permissionIndex, _, permission -> + val packageName = permission.packageName + val externalState = state.externalState + if (packageName !in externalState.packageStates && + packageName !in externalState.disabledSystemPackageStates) { + Log.w( + LOG_TAG, + "Dropping permission with unknown package $packageName when parsing permissions" + ) + permissions.removeAt(permissionIndex) + systemState.requestWriteMode(WriteMode.ASYNCHRONOUS) + } + } } private fun BinaryXmlPullParser.parsePermission( diff --git a/services/permission/java/com/android/server/permission/access/permission/PermissionService.kt b/services/permission/java/com/android/server/permission/access/permission/PermissionService.kt index 4d5b2c9c79cc4..f125fc38f94dd 100644 --- a/services/permission/java/com/android/server/permission/access/permission/PermissionService.kt +++ b/services/permission/java/com/android/server/permission/access/permission/PermissionService.kt @@ -1815,18 +1815,36 @@ class PermissionService( withIndent { state.systemState.permissions.forEachIndexed { _, _, permission -> val protectionLevel = PermissionInfo.protectionToString(permission.protectionLevel) - println("${permission.name}: appId=${permission.appId}, " + - "type=${Permission.typeToString(permission.type)}, " + - "gids=${permission.gids.contentToString()}, " + - "protection=[$protectionLevel], " + - "flags=${PermissionInfo.flagsToString(permission.permissionInfo.flags)}" + println( + "${permission.name}: " + + "type=${Permission.typeToString(permission.type)}, " + + "packageName=${permission.packageName}, " + + "appId=${permission.appId}, " + + "gids=${permission.gids.contentToString()}, " + + "protectionLevel=[$protectionLevel], " + + "flags=${PermissionInfo.flagsToString(permission.permissionInfo.flags)}" ) } } + + println("Permission groups:") + withIndent { + state.systemState.permissionGroups.forEachIndexed { _, _, permissionGroup -> + println( + "${permissionGroup.name}: " + + "packageName=${permissionGroup.packageName}" + ) + } + } + println("Permission trees:") withIndent { state.systemState.permissionTrees.forEachIndexed { _, _, permissionTree -> - println("${permissionTree.name}: appId=${permissionTree.appId}") + println( + "${permissionTree.name}: " + + "packageName=${permissionTree.packageName}, " + + "appId=${permissionTree.appId}" + ) } } } @@ -1859,6 +1877,7 @@ class PermissionService( println("$appOpName: mode=${AppOpsManager.modeToName(appOpMode)}") } } + packageNames?.forEachIndexed { _, packageName -> println("Package: $packageName") withIndent {