From 83e006842fe3502cf941d609d03abb6129ec59e0 Mon Sep 17 00:00:00 2001 From: Winson Date: Tue, 22 Dec 2020 11:18:03 -0800 Subject: [PATCH 1/2] Add more domain verification debug functionality Bug: 163565712 Test: none, just for debugging Change-Id: I7b88d4253b5f02fda69b0305ef6e82e7a464ea6a --- .../pm/verify/domain/DomainVerificationDebug.java | 14 +++++++++++++- .../verify/domain/DomainVerificationService.java | 2 +- .../domain/proxy/DomainVerificationProxy.java | 5 +++-- .../domain/proxy/DomainVerificationProxyV1.java | 5 +++-- .../domain/proxy/DomainVerificationProxyV2.java | 3 ++- 5 files changed, 22 insertions(+), 7 deletions(-) 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 af9978b91e48f..ab0f4b53336b5 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 @@ -32,15 +32,27 @@ import android.util.SparseArray; import com.android.internal.util.CollectionUtils; 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.models.DomainVerificationStateMap; import com.android.server.pm.verify.domain.models.DomainVerificationUserState; -import com.android.server.pm.parsing.pkg.AndroidPackage; import java.util.Arrays; +@SuppressWarnings("PointlessBooleanExpression") public class DomainVerificationDebug { + // Disable to turn off all logging. This is used to allow a "basic" set of debug flags to be + // enabled and checked in, without having everything be on or off. + public static final boolean DEBUG_ANY = false; + + // Enable to turn on all logging. Requires enabling DEBUG_ANY. + public static final boolean DEBUG_ALL = false; + + public static final boolean DEBUG_APPROVAL = DEBUG_ANY && (DEBUG_ALL || true); + public static final boolean DEBUG_BROADCASTS = DEBUG_ANY && (DEBUG_ALL || false); + public static final boolean DEBUG_PROXIES = DEBUG_ANY && (DEBUG_ALL || false); + @NonNull private final DomainVerificationCollector mCollector; 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 53540c8e0d4f0..d15e600c1ec8f 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 @@ -70,7 +70,7 @@ public class DomainVerificationService extends SystemService private static final String TAG = "DomainVerificationService"; - public static final boolean DEBUG_APPROVAL = true; + public static final boolean DEBUG_APPROVAL = DomainVerificationDebug.DEBUG_APPROVAL; /** * The new user preference API for verifying domains marked autoVerify=true in diff --git a/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxy.java b/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxy.java index 715d8fb0fc2db..09abdd092648e 100644 --- a/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxy.java +++ b/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxy.java @@ -23,9 +23,10 @@ import android.content.Context; import android.util.Slog; import com.android.server.DeviceIdleInternal; -import com.android.server.pm.verify.domain.DomainVerificationMessageCodes; import com.android.server.pm.verify.domain.DomainVerificationCollector; +import com.android.server.pm.verify.domain.DomainVerificationDebug; import com.android.server.pm.verify.domain.DomainVerificationManagerInternal; +import com.android.server.pm.verify.domain.DomainVerificationMessageCodes; import java.util.Objects; import java.util.Set; @@ -35,7 +36,7 @@ public interface DomainVerificationProxy { String TAG = "DomainVerificationProxy"; - boolean DEBUG_PROXIES = false; + boolean DEBUG_PROXIES = DomainVerificationDebug.DEBUG_PROXIES; static DomainVerificationProxy makeProxy( diff --git a/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV1.java b/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV1.java index eab89e9878852..65ea78d6eee60 100644 --- a/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV1.java +++ b/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV1.java @@ -37,10 +37,11 @@ import android.util.Pair; import android.util.Slog; import com.android.internal.annotations.GuardedBy; +import com.android.server.pm.parsing.pkg.AndroidPackage; import com.android.server.pm.verify.domain.DomainVerificationCollector; +import com.android.server.pm.verify.domain.DomainVerificationDebug; import com.android.server.pm.verify.domain.DomainVerificationManagerInternal; import com.android.server.pm.verify.domain.DomainVerificationMessageCodes; -import com.android.server.pm.parsing.pkg.AndroidPackage; import java.util.Collections; import java.util.List; @@ -52,7 +53,7 @@ public class DomainVerificationProxyV1 implements DomainVerificationProxy { private static final String TAG = "DomainVerificationProxyV1"; - private static final boolean DEBUG_BROADCASTS = false; + private static final boolean DEBUG_BROADCASTS = DomainVerificationDebug.DEBUG_BROADCASTS; @NonNull private final Context mContext; diff --git a/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV2.java b/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV2.java index 9fcbce2ad0556..1ef06036021eb 100644 --- a/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV2.java +++ b/services/core/java/com/android/server/pm/verify/domain/proxy/DomainVerificationProxyV2.java @@ -28,6 +28,7 @@ import android.os.Process; import android.os.UserHandle; import android.util.Slog; +import com.android.server.pm.verify.domain.DomainVerificationDebug; import com.android.server.pm.verify.domain.DomainVerificationMessageCodes; import java.util.Set; @@ -36,7 +37,7 @@ public class DomainVerificationProxyV2 implements DomainVerificationProxy { private static final String TAG = "DomainVerificationProxyV2"; - private static final boolean DEBUG_BROADCASTS = true; + private static final boolean DEBUG_BROADCASTS = DomainVerificationDebug.DEBUG_BROADCASTS; @NonNull private final Context mContext; From 23f55288efdeeaa9663ee37d528bdb06ceb15987 Mon Sep 17 00:00:00 2001 From: Winson Date: Fri, 5 Feb 2021 08:55:45 -0800 Subject: [PATCH 2/2] 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); } }