Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 897a2256 authored by Winson's avatar Winson
Browse files

Fix DomainVerificationService deadlock 

Enforces the PackageManagerService lock to be taken before the DVS lock,
when neccessary, so that the locking is always one-way and cannot
deadlock when Settings attempts to serialize state.

Bug: 183643808

Test: manual run CTS which previously reproduced this issue

Change-Id: Ibccde458df16238755f3a67080321cb3ebdf7233
parent 5f5d4f60

services/core/java/com/android/server/pm/PackageManagerService.java

+79 −13
Original line number Diff line number Diff line
@@ -356,6 +356,7 @@ import com.android.internal.util.CollectionUtils; 
import com.android.internal.util.ConcurrentUtils;

import com.android.internal.util.DumpUtils;

import com.android.internal.util.FrameworkStatsLog;

import com.android.internal.util.FunctionalUtils;

import com.android.internal.util.IndentingPrintWriter;

import com.android.internal.util.Preconditions;

import com.android.permission.persistence.RuntimePermissionsPersistence;
@@ -1686,9 +1687,8 @@ public class PackageManagerService extends IPackageManager.Stub 
    private final DomainVerificationConnection mDomainVerificationConnection =

            new DomainVerificationConnection();













    private class DomainVerificationConnection implements













            DomainVerificationService.Connection, DomainVerificationProxyV1.Connection,













            DomainVerificationProxyV2.Connection {













    private class DomainVerificationConnection implements DomainVerificationService.Connection,













            DomainVerificationProxyV1.Connection, DomainVerificationProxyV2.Connection {





























        @Override















        public void scheduleWriteSettings() {










@@ -1734,20 +1734,75 @@ public class PackageManagerService extends IPackageManager.Stub



























        @Nullable















        @Override













        public PackageSetting getPackageSettingLocked(@NonNull String pkgName) {













            return PackageManagerService.this.getPackageSetting(pkgName);













        public AndroidPackage getPackage(@NonNull String packageName) {













            return PackageManagerService.this.getPackage(packageName);















        }



























        @Nullable













        @NonNull















        @Override













        public AndroidPackage getPackageLocked(@NonNull String pkgName) {













            return PackageManagerService.this.getPackage(pkgName);













        public void withPackageSettings(@NonNull Consumer<Function<String, PackageSetting>> 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);













            }















        }



























        @Nullable















        @Override













        public AndroidPackage getPackage(@NonNull String packageName) {













            return getPackageLocked(packageName);













        public <Output> Output withPackageSettingsReturning(













                @NonNull FunctionalUtils.ThrowingFunction<Function<String, PackageSetting>, 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 <ExceptionType extends Exception> void withPackageSettingsThrowing(













                @NonNull ThrowingConsumer<Function<String, PackageSetting>, 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 <Output, ExceptionType extends Exception> Output













                withPackageSettingsReturningThrowing(@NonNull ThrowingFunction<Function<String,













                PackageSetting>, 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










@@ -2012,7 +2067,7 @@ public class PackageManagerService extends IPackageManager.Stub



























        // Cached attributes.  The names in this class are the same as the















        // names in PackageManagerService; see that class for documentation.













        private final Settings mSettings;













        protected final Settings mSettings;















        private final WatchedSparseIntArray mIsolatedOwners;















        private final WatchedArrayMap<String, AndroidPackage> mPackages;















        private final WatchedArrayMap<ComponentName, ParsedInstrumentation>










@@ -4669,6 +4724,17 @@ public class PackageManagerService extends IPackageManager.Stub













            mLock = mService.mLock;















        }



























        /**













         * Explicilty snapshot {@link Settings#mPackages} for cases where the caller must not lock













         * in order to get package data. It is expected that the caller locks itself to be able













         * to block on changes to the package data and bring itself up to date once the change













         * propagates to it. Use with heavy caution.













         * @return













         */













        private Map<String, PackageSetting> snapshotPackageSettings() {













            return mSettings.snapshot().mPackages;













        }



























        public @NonNull List<ResolveInfo> queryIntentServicesInternalBody(Intent intent,















                String resolvedType, int flags, int userId, int callingUid,















                String instantAppPkgName) {










@@ -4800,7 +4866,7 @@ public class PackageManagerService extends IPackageManager.Stub



























    // Compute read-only functions, based on live data.  This attribute may be modified multiple















    // times during the PackageManagerService constructor but it should not be modified thereafter.













    private Computer mLiveComputer;













    private ComputerLocked mLiveComputer;















    // A lock-free cache for frequently called functions.















    private volatile Computer mSnapshotComputer;















    // If true, the snapshot is invalid (stale).  The attribute is static since it may be

























services/core/java/com/android/server/pm/verify/domain/DomainVerificationManagerInternal.java



+51
−20




























Original line number
Diff line number
Diff line








@@ -28,6 +28,7 @@ import android.content.pm.PackageManager.NameNotFoundException;













import android.content.pm.ResolveInfo;















import android.content.pm.verify.domain.DomainVerificationInfo;















import android.content.pm.verify.domain.DomainVerificationManager;













import android.content.pm.verify.domain.DomainVerificationState;















import android.os.Binder;















import android.os.UserHandle;















import android.util.IndentingPrintWriter;









@@ -35,8 +36,10 @@ 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.parsing.pkg.AndroidPackage;













import com.android.server.pm.Settings;















import com.android.server.pm.verify.domain.models.DomainVerificationPkgState;















import com.android.server.pm.verify.domain.proxy.DomainVerificationProxy;

























@@ -46,6 +49,7 @@ 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 {










@@ -108,9 +112,10 @@ public interface DomainVerificationManagerInternal {













    int APPROVAL_LEVEL_INSTANT_APP = 5;































    /**













     * Defines the possible values for {@link #approvalLevelForDomain(PackageSetting, Intent, int)}













     * which sorts packages by approval priority. A higher numerical value means the package should













     * override all lower values. This means that comparison using less/greater than IS valid.













     * Defines the possible values for













     * {@link #approvalLevelForDomain(PackageSetting, Intent, List, int, int)} which sorts packages













     * by approval priority. A higher numerical value means the package should override all lower













     * values. This means that comparison using less/greater than IS valid.















     *















     * Negative values are possible, although not implemented, reserved if explicit disable of a















     * package for a domain needs to be tracked.










@@ -184,7 +189,7 @@ public interface DomainVerificationManagerInternal {













    /**















     * Migrates verification state from a previous install to a new one. It is expected that the















     * {@link PackageSetting#getDomainSetId()} already be set to the correct value, usually from













     * {@link #generateNewId()}. This will preserve {@link DomainVerificationManager#STATE_SUCCESS}













     * {@link #generateNewId()}. This will preserve {@link DomainVerificationState#STATE_SUCCESS}















     * domains under the assumption that the new package will pass the same server side config as















     * the previous package, as they have matching signatures.















     * <p>










@@ -276,7 +281,7 @@ public interface DomainVerificationManagerInternal {





























    /**















     * Until the legacy APIs are entirely removed, returns the legacy state from the previously













     * written info stored in {@link com.android.server.pm.Settings}.













     * written info stored in {@link Settings}.















     */















    int getLegacyState(@NonNull String packageName, @UserIdInt int userId);

























@@ -288,15 +293,12 @@ public interface DomainVerificationManagerInternal {













     * @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)}













     *                           from {@link PackageManagerService}, it is













     *                           expected to pass in the snapshot of {@link PackageSetting} objects















     */















    void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName,















            @Nullable @UserIdInt Integer userId,













            @Nullable Function<String, PackageSetting> pkgSettingFunction)













            @NonNull Function<String, PackageSetting> pkgSettingFunction)















            throws NameNotFoundException;































    @NonNull










@@ -368,17 +370,46 @@ public interface DomainVerificationManagerInternal {













         */















        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);













        /**













         * 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<Function<String, PackageSetting>> block);





























        @Nullable













        AndroidPackage getPackageLocked(@NonNull String pkgName);













        /**













         * Variant which returns a value to the caller.













         * @see #withPackageSettings(Consumer)













         */













        <Output> Output withPackageSettingsReturning(













                @NonNull FunctionalUtils.ThrowingFunction<Function<String, PackageSetting>, Output>













                        block);



























        /**













         * Variant which throws.













         * @see #withPackageSettings(Consumer)













         */













        <ExceptionType extends Exception> void withPackageSettingsThrowing(













                @NonNull ThrowingConsumer<Function<String, PackageSetting>, ExceptionType> block)













                throws ExceptionType;



























        /**













         * Variant which returns a value to the caller and throws.













         * @see #withPackageSettings(Consumer)













         */













        <Output, ExceptionType extends Exception> Output withPackageSettingsReturningThrowing(













                @NonNull ThrowingFunction<Function<String, PackageSetting>, Output, ExceptionType>













                        block) throws ExceptionType;































        @UserIdInt















        int[] getAllUserIds();



























        interface ThrowingConsumer<Input, ExceptionType extends Exception> {













            void accept(Input input) throws ExceptionType;













        }



























        interface ThrowingFunction<Input, Output, ExceptionType extends Exception> {













            Output apply(Input input) throws ExceptionType;













        }















    }















}
























services/core/java/com/android/server/pm/verify/domain/DomainVerificationService.java



+407
−283


























File changed.
Preview size limit exceeded, changes collapsed.
























services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationEnforcerTest.kt



+8
−4




























Original line number
Diff line number
Diff line








@@ -31,6 +31,7 @@ import android.util.SparseArray













import androidx.test.platform.app.InstrumentationRegistry















import com.android.server.pm.PackageSetting















import com.android.server.pm.parsing.pkg.AndroidPackage













import com.android.server.pm.test.verify.domain.DomainVerificationTestUtils.mockPackageSettings















import com.android.server.pm.verify.domain.DomainVerificationEnforcer















import com.android.server.pm.verify.domain.DomainVerificationManagerInternal















import com.android.server.pm.verify.domain.DomainVerificationService










@@ -98,10 +99,13 @@ class DomainVerificationEnforcerTest {













                    mockThrowOnUnmocked {















                        whenever(callingUid) { callingUidInt.get() }















                        whenever(callingUserId) { callingUserIdInt.get() }













                        whenever(getPackageSettingLocked(VISIBLE_PKG)) { visiblePkgSetting }













                        whenever(getPackageLocked(VISIBLE_PKG)) { visiblePkg }













                        whenever(getPackageSettingLocked(INVISIBLE_PKG)) { invisiblePkgSetting }













                        whenever(getPackageLocked(INVISIBLE_PKG)) { invisiblePkg }













                        mockPackageSettings {













                            when (it) {













                                VISIBLE_PKG -> visiblePkgSetting













                                INVISIBLE_PKG -> invisiblePkgSetting













                                else -> null













                            }













                        }















                        whenever(schedule(anyInt(), any()))















                        whenever(scheduleWriteSettings())















                        whenever(filterAppAccess(eq(VISIBLE_PKG), anyInt(), anyInt())) { false }

































services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/verify/domain/DomainVerificationManagerApiTest.kt



+3
−5




























Original line number
Diff line number
Diff line








@@ -32,6 +32,7 @@ import android.os.Process













import android.util.ArraySet















import com.android.server.pm.PackageSetting















import com.android.server.pm.parsing.pkg.AndroidPackage













import com.android.server.pm.test.verify.domain.DomainVerificationTestUtils.mockPackageSettings















import com.android.server.pm.verify.domain.DomainVerificationService















import com.android.server.testutils.mockThrowOnUnmocked















import com.android.server.testutils.whenever










@@ -445,11 +446,8 @@ class DomainVerificationManagerApiTest {













                    whenever(callingUid) { Process.ROOT_UID }















                    whenever(callingUserId) { 0 }





























                    whenever(getPackageSettingLocked(anyString())) {













                        pkgSettingFunction(arguments[0] as String)













                    }













                    whenever(getPackageLocked(anyString())) {













                        pkgSettingFunction(arguments[0] as String)?.getPkg()













                    mockPackageSettings {













                        pkgSettingFunction(it)















                    }















                })















            }