From 4602e87f41eae248a8941bf1c24444899acbb6f1 Mon Sep 17 00:00:00 2001 From: Pavel Grafov Date: Tue, 27 Apr 2021 15:53:18 +0100 Subject: [PATCH] Drive-by fix for wtfIfInLock + two violations wtfIfInLock() method was checking 'this' lock while the code is actually using mLockDoNoUseDirectly returned by getLockObject(), thus violation werent reported. This CL changes this and fixes violations when calling hasIncompatibleAccountsOrNonAdbNoLock. Also move it to other lock-related methods. Bug: 162815601 Test: atest com.android.cts.devicepolicy.DeviceOwnerTest#testDeviceOwnerSetup Test: atest com.android.cts.devicepolicy.ProfileOwnerTest#testManagement Test: manual: add account, attempt setting a DO Change-Id: Id64930552f5a9b25464ba942b0b1ddbe9378928e --- .../DevicePolicyManagerService.java | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java index cda01dbae9459..66f47c6c7e0dd 100644 --- a/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java +++ b/services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java @@ -745,6 +745,15 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { Slogf.wtfStack(LOG_TAG, "Not holding DPMS lock."); } + /** + * Calls wtfStack() if called with the DPMS lock held. + */ + private void wtfIfInLock() { + if (Thread.holdsLock(mLockDoNoUseDirectly)) { + Slogf.wtfStack(LOG_TAG, "Shouldn't be called with DPMS lock held"); + } + } + @VisibleForTesting final TransferOwnershipMetadataManager mTransferOwnershipMetadataManager; @@ -8141,8 +8150,11 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { Preconditions.checkArgument(admin != null); final CallerIdentity caller = getCallerIdentity(); + // Cannot be called while holding the lock: + final boolean hasIncompatibleAccountsOrNonAdb = + hasIncompatibleAccountsOrNonAdbNoLock(caller, userId, admin); synchronized (getLockObject()) { - enforceCanSetDeviceOwnerLocked(caller, admin, userId); + enforceCanSetDeviceOwnerLocked(caller, admin, userId, hasIncompatibleAccountsOrNonAdb); Preconditions.checkArgument(isPackageInstalledForUser(admin.getPackageName(), userId), "Invalid component " + admin + " for device owner"); final ActiveAdmin activeAdmin = getActiveAdminUncheckedLocked(admin, userId); @@ -8537,8 +8549,12 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { Preconditions.checkArgument(who != null); final CallerIdentity caller = getCallerIdentity(); + // Cannot be called while holding the lock: + final boolean hasIncompatibleAccountsOrNonAdb = + hasIncompatibleAccountsOrNonAdbNoLock(caller, userHandle, who); synchronized (getLockObject()) { - enforceCanSetProfileOwnerLocked(caller, who, userHandle); + enforceCanSetProfileOwnerLocked( + caller, who, userHandle, hasIncompatibleAccountsOrNonAdb); Preconditions.checkArgument(isPackageInstalledForUser(who.getPackageName(), userHandle), "Component " + who + " not installed for userId:" + userHandle); final ActiveAdmin admin = getActiveAdminUncheckedLocked(who, userHandle); @@ -9116,15 +9132,6 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { }); } - /** - * Calls wtfStack() if called with the DPMS lock held. - */ - private void wtfIfInLock() { - if (Thread.holdsLock(this)) { - Slogf.wtfStack(LOG_TAG, "Shouldn't be called with DPMS lock held"); - } - } - /** * The profile owner can only be set by adb or an app with the MANAGE_PROFILE_AND_DEVICE_OWNERS * permission. @@ -9133,8 +9140,9 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { * - SYSTEM_UID * - adb unless hasIncompatibleAccountsOrNonAdb is true. */ - private void enforceCanSetProfileOwnerLocked(CallerIdentity caller, - @Nullable ComponentName owner, int userHandle) { + private void enforceCanSetProfileOwnerLocked( + CallerIdentity caller, @Nullable ComponentName owner, int userHandle, + boolean hasIncompatibleAccountsOrNonAdb) { UserInfo info = getUserInfo(userHandle); if (info == null) { // User doesn't exist. @@ -9154,7 +9162,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { } if (isAdb(caller)) { if ((mIsWatch || hasUserSetupCompleted(userHandle)) - && hasIncompatibleAccountsOrNonAdbNoLock(caller, userHandle, owner)) { + && hasIncompatibleAccountsOrNonAdb) { throw new IllegalStateException("Not allowed to set the profile owner because " + "there are already some accounts on the profile"); } @@ -9191,8 +9199,9 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { * The Device owner can only be set by adb or an app with the MANAGE_PROFILE_AND_DEVICE_OWNERS * permission. */ - private void enforceCanSetDeviceOwnerLocked(CallerIdentity caller, - @Nullable ComponentName owner, @UserIdInt int deviceOwnerUserId) { + private void enforceCanSetDeviceOwnerLocked( + CallerIdentity caller, @Nullable ComponentName owner, @UserIdInt int deviceOwnerUserId, + boolean hasIncompatibleAccountsOrNonAdb) { if (!isAdb(caller)) { Preconditions.checkCallAuthorization( hasCallingOrSelfPermission(permission.MANAGE_PROFILE_AND_DEVICE_OWNERS)); @@ -9200,8 +9209,7 @@ public class DevicePolicyManagerService extends BaseIDevicePolicyManager { final int code = checkDeviceOwnerProvisioningPreConditionLocked(owner, /* deviceOwnerUserId= */ deviceOwnerUserId, /* callingUserId*/ caller.getUserId(), - isAdb(caller), - hasIncompatibleAccountsOrNonAdbNoLock(caller, deviceOwnerUserId, owner)); + isAdb(caller), hasIncompatibleAccountsOrNonAdb); if (code != CODE_OK) { throw new IllegalStateException( computeProvisioningErrorString(code, deviceOwnerUserId));