From 47dbabfc14420ff54646df05039657df43901554 Mon Sep 17 00:00:00 2001 From: Winson Date: Wed, 20 Jan 2021 12:26:37 -0800 Subject: [PATCH] Convert DomainVerificationService.Connection to synchronous set Unfortunately it's possible to call this during PackageManagerService initialization, so this needs to be attached before PMS gets created. Rather than using a deferred injection, this makes PMS call setConnection to directly set the callback. A better isolated might be worth exploring in the future, but overall it shouldn't really matter. Exempt-From-Owner-Approval: Already approved by owners on main branch Bug: 163565712 Test: com.android.server.pm.test.domain.verify Change-Id: Ib60f5560ecaa510474228aa1c38eff5d6041ab58 --- .../server/pm/PackageManagerService.java | 4 +- .../DomainVerificationManagerInternal.java | 38 +++++ .../verify/DomainVerificationService.java | 130 ++++++------------ .../java/com/android/server/SystemServer.java | 12 +- .../verify/DomainVerificationEnforcerTest.kt | 23 ++-- 5 files changed, 98 insertions(+), 109 deletions(-) diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java index cbc66ac644364..f3cc7cf22387c 100644 --- a/services/core/java/com/android/server/pm/PackageManagerService.java +++ b/services/core/java/com/android/server/pm/PackageManagerService.java @@ -6456,6 +6456,9 @@ public class PackageManagerService extends IPackageManager.Stub mAppInstallDir = new File(Environment.getDataDirectory(), "app"); mAppLib32InstallDir = getAppLib32InstallDir(); + mDomainVerificationManager = injector.getDomainVerificationManagerInternal(); + mDomainVerificationManager.setConnection(mDomainVerificationConnection); + // Link up the watchers mPackages.registerObserver(mWatcher); mSharedLibraries.registerObserver(mWatcher); @@ -6482,7 +6485,6 @@ public class PackageManagerService extends IPackageManager.Stub mProcessLoggingHandler = new ProcessLoggingHandler(); Watchdog.getInstance().addThread(mHandler, WATCHDOG_TIMEOUT); mIntentFilterVerificationManager = injector.getIntentFilterVerificationManager(); - mDomainVerificationManager = injector.getDomainVerificationManagerInternal(); ArrayMap libConfig = systemConfig.getSharedLibraries(); diff --git a/services/core/java/com/android/server/pm/domain/verify/DomainVerificationManagerInternal.java b/services/core/java/com/android/server/pm/domain/verify/DomainVerificationManagerInternal.java index 56994c864870b..f123aa22a0a75 100644 --- a/services/core/java/com/android/server/pm/domain/verify/DomainVerificationManagerInternal.java +++ b/services/core/java/com/android/server/pm/domain/verify/DomainVerificationManagerInternal.java @@ -25,10 +25,14 @@ import android.content.pm.IntentFilterVerificationInfo; import android.content.pm.PackageManager.NameNotFoundException; import android.content.pm.domain.verify.DomainVerificationManager; import android.content.pm.domain.verify.DomainVerificationSet; +import android.os.Binder; +import android.os.UserHandle; import android.util.IndentingPrintWriter; import android.util.TypedXmlPullParser; import android.util.TypedXmlSerializer; +import com.android.internal.annotations.GuardedBy; +import com.android.server.pm.PackageManagerService; import com.android.server.pm.PackageSetting; import com.android.server.pm.domain.verify.models.DomainVerificationPkgState; import com.android.server.pm.domain.verify.proxy.DomainVerificationProxy; @@ -50,6 +54,8 @@ public interface DomainVerificationManagerInternal extends DomainVerificationMan @NonNull UUID generateNewId(); + void setConnection(@NonNull Connection connection); + @NonNull DomainVerificationProxy getProxy(); @@ -219,4 +225,36 @@ public interface DomainVerificationManagerInternal extends DomainVerificationMan void setDomainVerificationStatusInternal(int callingUid, @NonNull UUID domainSetId, @NonNull Set domains, int state) throws InvalidDomainSetException, NameNotFoundException; + + + interface Connection { + + /** + * Notify that a settings change has been made and that eventually + * {@link #writeSettings(TypedXmlSerializer)} should be invoked by the parent. + */ + void scheduleWriteSettings(); + + /** + * Delegate to {@link Binder#getCallingUid()} to allow mocking in tests. + */ + int getCallingUid(); + + /** + * Delegate to {@link UserHandle#getCallingUserId()} to allow mocking in tests. + */ + @UserIdInt + int getCallingUserId(); + + /** + * @see DomainVerificationProxy.BaseConnection#schedule(int, java.lang.Object) + */ + void schedule(int code, @Nullable Object object); + + @Nullable + PackageSetting getPackageSettingLocked(@NonNull String pkgName); + + @Nullable + AndroidPackage getPackageLocked(@NonNull String pkgName); + } } diff --git a/services/core/java/com/android/server/pm/domain/verify/DomainVerificationService.java b/services/core/java/com/android/server/pm/domain/verify/DomainVerificationService.java index ace72090193aa..d316773031b1b 100644 --- a/services/core/java/com/android/server/pm/domain/verify/DomainVerificationService.java +++ b/services/core/java/com/android/server/pm/domain/verify/DomainVerificationService.java @@ -110,7 +110,7 @@ public class DomainVerificationService extends SystemService private final Object mLock = new Object(); @NonNull - private final Singleton mConnection; + private Connection mConnection; @NonNull private final SystemConfig mSystemConfig; @@ -143,9 +143,8 @@ public class DomainVerificationService extends SystemService private DomainVerificationProxy mProxy = new DomainVerificationProxyUnavailable(); public DomainVerificationService(@NonNull Context context, @NonNull SystemConfig systemConfig, - @NonNull PlatformCompat platformCompat, @NonNull Singleton connection) { + @NonNull PlatformCompat platformCompat) { super(context); - mConnection = connection; mSystemConfig = systemConfig; mPlatformCompat = platformCompat; mSettings = new DomainVerificationSettings(); @@ -161,6 +160,11 @@ public class DomainVerificationService extends SystemService publishBinderService(Context.DOMAIN_VERIFICATION_SERVICE, mStub); } + @Override + public void setConnection(@NonNull Connection connection) { + mConnection = connection; + } + @NonNull @Override public DomainVerificationProxy getProxy() { @@ -185,7 +189,7 @@ public class DomainVerificationService extends SystemService @NonNull @Override public List getValidVerificationPackageNames() { - mEnforcer.assertApprovedVerifier(mConnection.get().getCallingUid(), mProxy); + mEnforcer.assertApprovedVerifier(mConnection.getCallingUid(), mProxy); List packageNames = new ArrayList<>(); synchronized (mLock) { int size = mAttachedPkgStates.size(); @@ -216,14 +220,14 @@ public class DomainVerificationService extends SystemService @Override public DomainVerificationSet getDomainVerificationSet(@NonNull String packageName) throws NameNotFoundException { - mEnforcer.assertApprovedQuerent(mConnection.get().getCallingUid(), mProxy); + mEnforcer.assertApprovedQuerent(mConnection.getCallingUid(), mProxy); synchronized (mLock) { DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); if (pkgState == null) { return null; } - AndroidPackage pkg = mConnection.get().getPackageLocked(packageName); + AndroidPackage pkg = mConnection.getPackageLocked(packageName); if (pkg == null) { throw DomainVerificationUtils.throwPackageUnavailable(packageName); } @@ -258,7 +262,7 @@ public class DomainVerificationService extends SystemService } } - setDomainVerificationStatusInternal(mConnection.get().getCallingUid(), domainSetId, domains, + setDomainVerificationStatusInternal(mConnection.getCallingUid(), domainSetId, domains, state); } @@ -282,13 +286,13 @@ public class DomainVerificationService extends SystemService } } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } @Override public void setDomainVerificationStatusInternal(@Nullable String packageName, int state, @Nullable ArraySet domains) throws NameNotFoundException { - mEnforcer.assertInternal(mConnection.get().getCallingUid()); + mEnforcer.assertInternal(mConnection.getCallingUid()); switch (state) { case DomainVerificationState.STATE_NO_RESPONSE: @@ -309,7 +313,7 @@ public class DomainVerificationService extends SystemService for (int index = 0; index < size; index++) { DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.get().getPackageSettingLocked(pkgName); + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); if (pkgSetting == null || pkgSetting.getPkg() == null) { continue; } @@ -336,7 +340,7 @@ public class DomainVerificationService extends SystemService throw DomainVerificationUtils.throwPackageUnavailable(packageName); } - PackageSetting pkgSetting = mConnection.get().getPackageSettingLocked(packageName); + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(packageName); if (pkgSetting == null || pkgSetting.getPkg() == null) { throw DomainVerificationUtils.throwPackageUnavailable(packageName); } @@ -352,7 +356,7 @@ public class DomainVerificationService extends SystemService } } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } private void setDomainVerificationStatusInternal(@NonNull DomainVerificationPkgState pkgState, @@ -368,14 +372,13 @@ public class DomainVerificationService extends SystemService public void setDomainVerificationLinkHandlingAllowed(@NonNull String packageName, boolean allowed) throws NameNotFoundException { setDomainVerificationLinkHandlingAllowed(packageName, allowed, - mConnection.get().getCallingUserId()); + mConnection.getCallingUserId()); } public void setDomainVerificationLinkHandlingAllowed(@NonNull String packageName, boolean allowed, @UserIdInt int userId) throws NameNotFoundException { - Connection connection = mConnection.get(); - mEnforcer.assertApprovedUserSelector(connection.getCallingUid(), - connection.getCallingUserId(), userId); + mEnforcer.assertApprovedUserSelector(mConnection.getCallingUid(), + mConnection.getCallingUserId(), userId); synchronized (mLock) { DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); if (pkgState == null) { @@ -386,13 +389,13 @@ public class DomainVerificationService extends SystemService .setDisallowLinkHandling(!allowed); } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } @Override public void setDomainVerificationLinkHandlingAllowedInternal(@Nullable String packageName, boolean allowed, @UserIdInt int userId) throws NameNotFoundException { - mEnforcer.assertInternal(mConnection.get().getCallingUid()); + mEnforcer.assertInternal(mConnection.getCallingUid()); if (packageName == null) { synchronized (mLock) { int pkgStateSize = mAttachedPkgStates.size(); @@ -426,7 +429,7 @@ public class DomainVerificationService extends SystemService } } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } @Override @@ -434,15 +437,14 @@ public class DomainVerificationService extends SystemService @NonNull Set domains, boolean enabled) throws InvalidDomainSetException, NameNotFoundException { setDomainVerificationUserSelection(domainSetId, domains, enabled, - mConnection.get().getCallingUserId()); + mConnection.getCallingUserId()); } public void setDomainVerificationUserSelection(@NonNull UUID domainSetId, @NonNull Set domains, boolean enabled, @UserIdInt int userId) throws InvalidDomainSetException, NameNotFoundException { - Connection connection = mConnection.get(); - mEnforcer.assertApprovedUserSelector(connection.getCallingUid(), - connection.getCallingUserId(), userId); + mEnforcer.assertApprovedUserSelector(mConnection.getCallingUid(), + mConnection.getCallingUserId(), userId); synchronized (mLock) { DomainVerificationPkgState pkgState = getAndValidateAttachedLocked(domainSetId, domains, false /* forAutoVerify */); @@ -454,14 +456,14 @@ public class DomainVerificationService extends SystemService } } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } @Override public void setDomainVerificationUserSelectionInternal(@UserIdInt int userId, @Nullable String packageName, boolean enabled, @NonNull ArraySet domains) throws NameNotFoundException { - mEnforcer.assertInternal(mConnection.get().getCallingUid()); + mEnforcer.assertInternal(mConnection.getCallingUid()); if (packageName == null) { synchronized (mLock) { @@ -471,7 +473,7 @@ public class DomainVerificationService extends SystemService for (int index = 0; index < size; index++) { DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.get().getPackageSettingLocked(pkgName); + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); if (pkgSetting == null || pkgSetting.getPkg() == null) { continue; } @@ -490,7 +492,7 @@ public class DomainVerificationService extends SystemService throw DomainVerificationUtils.throwPackageUnavailable(packageName); } - PackageSetting pkgSetting = mConnection.get().getPackageSettingLocked(packageName); + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(packageName); if (pkgSetting == null || pkgSetting.getPkg() == null) { throw DomainVerificationUtils.throwPackageUnavailable(packageName); } @@ -500,7 +502,7 @@ public class DomainVerificationService extends SystemService } } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } private void setDomainVerificationUserSelectionInternal(int userId, @@ -529,7 +531,7 @@ public class DomainVerificationService extends SystemService } } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } @Nullable @@ -537,23 +539,22 @@ public class DomainVerificationService extends SystemService public DomainVerificationUserSelection getDomainVerificationUserSelection( @NonNull String packageName) throws NameNotFoundException { return getDomainVerificationUserSelection(packageName, - mConnection.get().getCallingUserId()); + mConnection.getCallingUserId()); } @Nullable @Override public DomainVerificationUserSelection getDomainVerificationUserSelection( @NonNull String packageName, @UserIdInt int userId) throws NameNotFoundException { - Connection connection = mConnection.get(); - mEnforcer.assertApprovedUserSelector(connection.getCallingUid(), - connection.getCallingUserId(), userId); + mEnforcer.assertApprovedUserSelector(mConnection.getCallingUid(), + mConnection.getCallingUserId(), userId); synchronized (mLock) { DomainVerificationPkgState pkgState = mAttachedPkgStates.get(packageName); if (pkgState == null) { return null; } - AndroidPackage pkg = connection.getPackageLocked(packageName); + AndroidPackage pkg = mConnection.getPackageLocked(packageName); if (pkg == null) { throw DomainVerificationUtils.throwPackageUnavailable(packageName); } @@ -834,7 +835,7 @@ public class DomainVerificationService extends SystemService @Override public void setLegacyUserState(@NonNull String packageName, @UserIdInt int userId, int state) { - mEnforcer.callerIsLegacyUserSelector(mConnection.get().getCallingUid()); + mEnforcer.callerIsLegacyUserSelector(mConnection.getCallingUid()); mLegacySettings.add(packageName, userId, state); } @@ -854,7 +855,7 @@ public class DomainVerificationService extends SystemService mAttachedPkgStates.remove(packageName); } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } @Override @@ -868,7 +869,7 @@ public class DomainVerificationService extends SystemService mSettings.removeUser(userId); } - mConnection.get().scheduleWriteSettings(); + mConnection.scheduleWriteSettings(); } @Override @@ -880,7 +881,7 @@ public class DomainVerificationService extends SystemService public void printState(@NonNull IndentingPrintWriter writer, @Nullable String packageName, @Nullable @UserIdInt Integer userId) throws NameNotFoundException { synchronized (mLock) { - mDebug.printState(writer, packageName, userId, mConnection.get(), mAttachedPkgStates); + mDebug.printState(writer, packageName, userId, mConnection, mAttachedPkgStates); } } @@ -925,7 +926,7 @@ public class DomainVerificationService extends SystemService } String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.get().getPackageSettingLocked(pkgName); + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); if (pkgSetting == null || pkgSetting.getPkg() == null) { throw DomainVerificationUtils.throwPackageUnavailable(pkgName); } @@ -949,7 +950,7 @@ public class DomainVerificationService extends SystemService @Override public void verifyPackages(@Nullable List packageNames, boolean reVerify) { - mEnforcer.assertInternal(mConnection.get().getCallingUid()); + mEnforcer.assertInternal(mConnection.getCallingUid()); Set packagesToBroadcast = new ArraySet<>(); if (packageNames == null) { @@ -1013,14 +1014,14 @@ public class DomainVerificationService extends SystemService @Override public void clearDomainVerificationState(@Nullable List packageNames) { - mEnforcer.assertInternal(mConnection.get().getCallingUid()); + mEnforcer.assertInternal(mConnection.getCallingUid()); synchronized (mLock) { if (packageNames == null) { int size = mAttachedPkgStates.size(); for (int index = 0; index < size; index++) { DomainVerificationPkgState pkgState = mAttachedPkgStates.valueAt(index); String pkgName = pkgState.getPackageName(); - PackageSetting pkgSetting = mConnection.get().getPackageSettingLocked(pkgName); + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); if (pkgSetting == null || pkgSetting.getPkg() == null) { continue; } @@ -1031,7 +1032,7 @@ public class DomainVerificationService extends SystemService for (int index = 0; index < size; index++) { String pkgName = packageNames.get(index); DomainVerificationPkgState pkgState = mAttachedPkgStates.get(pkgName); - PackageSetting pkgSetting = mConnection.get().getPackageSettingLocked(pkgName); + PackageSetting pkgSetting = mConnection.getPackageSettingLocked(pkgName); if (pkgSetting == null || pkgSetting.getPkg() == null) { continue; } @@ -1071,7 +1072,7 @@ public class DomainVerificationService extends SystemService @Override public void clearUserSelections(@Nullable List packageNames, @UserIdInt int userId) { - mEnforcer.assertInternal(mConnection.get().getCallingUid()); + mEnforcer.assertInternal(mConnection.getCallingUid()); synchronized (mLock) { if (packageNames == null) { int size = mAttachedPkgStates.size(); @@ -1226,45 +1227,4 @@ public class DomainVerificationService extends SystemService Slog.d(TAG + "Approval", packageName + " was " + approvalString + " for " + intent + " for user " + userId + ": " + reason); } - - public interface Connection { - - /** - * Notify that a settings change has been made and that eventually - * {@link #writeSettings(TypedXmlSerializer)} should be invoked by the parent. - */ - void scheduleWriteSettings(); - - /** - * Delegate to {@link Binder#getCallingUid()} to allow mocking in tests. - */ - int getCallingUid(); - - /** - * Delegate to {@link UserHandle#getCallingUserId()} to allow mocking in tests. - */ - @UserIdInt - int getCallingUserId(); - - /** - * @see DomainVerificationProxy.BaseConnection#schedule(int, java.lang.Object) - */ - void schedule(int code, @Nullable Object object); - - /** - * This can only be called when the internal {@link #mLock} is held. Otherwise it's possible - * to deadlock with {@link PackageManagerService}. - */ - @GuardedBy("mLock") - @Nullable - PackageSetting getPackageSettingLocked(@NonNull String pkgName); - - /** - * This can only be called when the internal {@link #mLock} is held. Otherwise it's possible - * to deadlock with {@link PackageManagerService}. - */ - @GuardedBy("mLock") - @Nullable - AndroidPackage getPackageLocked(@NonNull String pkgName); - } } diff --git a/services/java/com/android/server/SystemServer.java b/services/java/com/android/server/SystemServer.java index b09d8a67577fb..7b117d76b3282 100644 --- a/services/java/com/android/server/SystemServer.java +++ b/services/java/com/android/server/SystemServer.java @@ -87,7 +87,6 @@ import android.util.DisplayMetrics; import android.util.EventLog; import android.util.IndentingPrintWriter; import android.util.Pair; -import android.util.Singleton; import android.util.Slog; import android.util.TimeUtils; import android.view.contentcapture.ContentCaptureManager; @@ -1064,16 +1063,7 @@ public final class SystemServer implements Dumpable { t.traceBegin("StartDomainVerificationService"); DomainVerificationService domainVerificationService = new DomainVerificationService( - mSystemContext, SystemConfig.getInstance(), platformCompat, - new Singleton() { - @Override - protected DomainVerificationService.Connection create() { - // Deferred retrieval from PackageManagerService, since PMS is initialized after - // DVS. The alternative would be to expose this through the PackageManagerInternal - // local service, but making it visible to consumers of that interface isn't useful. - return mPackageManagerService.getDomainVerificationConnection(); - } - }); + mSystemContext, SystemConfig.getInstance(), platformCompat); mSystemServiceManager.startService(domainVerificationService); t.traceEnd(); diff --git a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/domain/verify/DomainVerificationEnforcerTest.kt b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/domain/verify/DomainVerificationEnforcerTest.kt index c944cff8c04f1..fbd5ae8863369 100644 --- a/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/domain/verify/DomainVerificationEnforcerTest.kt +++ b/services/tests/PackageManagerServiceTests/unit/src/com/android/server/pm/test/domain/verify/DomainVerificationEnforcerTest.kt @@ -30,6 +30,7 @@ import android.util.SparseArray import androidx.test.platform.app.InstrumentationRegistry import com.android.server.pm.PackageSetting import com.android.server.pm.domain.verify.DomainVerificationEnforcer +import com.android.server.pm.domain.verify.DomainVerificationManagerInternal import com.android.server.pm.domain.verify.DomainVerificationService import com.android.server.pm.domain.verify.proxy.DomainVerificationProxy import com.android.server.pm.parsing.pkg.AndroidPackage @@ -140,18 +141,16 @@ class DomainVerificationEnforcerTest { any() ) ) { true } - }, - object : Singleton() { - override fun create(): DomainVerificationService.Connection = - mockThrowOnUnmocked { - whenever(callingUid) { callingUidInt.get() } - whenever(callingUserId) { callingUserIdInt.get() } - whenever(getPackageSettingLocked(TEST_PKG)) { mockPkgSetting } - whenever(getPackageLocked(TEST_PKG)) { mockPkg } - whenever(schedule(anyInt(), any())) - whenever(scheduleWriteSettings()) - } - }) + }).apply { + setConnection(mockThrowOnUnmocked { + whenever(callingUid) { callingUidInt.get() } + whenever(callingUserId) { callingUserIdInt.get() } + whenever(getPackageSettingLocked(TEST_PKG)) { mockPkgSetting } + whenever(getPackageLocked(TEST_PKG)) { mockPkg } + whenever(schedule(anyInt(), any())) + whenever(scheduleWriteSettings()) + }) + } ) }