From 23f55288efdeeaa9663ee37d528bdb06ceb15987 Mon Sep 17 00:00:00 2001 From: Winson Date: Fri, 5 Feb 2021 08:55:45 -0800 Subject: [PATCH] Avoid locking PMS when printing from DomainVerificationService Passes in a String -> PackageSetting function rather than calling into PackageManagerService directly and potentially taking the locked computer. Note that this change itself does not prevent locking, just allows that change to be made. Bug: 179461758 Test: builds, mostly debug functionality Change-Id: Ib9172e797edd26c5cd8621c16e2e06181e9b91a4 --- .../server/pm/PackageManagerService.java | 20 +++++------- .../domain/DomainVerificationDebug.java | 9 ++++-- .../DomainVerificationManagerInternal.java | 31 +++++++++++++++---- .../domain/DomainVerificationService.java | 20 ++++++++++-- 4 files changed, 56 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index a702f5ea35828..d38ec14753482 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -24,7 +24,6 @@ import static android.Manifest.permission.SET_HARMFUL_APP_WARNINGS; import static android.app.AppOpsManager.MODE_DEFAULT; import static android.app.AppOpsManager.MODE_IGNORED; import static android.content.Intent.ACTION_MAIN; -import static android.content.Intent.CATEGORY_BROWSABLE; import static android.content.Intent.CATEGORY_DEFAULT; import static android.content.Intent.CATEGORY_HOME; import static android.content.Intent.EXTRA_LONG_VERSION_CODE; @@ -68,11 +67,7 @@ import static android.content.pm.PackageManager.INSTALL_REASON_DEVICE_RESTORE; import static android.content.pm.PackageManager.INSTALL_REASON_DEVICE_SETUP; import static android.content.pm.PackageManager.INSTALL_STAGED; import static android.content.pm.PackageManager.INSTALL_SUCCEEDED; -import static android.content.pm.PackageManager.INTENT_FILTER_DOMAIN_VERIFICATION_STATUS_ALWAYS; -import static android.content.pm.PackageManager.INTENT_FILTER_DOMAIN_VERIFICATION_STATUS_ALWAYS_ASK; -import static android.content.pm.PackageManager.INTENT_FILTER_DOMAIN_VERIFICATION_STATUS_ASK; import static android.content.pm.PackageManager.INTENT_FILTER_DOMAIN_VERIFICATION_STATUS_NEVER; -import static android.content.pm.PackageManager.INTENT_FILTER_DOMAIN_VERIFICATION_STATUS_UNDEFINED; import static android.content.pm.PackageManager.MATCH_ALL; import static android.content.pm.PackageManager.MATCH_ANY_USER; import static android.content.pm.PackageManager.MATCH_APEX; @@ -377,11 +372,6 @@ import com.android.server.pm.dex.DexManager; import com.android.server.pm.dex.DexoptOptions; import com.android.server.pm.dex.PackageDexUsage; import com.android.server.pm.dex.ViewCompiler; -import com.android.server.pm.verify.domain.DomainVerificationManagerInternal; -import com.android.server.pm.verify.domain.DomainVerificationService; -import com.android.server.pm.verify.domain.proxy.DomainVerificationProxy; -import com.android.server.pm.verify.domain.proxy.DomainVerificationProxyV1; -import com.android.server.pm.verify.domain.proxy.DomainVerificationProxyV2; import com.android.server.pm.parsing.PackageCacher; import com.android.server.pm.parsing.PackageInfoUtils; import com.android.server.pm.parsing.PackageParser2; @@ -395,6 +385,11 @@ import com.android.server.pm.permission.LegacyPermissionManagerService; import com.android.server.pm.permission.Permission; import com.android.server.pm.permission.PermissionManagerService; import com.android.server.pm.permission.PermissionManagerServiceInternal; +import com.android.server.pm.verify.domain.DomainVerificationManagerInternal; +import com.android.server.pm.verify.domain.DomainVerificationService; +import com.android.server.pm.verify.domain.proxy.DomainVerificationProxy; +import com.android.server.pm.verify.domain.proxy.DomainVerificationProxyV1; +import com.android.server.pm.verify.domain.proxy.DomainVerificationProxyV2; import com.android.server.rollback.RollbackManagerInternal; import com.android.server.security.VerityUtils; import com.android.server.storage.DeviceStorageMonitorInternal; @@ -404,7 +399,6 @@ import com.android.server.utils.Watchable; import com.android.server.utils.Watched; import com.android.server.utils.WatchedArrayMap; import com.android.server.utils.WatchedSparseBooleanArray; -import com.android.server.utils.WatchedSparseIntArray; import com.android.server.utils.Watcher; import com.android.server.wm.ActivityTaskManagerInternal; @@ -466,7 +460,6 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BiConsumer; import java.util.function.Consumer; import java.util.function.Predicate; -import java.util.function.Supplier; /** * Keep track of all those APKs everywhere. @@ -23937,7 +23930,8 @@ public class PackageManagerService extends IPackageManager.Stub writer.println("Domain verification status:"); writer.increaseIndent(); try { - mDomainVerificationManager.printState(writer, packageName, UserHandle.USER_ALL); + mDomainVerificationManager.printState(writer, packageName, UserHandle.USER_ALL, + mSettings::getPackageLPr); } catch (PackageManager.NameNotFoundException e) { pw.println("Failure printing domain verification information"); Slog.e(TAG, "Failure printing domain verification information", e); diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationDebug.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationDebug.java index ab0f4b53336b5..1925590112f8d 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationDebug.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationDebug.java @@ -38,6 +38,9 @@ import com.android.server.pm.verify.domain.models.DomainVerificationStateMap; import com.android.server.pm.verify.domain.models.DomainVerificationUserState; import java.util.Arrays; +import java.util.Objects; +import java.util.Set; +import java.util.function.Function; @SuppressWarnings("PointlessBooleanExpression") public class DomainVerificationDebug { @@ -62,7 +65,7 @@ public class DomainVerificationDebug { public void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, @Nullable @UserIdInt Integer userId, - @NonNull DomainVerificationService.Connection connection, + @NonNull Function pkgSettingFunction, @NonNull DomainVerificationStateMap stateMap) throws NameNotFoundException { ArrayMap reusedMap = new ArrayMap<>(); @@ -73,7 +76,7 @@ public class DomainVerificationDebug { for (int index = 0; index < size; index++) { DomainVerificationPkgState pkgState = stateMap.valueAt(index); String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = connection.getPackageSettingLocked(pkgName); + PackageSetting pkgSetting = pkgSettingFunction.apply(pkgName); if (pkgSetting == null || pkgSetting.getPkg() == null) { continue; } @@ -89,7 +92,7 @@ public class DomainVerificationDebug { throw DomainVerificationUtils.throwPackageUnavailable(packageName); } - PackageSetting pkgSetting = connection.getPackageSettingLocked(packageName); + PackageSetting pkgSetting = pkgSettingFunction.apply(packageName); if (pkgSetting == null || pkgSetting.getPkg() == null) { throw DomainVerificationUtils.throwPackageUnavailable(packageName); } diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java index 7ad275a6f351c..0474d78a3e534 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java @@ -32,15 +32,16 @@ import android.util.TypedXmlPullParser; import android.util.TypedXmlSerializer; import com.android.server.pm.PackageSetting; +import com.android.server.pm.parsing.pkg.AndroidPackage; import com.android.server.pm.verify.domain.models.DomainVerificationPkgState; import com.android.server.pm.verify.domain.proxy.DomainVerificationProxy; -import com.android.server.pm.parsing.pkg.AndroidPackage; import org.xmlpull.v1.XmlPullParserException; import java.io.IOException; import java.util.Set; import java.util.UUID; +import java.util.function.Function; public interface DomainVerificationManagerInternal extends DomainVerificationManager { @@ -191,12 +192,21 @@ public interface DomainVerificationManagerInternal extends DomainVerificationMan /** * Print the verification state and user selection state of a package. * - * @param packageName the package whose state to change, or all packages if none is specified - * @param userId the specific user to print, or null to skip printing user selection - * states, supports {@link android.os.UserHandle#USER_ALL} + * @param packageName the package whose state to change, or all packages if none is + * specified + * @param userId the specific user to print, or null to skip printing user selection + * states, supports {@link android.os.UserHandle#USER_ALL} + * @param pkgSettingFunction the method by which to retrieve package data; if this is called + * from {@link com.android.server.pm.PackageManagerService}, it is + * expected to pass in the snapshot of {@link PackageSetting} objects, + * or if null is passed, the manager may decide to lock {@link + * com.android.server.pm.PackageManagerService} through {@link + * Connection#getPackageSettingLocked(String)} */ void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, - @Nullable @UserIdInt Integer userId) throws NameNotFoundException; + @Nullable @UserIdInt Integer userId, + @Nullable Function pkgSettingFunction) + throws NameNotFoundException; @NonNull DomainVerificationShell getShell(); @@ -225,7 +235,7 @@ public interface DomainVerificationManagerInternal extends DomainVerificationMan throws IllegalArgumentException, NameNotFoundException; - interface Connection { + interface Connection extends Function { /** * Notify that a settings change has been made and that eventually @@ -249,10 +259,19 @@ public interface DomainVerificationManagerInternal extends DomainVerificationMan */ void schedule(int code, @Nullable Object object); + // TODO(b/178733426): Make DomainVerificationService PMS snapshot aware so it can avoid + // locking package state at all. This can be as simple as removing this method in favor of + // accepting a PackageSetting function in at every method call, although should probably + // be abstracted to a wrapper class. @Nullable PackageSetting getPackageSettingLocked(@NonNull String pkgName); @Nullable AndroidPackage getPackageLocked(@NonNull String pkgName); + + @Override + default PackageSetting apply(@NonNull String pkgName) { + return getPackageSettingLocked(pkgName); + } } } diff --git a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java index d15e600c1ec8f..8f191580e2c9b 100644 --- a/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java @@ -64,6 +64,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.UUID; +import java.util.function.Function; public class DomainVerificationService extends SystemService implements DomainVerificationManagerInternal, DomainVerificationShell.Callback { @@ -890,9 +891,24 @@ public class DomainVerificationService extends SystemService @Override public void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, - @Nullable @UserIdInt Integer userId) throws NameNotFoundException { + @Nullable Integer userId) throws NameNotFoundException { + // This method is only used by DomainVerificationShell, which doesn't lock PMS, so it's + // safe to pass mConnection directly here and lock PMS. This method is not exposed + // to the general system server/PMS. + printState(writer, packageName, userId, mConnection); + } + + @Override + public void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, + @Nullable @UserIdInt Integer userId, + @Nullable Function pkgSettingFunction) + throws NameNotFoundException { + if (pkgSettingFunction == null) { + pkgSettingFunction = mConnection; + } + synchronized (mLock) { - mDebug.printState(writer, packageName, userId, mConnection, mAttachedPkgStates); + mDebug.printState(writer, packageName, userId, pkgSettingFunction, mAttachedPkgStates); } }