From a74c1f3c69a3fa575f61c56ab321097ae84f13a3 Mon Sep 17 00:00:00 2001 From: Winson Date: Thu, 1 Apr 2021 14:24:35 -0700 Subject: [PATCH 1/2] Move withPackageSettingsSnapshot methods into PackageManagerInternal For consumers who don't care about blocking package data updates and only need data at a specific snapshot in time, these methods would avoid locking PMS during the iteration process. Bug: 183643808 Test: atest com.android.server.pm.test.verify.domain Change-Id: Ia951381798d75f77c4ad5fc010a469b69992f074 --- .../internal/util/FunctionalUtils.java | 42 ++++ .../content/pm/PackageManagerInternal.java | 8 +- .../pm/PackageSettingsSnapshotProvider.java | 75 ++++++ .../server/pm/PackageManagerService.java | 213 +++++++++++------- .../DomainVerificationManagerInternal.java | 59 +---- .../domain/DomainVerificationService.java | 63 +++--- .../domain/DomainVerificationTestUtils.kt | 16 +- 7 files changed, 295 insertions(+), 181 deletions(-) create mode 100644 services/core/java/android/content/pm/PackageSettingsSnapshotProvider.java diff --git a/core/java/com/android/internal/util/FunctionalUtils.java b/core/java/com/android/internal/util/FunctionalUtils.java index 05ecdf9056236..91cc4b93dafbf 100644 --- a/core/java/com/android/internal/util/FunctionalUtils.java +++ b/core/java/com/android/internal/util/FunctionalUtils.java @@ -247,6 +247,48 @@ public class FunctionalUtils { } } + /** + * A {@link Consumer} that allows the caller to specify a custom checked {@link Exception} that + * can be thrown by the implementer. This is usually used when proxying/wrapping calls between + * different classes. + * + * @param Method parameter type + * @param Checked exception type + */ + @FunctionalInterface + public interface ThrowingCheckedConsumer { + void accept(Input input) throws ExceptionType; + } + + /** + * A {@link Consumer} that allows the caller to specify 2 different custom checked + * {@link Exception}s that can be thrown by the implementer. This is usually used when + * proxying/wrapping calls between different classes. + * + * @param Method parameter type + * @param First checked exception type + * @param Second checked exception type + */ + @FunctionalInterface + public interface ThrowingChecked2Consumer { + void accept(Input input) throws ExceptionOne, ExceptionTwo; + } + + /** + * A {@link Function} that allows the caller to specify a custom checked {@link Exception} that + * can be thrown by the implementer. This is usually used when proxying/wrapping calls between + * different classes. + * + * @param Method parameter type + * @param Method return type + * @param Checked exception type + */ + @FunctionalInterface + public interface ThrowingCheckedFunction { + Output apply(Input input) throws ExceptionType; + } + // TODO: add unit test /** * Gets a user-friendly name for a lambda function. diff --git a/services/core/java/android/content/pm/PackageManagerInternal.java b/services/core/java/android/content/pm/PackageManagerInternal.java index b4fcb9cd3bb7b..816c50dde2a80 100644 --- a/services/core/java/android/content/pm/PackageManagerInternal.java +++ b/services/core/java/android/content/pm/PackageManagerInternal.java @@ -60,7 +60,7 @@ import java.util.function.Consumer; * * @hide Only for use within the system server. */ -public abstract class PackageManagerInternal { +public abstract class PackageManagerInternal implements PackageSettingsSnapshotProvider { @IntDef(prefix = "PACKAGE_", value = { PACKAGE_SYSTEM, PACKAGE_SETUP_WIZARD, @@ -795,6 +795,9 @@ public abstract class PackageManagerInternal { * Perform the given action for each package. * Note that packages lock will be held while performing the actions. * + * If the caller does not need all packages, prefer the potentially non-locking + * {@link #withPackageSettingsSnapshot(Consumer)}. + * * @param actionLocked action to be performed */ public abstract void forEachPackage(Consumer actionLocked); @@ -803,6 +806,9 @@ public abstract class PackageManagerInternal { * Perform the given action for each {@link PackageSetting}. * Note that packages lock will be held while performing the actions. * + * If the caller does not need all packages, prefer the potentially non-locking + * {@link #withPackageSettingsSnapshot(Consumer)}. + * * @param actionLocked action to be performed */ public abstract void forEachPackageSetting(Consumer actionLocked); diff --git a/services/core/java/android/content/pm/PackageSettingsSnapshotProvider.java b/services/core/java/android/content/pm/PackageSettingsSnapshotProvider.java new file mode 100644 index 0000000000000..b9130d76dbf44 --- /dev/null +++ b/services/core/java/android/content/pm/PackageSettingsSnapshotProvider.java @@ -0,0 +1,75 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.content.pm; + +import android.annotation.NonNull; + +import com.android.internal.util.FunctionalUtils; +import com.android.server.pm.PackageManagerService; +import com.android.server.pm.PackageSetting; + +import java.util.function.Consumer; +import java.util.function.Function; + +/** @hide */ +public interface PackageSettingsSnapshotProvider { + + /** + * Run a function block that requires access to {@link PackageSetting} data. This will + * ensure the {@link PackageManagerService} lock is taken before any caller's internal lock + * to avoid deadlock. Note that this method may or may not lock. If a snapshot is available + * and valid, it will iterate the snapshot set of data. + */ + void withPackageSettingsSnapshot( + @NonNull Consumer> block); + + /** + * Variant which returns a value to the caller. + * @see #withPackageSettingsSnapshot(Consumer) + */ + Output withPackageSettingsSnapshotReturning( + @NonNull FunctionalUtils.ThrowingFunction, Output> + block); + + /** + * Variant which throws. + * @see #withPackageSettingsSnapshot(Consumer) + */ + void withPackageSettingsSnapshotThrowing( + @NonNull FunctionalUtils.ThrowingCheckedConsumer, + ExceptionType> block) throws ExceptionType; + + /** + * Variant which throws 2 exceptions. + * @see #withPackageSettingsSnapshot(Consumer) + */ + void + withPackageSettingsSnapshotThrowing2( + @NonNull FunctionalUtils.ThrowingChecked2Consumer< + Function, ExceptionOne, ExceptionTwo> block) + throws ExceptionOne, ExceptionTwo; + + /** + * Variant which returns a value to the caller and throws. + * @see #withPackageSettingsSnapshot(Consumer) + */ + Output + withPackageSettingsSnapshotReturningThrowing( + @NonNull FunctionalUtils.ThrowingCheckedFunction< + Function, Output, ExceptionType> block) + throws ExceptionType; +} diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index 03153d375d337..30aa8fdc4aaf0 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -1731,91 +1731,6 @@ public class PackageManagerService extends IPackageManager.Stub return PackageManagerService.this.getPackage(packageName); } - @NonNull - @Override - public void withPackageSettings(@NonNull Consumer> block) { - final Computer snapshot = snapshotComputer(); - - // This method needs to either lock or not lock consistently throughout the method, - // so if the live computer is returned, force a wrapping sync block. - if (snapshot == mLiveComputer) { - synchronized (mLock) { - block.accept(snapshot::getPackageSetting); - } - } else { - block.accept(snapshot::getPackageSetting); - } - } - - @Override - public Output withPackageSettingsReturning( - @NonNull FunctionalUtils.ThrowingFunction, Output> - block) { - final Computer snapshot = snapshotComputer(); - - // This method needs to either lock or not lock consistently throughout the method, - // so if the live computer is returned, force a wrapping sync block. - if (snapshot == mLiveComputer) { - synchronized (mLock) { - return block.apply(snapshot::getPackageSetting); - } - } else { - return block.apply(snapshot::getPackageSetting); - } - } - - @Override - public void withPackageSettingsThrowing( - @NonNull ThrowingConsumer, ExceptionType> block) - throws ExceptionType { - final Computer snapshot = snapshotComputer(); - - // This method needs to either lock or not lock consistently throughout the method, - // so if the live computer is returned, force a wrapping sync block. - if (snapshot == mLiveComputer) { - synchronized (mLock) { - block.accept(snapshot::getPackageSetting); - } - } else { - block.accept(snapshot::getPackageSetting); - } - } - - @Override - public void - withPackageSettingsThrowing2( - @NonNull Throwing2Consumer, ExceptionOne, - ExceptionTwo> block) throws ExceptionOne, ExceptionTwo { - final Computer snapshot = snapshotComputer(); - - // This method needs to either lock or not lock consistently throughout the method, - // so if the live computer is returned, force a wrapping sync block. - if (snapshot == mLiveComputer) { - synchronized (mLock) { - block.accept(snapshot::getPackageSetting); - } - } else { - block.accept(snapshot::getPackageSetting); - } - } - - @Override - public Output - withPackageSettingsReturningThrowing(@NonNull ThrowingFunction, Output, ExceptionType> block) throws ExceptionType { - final Computer snapshot = snapshotComputer(); - - // This method needs to either lock or not lock consistently throughout the method, - // so if the live computer is returned, force a wrapping sync block. - if (snapshot == mLiveComputer) { - synchronized (mLock) { - return block.apply(snapshot::getPackageSetting); - } - } else { - return block.apply(snapshot::getPackageSetting); - } - } - @Override public boolean filterAppAccess(String packageName, int callingUid, int userId) { return mPmInternal.filterAppAccess(packageName, callingUid, userId); @@ -1830,6 +1745,44 @@ public class PackageManagerService extends IPackageManager.Stub public boolean doesUserExist(@UserIdInt int userId) { return mUserManager.exists(userId); } + + @Override + public void withPackageSettingsSnapshot( + @NonNull Consumer> block) { + mPmInternal.withPackageSettingsSnapshot(block); + } + + @Override + public Output withPackageSettingsSnapshotReturning( + @NonNull FunctionalUtils.ThrowingFunction, Output> + block) { + return mPmInternal.withPackageSettingsSnapshotReturning(block); + } + + @Override + public void withPackageSettingsSnapshotThrowing( + @NonNull FunctionalUtils.ThrowingCheckedConsumer, + ExceptionType> block) throws ExceptionType { + mPmInternal.withPackageSettingsSnapshotThrowing(block); + } + + @Override + public void + withPackageSettingsSnapshotThrowing2( + @NonNull FunctionalUtils.ThrowingChecked2Consumer< + Function, ExceptionOne, ExceptionTwo> block) + throws ExceptionOne, ExceptionTwo { + mPmInternal.withPackageSettingsSnapshotThrowing2(block); + } + + @Override + public Output + withPackageSettingsSnapshotReturningThrowing( + @NonNull FunctionalUtils.ThrowingCheckedFunction< + Function, Output, ExceptionType> block) + throws ExceptionType { + return mPmInternal.withPackageSettingsSnapshotReturningThrowing(block); + } } /** @@ -1843,7 +1796,7 @@ public class PackageManagerService extends IPackageManager.Stub private final Watcher mWatcher = new Watcher() { @Override - public void onChange(@Nullable Watchable what) { + public void onChange(@Nullable Watchable what) { PackageManagerService.this.onChange(what); } }; @@ -27290,6 +27243,94 @@ public class PackageManagerService extends IPackageManager.Stub public void deleteOatArtifactsOfPackage(String packageName) { PackageManagerService.this.deleteOatArtifactsOfPackage(packageName); } + + @Override + public void withPackageSettingsSnapshot( + @NonNull Consumer> block) { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + block.accept(snapshot::getPackageSetting); + } + } else { + block.accept(snapshot::getPackageSetting); + } + } + + @Override + public Output withPackageSettingsSnapshotReturning( + @NonNull FunctionalUtils.ThrowingFunction, Output> + block) { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + return block.apply(snapshot::getPackageSetting); + } + } else { + return block.apply(snapshot::getPackageSetting); + } + } + + @Override + public void withPackageSettingsSnapshotThrowing( + @NonNull FunctionalUtils.ThrowingCheckedConsumer, + ExceptionType> block) throws ExceptionType { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + block.accept(snapshot::getPackageSetting); + } + } else { + block.accept(snapshot::getPackageSetting); + } + } + + @Override + public void + withPackageSettingsSnapshotThrowing2( + @NonNull FunctionalUtils.ThrowingChecked2Consumer< + Function, ExceptionOne, ExceptionTwo> block) + throws ExceptionOne, ExceptionTwo { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + block.accept(snapshot::getPackageSetting); + } + } else { + block.accept(snapshot::getPackageSetting); + } + } + + @Override + public Output + withPackageSettingsSnapshotReturningThrowing( + @NonNull FunctionalUtils.ThrowingCheckedFunction< + Function, Output, ExceptionType> block) + throws ExceptionType { + final Computer snapshot = snapshotComputer(); + + // This method needs to either lock or not lock consistently throughout the method, + // so if the live computer is returned, force a wrapping sync block. + if (snapshot == mLiveComputer) { + synchronized (mLock) { + return block.apply(snapshot::getPackageSetting); + } + } else { + return block.apply(snapshot::getPackageSetting); + } + } } @GuardedBy("mLock") 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 b54b1698a3f42..65e4e95759b8c 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 @@ -25,6 +25,7 @@ import android.content.Intent; import android.content.pm.IntentFilterVerificationInfo; import android.content.pm.PackageManager; import android.content.pm.PackageManager.NameNotFoundException; +import android.content.pm.PackageSettingsSnapshotProvider; import android.content.pm.ResolveInfo; import android.content.pm.verify.domain.DomainVerificationInfo; import android.content.pm.verify.domain.DomainVerificationManager; @@ -36,7 +37,6 @@ import android.util.Pair; import android.util.TypedXmlPullParser; import android.util.TypedXmlSerializer; -import com.android.internal.util.FunctionalUtils; import com.android.server.pm.PackageManagerService; import com.android.server.pm.PackageSetting; import com.android.server.pm.Settings; @@ -49,7 +49,6 @@ import java.io.IOException; import java.util.List; import java.util.Set; import java.util.UUID; -import java.util.function.Consumer; import java.util.function.Function; public interface DomainVerificationManagerInternal { @@ -406,7 +405,8 @@ public interface DomainVerificationManagerInternal { @NonNull Set domains, int state) throws NameNotFoundException; - interface Connection extends DomainVerificationEnforcer.Callback { + interface Connection extends DomainVerificationEnforcer.Callback, + PackageSettingsSnapshotProvider { /** * Notify that a settings change has been made and that eventually @@ -430,60 +430,7 @@ public interface DomainVerificationManagerInternal { */ void schedule(int code, @Nullable Object object); - /** - * Run a function block that requires access to {@link PackageSetting} data. This will - * ensure the {@link PackageManagerService} is taken before - * {@link DomainVerificationManagerInternal}'s lock is taken to avoid deadlock. - */ - void withPackageSettings(@NonNull Consumer> block); - - /** - * Variant which returns a value to the caller. - * @see #withPackageSettings(Consumer) - */ - Output withPackageSettingsReturning( - @NonNull FunctionalUtils.ThrowingFunction, Output> - block); - - /** - * Variant which throws. - * @see #withPackageSettings(Consumer) - */ - void withPackageSettingsThrowing( - @NonNull ThrowingConsumer, ExceptionType> block) - throws ExceptionType; - - /** - * Variant which throws 2 exceptions. - * @see #withPackageSettings(Consumer) - */ - void - withPackageSettingsThrowing2( - @NonNull Throwing2Consumer, ExceptionOne, - ExceptionTwo> block) throws ExceptionOne, ExceptionTwo; - - /** - * Variant which returns a value to the caller and throws. - * @see #withPackageSettings(Consumer) - */ - Output withPackageSettingsReturningThrowing( - @NonNull ThrowingFunction, Output, ExceptionType> - block) throws ExceptionType; - @UserIdInt int[] getAllUserIds(); - - interface ThrowingConsumer { - void accept(Input input) throws ExceptionType; - } - - interface Throwing2Consumer { - void accept(Input input) throws ExceptionOne, ExceptionTwo; - } - - interface ThrowingFunction { - Output apply(Input input) throws ExceptionType; - } } } 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 f96eeb39e69e4..e2c837f28520a 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 @@ -260,7 +260,7 @@ public class DomainVerificationService extends SystemService public DomainVerificationInfo getDomainVerificationInfo(@NonNull String packageName) throws NameNotFoundException { mEnforcer.assertApprovedQuerent(mConnection.getCallingUid(), mProxy); - return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + return mConnection.withPackageSettingsSnapshotReturningThrowing(pkgSettings -> { synchronized (mLock) { PackageSetting pkgSetting = pkgSettings.apply(packageName); AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); @@ -320,7 +320,7 @@ public class DomainVerificationService extends SystemService @NonNull Set domains, int state) throws NameNotFoundException { mEnforcer.assertApprovedVerifier(callingUid, mProxy); - return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + return mConnection.withPackageSettingsSnapshotReturningThrowing(pkgSettings -> { synchronized (mLock) { List verifiedDomains = new ArrayList<>(); @@ -376,7 +376,7 @@ public class DomainVerificationService extends SystemService ArraySet verifiedDomains = new ArraySet<>(); if (packageName == null) { - mConnection.withPackageSettings(pkgSettings -> { + mConnection.withPackageSettingsSnapshot(pkgSettings -> { synchronized (mLock) { ArraySet validDomains = new ArraySet<>(); @@ -411,7 +411,7 @@ public class DomainVerificationService extends SystemService } }); } else { - mConnection.withPackageSettingsThrowing(pkgSettings -> { + mConnection.withPackageSettingsSnapshotThrowing(pkgSettings -> { synchronized (mLock) { DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); if (pkgState == null) { @@ -548,7 +548,7 @@ public class DomainVerificationService extends SystemService return DomainVerificationManager.ERROR_DOMAIN_SET_ID_INVALID; } - return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + return mConnection.withPackageSettingsSnapshotReturningThrowing(pkgSettings -> { synchronized (mLock) { GetAttachedResult result = getAndValidateAttachedLocked(domainSetId, domains, false /* forAutoVerify */, callingUid, userId, pkgSettings); @@ -588,7 +588,7 @@ public class DomainVerificationService extends SystemService @NonNull String packageName, boolean enabled, @Nullable ArraySet domains) throws NameNotFoundException { mEnforcer.assertInternal(mConnection.getCallingUid()); - mConnection.withPackageSettingsThrowing(pkgSettings -> { + mConnection.withPackageSettingsSnapshotThrowing(pkgSettings -> { synchronized (mLock) { DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); if (pkgState == null) { @@ -694,7 +694,7 @@ public class DomainVerificationService extends SystemService throw DomainVerificationUtils.throwPackageUnavailable(packageName); } - return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + return mConnection.withPackageSettingsSnapshotReturningThrowing(pkgSettings -> { synchronized (mLock) { PackageSetting pkgSetting = pkgSettings.apply(packageName); AndroidPackage pkg = pkgSetting == null ? null : pkgSetting.getPkg(); @@ -747,7 +747,7 @@ public class DomainVerificationService extends SystemService mEnforcer.assertOwnerQuerent(mConnection.getCallingUid(), mConnection.getCallingUserId(), userId); - return mConnection.withPackageSettingsReturningThrowing(pkgSettings -> { + return mConnection.withPackageSettingsSnapshotReturningThrowing(pkgSettings -> { SparseArray> levelToPackages = getOwnersForDomainInternal(domain, false, userId, pkgSettings); if (levelToPackages.size() == 0) { @@ -1048,7 +1048,7 @@ public class DomainVerificationService extends SystemService public void writeSettings(@NonNull TypedXmlSerializer serializer, boolean includeSignatures, @UserIdInt int userId) throws IOException { - mConnection.withPackageSettingsThrowing(pkgSettings -> { + mConnection.withPackageSettingsSnapshotThrowing(pkgSettings -> { synchronized (mLock) { Function pkgNameToSignature = null; if (includeSignatures) { @@ -1077,7 +1077,7 @@ public class DomainVerificationService extends SystemService @Override public void readSettings(@NonNull TypedXmlPullParser parser) throws IOException, XmlPullParserException { - mConnection.withPackageSettingsThrowing2( + mConnection.withPackageSettingsSnapshotThrowing2( pkgSettings -> { synchronized (mLock) { mSettings.readSettings(parser, mAttachedPkgStates, pkgSettings); @@ -1094,7 +1094,7 @@ public class DomainVerificationService extends SystemService @Override public void restoreSettings(@NonNull TypedXmlPullParser parser) throws IOException, XmlPullParserException { - mConnection.withPackageSettingsThrowing2( + mConnection.withPackageSettingsSnapshotThrowing2( pkgSettings -> { synchronized (mLock) { mSettings.restoreSettings(parser, mAttachedPkgStates, pkgSettings); @@ -1175,7 +1175,7 @@ public class DomainVerificationService extends SystemService @Override public void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, @Nullable Integer userId) throws NameNotFoundException { - mConnection.withPackageSettingsThrowing( + mConnection.withPackageSettingsSnapshotThrowing( pkgSettings -> printState(writer, packageName, userId, pkgSettings)); } @@ -1193,7 +1193,7 @@ public class DomainVerificationService extends SystemService public void printOwnersForPackage(@NonNull IndentingPrintWriter writer, @Nullable String packageName, @Nullable @UserIdInt Integer userId) throws NameNotFoundException { - mConnection.withPackageSettingsThrowing(pkgSettings -> { + mConnection.withPackageSettingsSnapshotThrowing(pkgSettings -> { synchronized (mLock) { if (packageName == null) { int size = mAttachedPkgStates.size(); @@ -1242,7 +1242,7 @@ public class DomainVerificationService extends SystemService @Override public void printOwnersForDomains(@NonNull IndentingPrintWriter writer, @NonNull List domains, @Nullable @UserIdInt Integer userId) { - mConnection.withPackageSettings(pkgSettings -> { + mConnection.withPackageSettingsSnapshot(pkgSettings -> { synchronized (mLock) { int size = domains.size(); for (int index = 0; index < size; index++) { @@ -1421,7 +1421,7 @@ public class DomainVerificationService extends SystemService @Override public void clearDomainVerificationState(@Nullable List packageNames) { mEnforcer.assertInternal(mConnection.getCallingUid()); - mConnection.withPackageSettings(pkgSettings -> { + mConnection.withPackageSettingsSnapshot(pkgSettings -> { synchronized (mLock) { if (packageNames == null) { int size = mAttachedPkgStates.size(); @@ -2022,43 +2022,46 @@ public class DomainVerificationService extends SystemService } @Override - public void withPackageSettings( + public void withPackageSettingsSnapshot( @NonNull Consumer> block) { enforceLocking(); - mConnection.withPackageSettings(block); + mConnection.withPackageSettingsSnapshot(block); } @Override - public Output withPackageSettingsReturning( + public Output withPackageSettingsSnapshotReturning( @NonNull FunctionalUtils.ThrowingFunction, Output> block) { enforceLocking(); - return mConnection.withPackageSettingsReturning(block); + return mConnection.withPackageSettingsSnapshotReturning(block); } @Override - public void withPackageSettingsThrowing( - @NonNull ThrowingConsumer, ExceptionType> block) - throws ExceptionType { + public void withPackageSettingsSnapshotThrowing( + @NonNull FunctionalUtils.ThrowingCheckedConsumer, + ExceptionType> block) throws ExceptionType { enforceLocking(); - mConnection.withPackageSettingsThrowing(block); + mConnection.withPackageSettingsSnapshotThrowing(block); } @Override public void - withPackageSettingsThrowing2( - @NonNull Throwing2Consumer, ExceptionOne, - ExceptionTwo> block) throws ExceptionOne, ExceptionTwo { + withPackageSettingsSnapshotThrowing2( + @NonNull FunctionalUtils.ThrowingChecked2Consumer< + Function, ExceptionOne, ExceptionTwo> block) + throws ExceptionOne, ExceptionTwo { enforceLocking(); - mConnection.withPackageSettingsThrowing2(block); + mConnection.withPackageSettingsSnapshotThrowing2(block); } @Override public Output - withPackageSettingsReturningThrowing(@NonNull ThrowingFunction, Output, ExceptionType> block) throws ExceptionType { + withPackageSettingsSnapshotReturningThrowing( + @NonNull FunctionalUtils.ThrowingCheckedFunction< + Function, Output, ExceptionType> block) + throws ExceptionType { enforceLocking(); - return mConnection.withPackageSettingsReturningThrowing(block); + return mConnection.withPackageSettingsSnapshotReturningThrowing(block); } @Override diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt index 00986f741eea4..48845be693feb 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationTestUtils.kt @@ -30,25 +30,25 @@ internal object DomainVerificationTestUtils { fun DomainVerificationManagerInternal.Connection.mockPackageSettings( block: (String) -> PackageSetting? ) { - whenever(withPackageSettings(any())) { + whenever(withPackageSettingsSnapshot(any())) { (arguments[0] as Consumer>).accept { block(it) } } - whenever(withPackageSettingsReturning(any())) { + whenever(withPackageSettingsSnapshotReturning(any())) { (arguments[0] as FunctionalUtils.ThrowingFunction, *>) .apply { block(it) } } - whenever(withPackageSettingsThrowing(any())) { - (arguments[0] as DomainVerificationManagerInternal.Connection.ThrowingConsumer< + whenever(withPackageSettingsSnapshotThrowing(any())) { + (arguments[0] as FunctionalUtils.ThrowingCheckedConsumer< Function, *>) .accept { block(it) } } - whenever(withPackageSettingsThrowing2(any())) { - (arguments[0] as DomainVerificationManagerInternal.Connection.Throwing2Consumer< + whenever(withPackageSettingsSnapshotThrowing2(any())) { + (arguments[0] as FunctionalUtils.ThrowingChecked2Consumer< Function, *, *>) .accept { block(it) } } - whenever(withPackageSettingsReturningThrowing(any())) { - (arguments[0] as DomainVerificationManagerInternal.Connection.ThrowingFunction< + whenever(withPackageSettingsSnapshotReturningThrowing(any())) { + (arguments[0] as FunctionalUtils.ThrowingCheckedFunction< Function, *, *>) .apply { block(it) } } From a83c8635e82987001c66a172464a92df9ba99e3f Mon Sep 17 00:00:00 2001 From: Winson Date: Mon, 5 Apr 2021 12:52:47 -0700 Subject: [PATCH 2/2] Strip invalid domains in DVS#addPackage If a package is ever updated as part of an OTA, it have may removed domains that were included in the original set, and so that state should be evicted when it's re-attached. Bug: 184562304 Test: atest DomainVerificationPackageTest Change-Id: Ie90dc390afa89b0c26f73de64b1e513c6fa272a3 --- .../domain/DomainVerificationService.java | 21 ++- .../DomainVerificationInternalUserState.java | 5 + .../domain/DomainVerificationPackageTest.kt | 126 ++++++++++++++++-- 3 files changed, 134 insertions(+), 18 deletions(-) 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 e2c837f28520a..b1b4e2afeb497 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 @@ -956,17 +956,25 @@ public class DomainVerificationService extends SystemService } AndroidPackage pkg = newPkgSetting.getPkg(); - ArraySet domains = mCollector.collectValidAutoVerifyDomains(pkg); - boolean hasAutoVerifyDomains = !domains.isEmpty(); + ArraySet autoVerifyDomains = mCollector.collectValidAutoVerifyDomains(pkg); + boolean hasAutoVerifyDomains = !autoVerifyDomains.isEmpty(); boolean isPendingOrRestored = pkgState != null; if (isPendingOrRestored) { pkgState = new DomainVerificationPkgState(pkgState, domainSetId, hasAutoVerifyDomains); + pkgState.getStateMap().retainAll(autoVerifyDomains); + + Set webDomains = mCollector.collectAllWebDomains(pkg); + SparseArray userStates = pkgState.getUserStates(); + int size = userStates.size(); + for (int index = 0; index < size; index++) { + userStates.valueAt(index).retainHosts(webDomains); + } } else { pkgState = new DomainVerificationPkgState(pkgName, domainSetId, hasAutoVerifyDomains); } - boolean needsBroadcast = - applyImmutableState(newPkgSetting, pkgState.getStateMap(), domains); + boolean needsBroadcast = applyImmutableState(newPkgSetting, pkgState.getStateMap(), + autoVerifyDomains); if (needsBroadcast && !isPendingOrRestored) { // TODO(b/159952358): Test this behavior // Attempt to preserve user experience by automatically verifying all domains from @@ -997,9 +1005,10 @@ public class DomainVerificationService extends SystemService && legacyInfo.getStatus() == PackageManager.INTENT_FILTER_DOMAIN_VERIFICATION_STATUS_ALWAYS) { ArrayMap stateMap = pkgState.getStateMap(); - int domainsSize = domains.size(); + int domainsSize = autoVerifyDomains.size(); for (int index = 0; index < domainsSize; index++) { - stateMap.put(domains.valueAt(index), DomainVerificationState.STATE_MIGRATED); + stateMap.put(autoVerifyDomains.valueAt(index), + DomainVerificationState.STATE_MIGRATED); } } } diff --git a/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationInternalUserState.java b/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationInternalUserState.java index aa7407ce3fe84..41de3fc06d47f 100644 --- a/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationInternalUserState.java +++ b/services/core/java/com/android/server/pm/verify/domain/models/DomainVerificationInternalUserState.java @@ -73,6 +73,11 @@ public class DomainVerificationInternalUserState { return this; } + public DomainVerificationInternalUserState retainHosts(@NonNull Set hosts) { + mEnabledHosts.retainAll(hosts); + return this; + } + // Code below generated by codegen v1.0.22. diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt index 83c126842213b..6c2a8916617b2 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationPackageTest.kt @@ -22,6 +22,7 @@ import android.content.pm.PackageUserState import android.content.pm.Signature import android.content.pm.parsing.component.ParsedActivity import android.content.pm.parsing.component.ParsedIntentInfo +import android.content.pm.verify.domain.DomainOwner import android.content.pm.verify.domain.DomainVerificationInfo.STATE_MODIFIABLE_VERIFIED import android.content.pm.verify.domain.DomainVerificationInfo.STATE_NO_RESPONSE import android.content.pm.verify.domain.DomainVerificationInfo.STATE_SUCCESS @@ -297,11 +298,95 @@ class DomainVerificationPackageTest { service.addPackage(pkg1) + assertAddPackageActivePendingRestoredState(service) + } + + @Test + fun addPackagePendingStripInvalidDomains() { + val xml = addPackagePendingOrRestoredWithInvalidDomains() + val service = makeService(pkg1, pkg2) + xml.byteInputStream().use { + service.readSettings(Xml.resolvePullParser(it)) + } + + service.addPackage(pkg1) + + val userState = service.getUserState(pkg1.getName()) + assertThat(userState.packageName).isEqualTo(pkg1.getName()) + assertThat(userState.identifier).isEqualTo(pkg1.domainSetId) + assertThat(userState.isLinkHandlingAllowed).isEqualTo(false) + assertThat(userState.user.identifier).isEqualTo(USER_ID) + assertThat(userState.hostToStateMap).containsExactlyEntriesIn(mapOf( + DOMAIN_1 to DOMAIN_STATE_VERIFIED, + DOMAIN_2 to DOMAIN_STATE_SELECTED, + )) + + assertAddPackageActivePendingRestoredState(service) + } + + @Test + fun addPackageRestoredStripInvalidDomains() { + val xml = addPackagePendingOrRestoredWithInvalidDomains() + val service = makeService(pkg1, pkg2) + xml.byteInputStream().use { + service.restoreSettings(Xml.resolvePullParser(it)) + } + + service.addPackage(pkg1) + + assertAddPackageActivePendingRestoredState(service, expectRestore = true) + } + + /** + * Shared string that contains invalid [DOMAIN_3] and [DOMAIN_4] which should be stripped from + * the final state. + */ + private fun addPackagePendingOrRestoredWithInvalidDomains(): String = + // language=XML + """ + + + + + + + + + + + + + + + + + + + + + + + + + """.trimIndent() + + /** + * Shared method to assert the same output when testing adding pkg1. + */ + private fun assertAddPackageActivePendingRestoredState( + service: DomainVerificationService, + expectRestore: Boolean = false + ) { val info = service.getInfo(pkg1.getName()) assertThat(info.packageName).isEqualTo(pkg1.getName()) assertThat(info.identifier).isEqualTo(pkg1.domainSetId) assertThat(info.hostToStateMap).containsExactlyEntriesIn(mapOf( - DOMAIN_1 to STATE_SUCCESS, + // To share the majority of code, special case restoration to check a different int + DOMAIN_1 to if (expectRestore) STATE_MODIFIABLE_VERIFIED else STATE_SUCCESS, DOMAIN_2 to STATE_NO_RESPONSE, )) @@ -317,6 +402,23 @@ class DomainVerificationPackageTest { assertThat(service.queryValidVerificationPackageNames()) .containsExactly(pkg1.getName()) + + // Re-enable link handling to check that the 3/4 domains were stripped + service.setDomainVerificationLinkHandlingAllowed(pkg1.getName(), true, USER_ID) + + assertThat(service.getOwnersForDomain(DOMAIN_1, USER_ID)) + .containsExactly(DomainOwner(PKG_ONE, false)) + + assertThat(service.getOwnersForDomain(DOMAIN_2, USER_ID)) + .containsExactly(DomainOwner(PKG_ONE, true)) + + assertThat(service.getOwnersForDomain(DOMAIN_2, USER_ID + 10)).isEmpty() + + listOf(DOMAIN_3, DOMAIN_4).forEach { domain -> + listOf(USER_ID, USER_ID + 10).forEach { userId -> + assertThat(service.getOwnersForDomain(domain, userId)).isEmpty() + } + } } @Test @@ -528,9 +630,9 @@ class DomainVerificationPackageTest { serviceBefore.addPackage(pkg2) serviceBefore.setStatus(pkg1.domainSetId, setOf(DOMAIN_1), STATE_SUCCESS) - serviceBefore.setDomainVerificationLinkHandlingAllowed(pkg1.getName(), false, 1) + serviceBefore.setDomainVerificationLinkHandlingAllowed(pkg1.getName(), false, 10) serviceBefore.setUserSelection(pkg2.domainSetId, setOf(DOMAIN_2), true, 0) - serviceBefore.setUserSelection(pkg2.domainSetId, setOf(DOMAIN_3), true, 1) + serviceBefore.setUserSelection(pkg2.domainSetId, setOf(DOMAIN_3), true, 10) fun assertExpectedState(service: DomainVerificationService) { service.assertState( @@ -541,7 +643,7 @@ class DomainVerificationPackageTest { ) service.assertState( - pkg1, userId = 1, linkHandingAllowed = false, hostToStateMap = mapOf( + pkg1, userId = 10, linkHandingAllowed = false, hostToStateMap = mapOf( DOMAIN_1 to DOMAIN_STATE_VERIFIED, DOMAIN_2 to DOMAIN_STATE_NONE, ) @@ -556,7 +658,7 @@ class DomainVerificationPackageTest { ) service.assertState( - pkg2, userId = 1, hostToStateMap = mapOf( + pkg2, userId = 10, hostToStateMap = mapOf( DOMAIN_1 to DOMAIN_STATE_NONE, DOMAIN_2 to DOMAIN_STATE_NONE, DOMAIN_3 to DOMAIN_STATE_SELECTED, @@ -572,7 +674,7 @@ class DomainVerificationPackageTest { } val backupUser1 = ByteArrayOutputStream().use { - serviceBefore.writeSettings(Xml.resolveSerializer(it), true, 1) + serviceBefore.writeSettings(Xml.resolveSerializer(it), true, 10) it.toByteArray() } @@ -581,7 +683,7 @@ class DomainVerificationPackageTest { serviceAfter.addPackage(pkg2) // Check the state is default before the restoration applies - listOf(0, 1).forEach { + listOf(0, 10).forEach { serviceAfter.assertState( pkg1, userId = it, hostToStateMap = mapOf( DOMAIN_1 to DOMAIN_STATE_NONE, @@ -590,7 +692,7 @@ class DomainVerificationPackageTest { ) } - listOf(0, 1).forEach { + listOf(0, 10).forEach { serviceAfter.assertState( pkg2, userId = it, hostToStateMap = mapOf( DOMAIN_1 to DOMAIN_STATE_NONE, @@ -606,14 +708,14 @@ class DomainVerificationPackageTest { // Assert user 1 was restored serviceAfter.assertState( - pkg1, userId = 1, linkHandingAllowed = false, hostToStateMap = mapOf( + pkg1, userId = 10, linkHandingAllowed = false, hostToStateMap = mapOf( DOMAIN_1 to DOMAIN_STATE_VERIFIED, DOMAIN_2 to DOMAIN_STATE_NONE, ) ) serviceAfter.assertState( - pkg2, userId = 1, hostToStateMap = mapOf( + pkg2, userId = 10, hostToStateMap = mapOf( DOMAIN_1 to DOMAIN_STATE_NONE, DOMAIN_2 to DOMAIN_STATE_NONE, DOMAIN_3 to DOMAIN_STATE_SELECTED, @@ -679,7 +781,7 @@ class DomainVerificationPackageTest { setConnection(mockThrowOnUnmocked { whenever(filterAppAccess(anyString(), anyInt(), anyInt())) { false } whenever(doesUserExist(0)) { true } - whenever(doesUserExist(1)) { true } + whenever(doesUserExist(10)) { true } whenever(scheduleWriteSettings()) // Need to provide an internal UID so some permission checks are ignored @@ -732,7 +834,7 @@ class DomainVerificationPackageTest { whenever(getInstantApp(anyInt())) { false } whenever(firstInstallTime) { 0L } whenever(readUserState(0)) { PackageUserState() } - whenever(readUserState(1)) { PackageUserState() } + whenever(readUserState(10)) { PackageUserState() } whenever(signatures) { arrayOf(Signature(signature)) } whenever(isSystem) { isSystemApp } }