Merge "Don't take the DPMS lock in DPMI.createPackageSuspendedDialogIntent" into nyc-dev

core/java/android/app/admin/DevicePolicyManagerInternal.java

@@ -47,6 +47,8 @@ public abstract class DevicePolicyManagerInternal {
      * Gets the packages whose widget providers are white-listed to be
      * available in the parent user.
      *
+     * <p>This takes the DPMS lock.  DO NOT call from PM/UM/AM with their lock held.
+     *
      * @param profileId The profile id.
      * @return The list of packages if such or empty list if there are
      *    no white-listed packages or the profile id is not a managed
@@ -58,6 +60,8 @@ public abstract class DevicePolicyManagerInternal {
      * Adds a listener for changes in the white-listed packages to show
      * cross-profile app widgets.
      *
+     * <p>This takes the DPMS lock.  DO NOT call from PM/UM/AM with their lock held.
+     *
      * @param listener The listener to add.
      */
     public abstract void addOnCrossProfileWidgetProvidersChangeListener(
@@ -66,6 +70,9 @@ public abstract class DevicePolicyManagerInternal {
     /**
      * Checks if an app with given uid is an active device admin of its user and has the policy
      * specified.
+     *
+     * <p>This takes the DPMS lock.  DO NOT call from PM/UM/AM with their lock held.
+     *
      * @param uid App uid.
      * @param reqPolicy Required policy, for policies see {@link DevicePolicyManager}.
      * @return true if the uid is an active admin with the given policy.
@@ -77,6 +84,8 @@ public abstract class DevicePolicyManagerInternal {
      * suspended by the admin. This assumes that {@param packageName} is suspended by the
      * device/profile owner. The caller should check if the package is suspended or not.
      *
+     * <p>This method does not take the DPMS lock.  Safe to be called from anywhere.
+     *
      * @param packageName The package that is suspended
      * @param userId The user having the suspended package.
      * @return The intent to trigger the admin support dialog.

services/core/java/com/android/server/am/ActivityStartInterceptor.java

@@ -147,6 +147,9 @@ class ActivityStartInterceptor {
         }
         DevicePolicyManagerInternal devicePolicyManager = LocalServices.getService(
                 DevicePolicyManagerInternal.class);
+        if (devicePolicyManager == null) {
+            return false;
+        }
         mIntent = devicePolicyManager.createPackageSuspendedDialogIntent(
                 mAInfo.packageName, mUserId);
         mCallingPid = mRealCallingPid;

services/devicepolicy/java/com/android/server/devicepolicy/DevicePolicyManagerService.java

@@ -8139,21 +8139,23 @@ public class DevicePolicyManagerService extends IDevicePolicyManager.Stub {
             Intent intent = new Intent(Settings.ACTION_SHOW_ADMIN_SUPPORT_DETAILS);
             intent.putExtra(Intent.EXTRA_USER_ID, userId);
             intent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
-            synchronized (DevicePolicyManagerService.this) {
-                ComponentName profileOwner = mOwners.getProfileOwnerComponent(userId);
-                if (profileOwner != null) {
-                    intent.putExtra(DevicePolicyManager.EXTRA_DEVICE_ADMIN, profileOwner);
-                    return intent;
-                }
 
-                if (mOwners.getDeviceOwnerUserId() == userId) {
+            // This method is called from AM with its lock held, so don't take the DPMS lock.
-                    ComponentName deviceOwner = mOwners.getDeviceOwnerComponent();
+            // b/29242568
-                    if (deviceOwner != null) {
+
-                        intent.putExtra(DevicePolicyManager.EXTRA_DEVICE_ADMIN, deviceOwner);
+            ComponentName profileOwner = mOwners.getProfileOwnerComponent(userId);
-                        return intent;
+            if (profileOwner != null) {
-                    }
+                intent.putExtra(DevicePolicyManager.EXTRA_DEVICE_ADMIN, profileOwner);
-                }
+                return intent;
             }
+
+            final Pair<Integer, ComponentName> deviceOwner =
+                    mOwners.getDeviceOwnerUserIdAndComponent();
+            if (deviceOwner != null && deviceOwner.first == userId) {
+                intent.putExtra(DevicePolicyManager.EXTRA_DEVICE_ADMIN, deviceOwner.second);
+                return intent;
+            }
+
             // We're not specifying the device admin because there isn't one.
             return intent;
         }

services/devicepolicy/java/com/android/server/devicepolicy/Owners.java

@@ -16,9 +16,9 @@
 
 package com.android.server.devicepolicy;
 
+import android.annotation.Nullable;
 import android.app.admin.SystemUpdatePolicy;
 import android.content.ComponentName;
-import android.content.Context;
 import android.content.pm.PackageManagerInternal;
 import android.content.pm.UserInfo;
 import android.os.Environment;
@@ -28,6 +28,7 @@ import android.os.UserManagerInternal;
 import android.util.ArrayMap;
 import android.util.AtomicFile;
 import android.util.Log;
+import android.util.Pair;
 import android.util.Slog;
 import android.util.SparseArray;
 import android.util.Xml;
@@ -54,8 +55,8 @@ import libcore.io.IoUtils;
  * Stores and restores state for the Device and Profile owners. By definition there can be
  * only one device owner, but there may be a profile owner for each user.
  *
- * <p>This class is not thread safe.  (i.e. access to this class must always be synchronized
+ * <p>This class is thread safe, so individual methods can safely be called without locking.
- * in the caller side.)
+ * However, caller must still synchronize on their side to ensure integrity between multiple calls.
  */
 class Owners {
     private static final String TAG = "DevicePolicyManagerService";
@@ -101,6 +102,8 @@ class Owners {
     // Local system update policy controllable by device owner.
     private SystemUpdatePolicy mSystemUpdatePolicy;
 
+    private final Object mLock = new Object();
+
     public Owners(UserManager userManager,
             UserManagerInternal userManagerInternal,
             PackageManagerInternal packageManagerInternal) {
@@ -113,47 +116,49 @@ class Owners {
      * Load configuration from the disk.
      */
     void load() {
-        // First, try to read from the legacy file.
+        synchronized (mLock) {
-        final File legacy = getLegacyConfigFileWithTestOverride();
+            // First, try to read from the legacy file.
+            final File legacy = getLegacyConfigFileWithTestOverride();
 
-        final List<UserInfo> users = mUserManager.getUsers();
+            final List<UserInfo> users = mUserManager.getUsers();
 
-        if (readLegacyOwnerFile(legacy)) {
+            if (readLegacyOwnerFileLocked(legacy)) {
-            if (DEBUG) {
+                if (DEBUG) {
-                Log.d(TAG, "Legacy config file found.");
+                    Log.d(TAG, "Legacy config file found.");
-            }
+                }
 
-            // Legacy file exists, write to new files and remove the legacy one.
+                // Legacy file exists, write to new files and remove the legacy one.
-            writeDeviceOwner();
+                writeDeviceOwner();
-            for (int userId : getProfileOwnerKeys()) {
+                for (int userId : getProfileOwnerKeys()) {
-                writeProfileOwner(userId);
+                    writeProfileOwner(userId);
-            }
+                }
-            if (DEBUG) {
+                if (DEBUG) {
-                Log.d(TAG, "Deleting legacy config file");
+                    Log.d(TAG, "Deleting legacy config file");
-            }
+                }
-            if (!legacy.delete()) {
+                if (!legacy.delete()) {
-                Slog.e(TAG, "Failed to remove the legacy setting file");
+                    Slog.e(TAG, "Failed to remove the legacy setting file");
-            }
+                }
-        } else {
+            } else {
-            // No legacy file, read from the new format files.
+                // No legacy file, read from the new format files.
-            new DeviceOwnerReadWriter().readFromFileLocked();
+                new DeviceOwnerReadWriter().readFromFileLocked();
 
+                for (UserInfo ui : users) {
+                    new ProfileOwnerReadWriter(ui.id).readFromFileLocked();
+                }
+            }
+            mUserManagerInternal.setDeviceManaged(hasDeviceOwner());
             for (UserInfo ui : users) {
-                new ProfileOwnerReadWriter(ui.id).readFromFileLocked();
+                mUserManagerInternal.setUserManaged(ui.id, hasProfileOwner(ui.id));
             }
+            if (hasDeviceOwner() && hasProfileOwner(getDeviceOwnerUserId())) {
+                Slog.w(TAG, String.format("User %d has both DO and PO, which is not supported",
+                        getDeviceOwnerUserId()));
+            }
+            pushToPackageManagerLocked();
         }
-        mUserManagerInternal.setDeviceManaged(hasDeviceOwner());
-        for (UserInfo ui : users) {
-            mUserManagerInternal.setUserManaged(ui.id, hasProfileOwner(ui.id));
-        }
-        if (hasDeviceOwner() && hasProfileOwner(getDeviceOwnerUserId())) {
-            Slog.w(TAG, String.format("User %d has both DO and PO, which is not supported",
-                    getDeviceOwnerUserId()));
-        }
-        pushToPackageManager();
     }
 
-    private void pushToPackageManager() {
+    private void pushToPackageManagerLocked() {
         final SparseArray<String> po = new SparseArray<>();
         for (int i = mProfileOwners.size() - 1; i >= 0; i--) {
             po.put(mProfileOwners.keyAt(i), mProfileOwners.valueAt(i).packageName);
@@ -164,27 +169,50 @@ class Owners {
     }
 
     String getDeviceOwnerPackageName() {
-        return mDeviceOwner != null ? mDeviceOwner.packageName : null;
+        synchronized (mLock) {
+            return mDeviceOwner != null ? mDeviceOwner.packageName : null;
+        }
     }
 
     int getDeviceOwnerUserId() {
-        return mDeviceOwnerUserId;
+        synchronized (mLock) {
+            return mDeviceOwnerUserId;
+        }
+    }
+
+    @Nullable
+    Pair<Integer, ComponentName> getDeviceOwnerUserIdAndComponent() {
+        synchronized (mLock) {
+            if (mDeviceOwner == null) {
+                return null;
+            } else {
+                return Pair.create(mDeviceOwnerUserId, mDeviceOwner.admin);
+            }
+        }
     }
 
     String getDeviceOwnerName() {
-        return mDeviceOwner != null ? mDeviceOwner.name : null;
+        synchronized (mLock) {
+            return mDeviceOwner != null ? mDeviceOwner.name : null;
+        }
     }
 
     ComponentName getDeviceOwnerComponent() {
-        return mDeviceOwner != null ? mDeviceOwner.admin : null;
+        synchronized (mLock) {
+            return mDeviceOwner != null ? mDeviceOwner.admin : null;
+        }
     }
 
     String getDeviceOwnerRemoteBugreportUri() {
-        return mDeviceOwner != null ? mDeviceOwner.remoteBugreportUri : null;
+        synchronized (mLock) {
+            return mDeviceOwner != null ? mDeviceOwner.remoteBugreportUri : null;
+        }
     }
 
     String getDeviceOwnerRemoteBugreportHash() {
-        return mDeviceOwner != null ? mDeviceOwner.remoteBugreportHash : null;
+        synchronized (mLock) {
+            return mDeviceOwner != null ? mDeviceOwner.remoteBugreportHash : null;
+        }
     }
 
     void setDeviceOwner(ComponentName admin, String ownerName, int userId) {
@@ -192,132 +220,172 @@ class Owners {
             Slog.e(TAG, "Invalid user id for device owner user: " + userId);
             return;
         }
-        // For a newly set DO, there's no need for migration.
+        synchronized (mLock) {
-        setDeviceOwnerWithRestrictionsMigrated(admin, ownerName, userId,
+            // For a newly set DO, there's no need for migration.
-                /* userRestrictionsMigrated =*/ true);
+            setDeviceOwnerWithRestrictionsMigrated(admin, ownerName, userId,
+                    /* userRestrictionsMigrated =*/ true);
+        }
     }
 
     // Note this should be only called during migration.  Normally when DO is set,
     // userRestrictionsMigrated should always be true.
     void setDeviceOwnerWithRestrictionsMigrated(ComponentName admin, String ownerName, int userId,
             boolean userRestrictionsMigrated) {
-        mDeviceOwner = new OwnerInfo(ownerName, admin, userRestrictionsMigrated,
+        synchronized (mLock) {
-                /* remoteBugreportUri =*/ null, /* remoteBugreportHash =*/ null);
+            mDeviceOwner = new OwnerInfo(ownerName, admin, userRestrictionsMigrated,
-        mDeviceOwnerUserId = userId;
+                    /* remoteBugreportUri =*/ null, /* remoteBugreportHash =*/ null);
+            mDeviceOwnerUserId = userId;
 
-        mUserManagerInternal.setDeviceManaged(true);
+            mUserManagerInternal.setDeviceManaged(true);
-        pushToPackageManager();
+            pushToPackageManagerLocked();
+        }
     }
 
     void clearDeviceOwner() {
-        mDeviceOwner = null;
+        synchronized (mLock) {
-        mDeviceOwnerUserId = UserHandle.USER_NULL;
+            mDeviceOwner = null;
+            mDeviceOwnerUserId = UserHandle.USER_NULL;
 
-        mUserManagerInternal.setDeviceManaged(false);
+            mUserManagerInternal.setDeviceManaged(false);
-        pushToPackageManager();
+            pushToPackageManagerLocked();
+        }
     }
 
     void setProfileOwner(ComponentName admin, String ownerName, int userId) {
-        // For a newly set PO, there's no need for migration.
+        synchronized (mLock) {
-        mProfileOwners.put(userId, new OwnerInfo(ownerName, admin,
+            // For a newly set PO, there's no need for migration.
-                /* userRestrictionsMigrated =*/ true, /* remoteBugreportUri =*/ null,
+            mProfileOwners.put(userId, new OwnerInfo(ownerName, admin,
-                /* remoteBugreportHash =*/ null));
+                    /* userRestrictionsMigrated =*/ true, /* remoteBugreportUri =*/ null,
-        mUserManagerInternal.setUserManaged(userId, true);
+                    /* remoteBugreportHash =*/ null));
-        pushToPackageManager();
+            mUserManagerInternal.setUserManaged(userId, true);
+            pushToPackageManagerLocked();
+        }
     }
 
     void removeProfileOwner(int userId) {
-        mProfileOwners.remove(userId);
+        synchronized (mLock) {
-        mUserManagerInternal.setUserManaged(userId, false);
+            mProfileOwners.remove(userId);
-        pushToPackageManager();
+            mUserManagerInternal.setUserManaged(userId, false);
+            pushToPackageManagerLocked();
+        }
     }
 
     ComponentName getProfileOwnerComponent(int userId) {
-        OwnerInfo profileOwner = mProfileOwners.get(userId);
+        synchronized (mLock) {
-        return profileOwner != null ? profileOwner.admin : null;
+            OwnerInfo profileOwner = mProfileOwners.get(userId);
+            return profileOwner != null ? profileOwner.admin : null;
+        }
     }
 
     String getProfileOwnerName(int userId) {
-        OwnerInfo profileOwner = mProfileOwners.get(userId);
+        synchronized (mLock) {
-        return profileOwner != null ? profileOwner.name : null;
+            OwnerInfo profileOwner = mProfileOwners.get(userId);
+            return profileOwner != null ? profileOwner.name : null;
+        }
     }
 
     String getProfileOwnerPackage(int userId) {
-        OwnerInfo profileOwner = mProfileOwners.get(userId);
+        synchronized (mLock) {
-        return profileOwner != null ? profileOwner.packageName : null;
+            OwnerInfo profileOwner = mProfileOwners.get(userId);
+            return profileOwner != null ? profileOwner.packageName : null;
+        }
     }
 
     Set<Integer> getProfileOwnerKeys() {
-        return mProfileOwners.keySet();
+        synchronized (mLock) {
+            return mProfileOwners.keySet();
+        }
     }
 
     SystemUpdatePolicy getSystemUpdatePolicy() {
-        return mSystemUpdatePolicy;
+        synchronized (mLock) {
+            return mSystemUpdatePolicy;
+        }
     }
 
     void setSystemUpdatePolicy(SystemUpdatePolicy systemUpdatePolicy) {
-        mSystemUpdatePolicy = systemUpdatePolicy;
+        synchronized (mLock) {
+            mSystemUpdatePolicy = systemUpdatePolicy;
+        }
     }
 
     void clearSystemUpdatePolicy() {
-        mSystemUpdatePolicy = null;
+        synchronized (mLock) {
+            mSystemUpdatePolicy = null;
+        }
     }
 
     boolean hasDeviceOwner() {
-        return mDeviceOwner != null;
+        synchronized (mLock) {
+            return mDeviceOwner != null;
+        }
     }
 
     boolean isDeviceOwnerUserId(int userId) {
-        return mDeviceOwner != null && mDeviceOwnerUserId == userId;
+        synchronized (mLock) {
+            return mDeviceOwner != null && mDeviceOwnerUserId == userId;
+        }
     }
 
     boolean hasProfileOwner(int userId) {
-        return getProfileOwnerComponent(userId) != null;
+        synchronized (mLock) {
+            return getProfileOwnerComponent(userId) != null;
+        }
     }
 
     /**
      * @return true if user restrictions need to be migrated for DO.
      */
     boolean getDeviceOwnerUserRestrictionsNeedsMigration() {
-        return mDeviceOwner != null && !mDeviceOwner.userRestrictionsMigrated;
+        synchronized (mLock) {
+            return mDeviceOwner != null && !mDeviceOwner.userRestrictionsMigrated;
+        }
     }
 
     /**
      * @return true if user restrictions need to be migrated for PO.
      */
     boolean getProfileOwnerUserRestrictionsNeedsMigration(int userId) {
-        OwnerInfo profileOwner = mProfileOwners.get(userId);
+        synchronized (mLock) {
-        return profileOwner != null && !profileOwner.userRestrictionsMigrated;
+            OwnerInfo profileOwner = mProfileOwners.get(userId);
+            return profileOwner != null && !profileOwner.userRestrictionsMigrated;
+        }
     }
 
     /** Sets the user restrictions migrated flag, and also writes to the file. */
     void setDeviceOwnerUserRestrictionsMigrated() {
-        if (mDeviceOwner != null) {
+        synchronized (mLock) {
-            mDeviceOwner.userRestrictionsMigrated = true;
+            if (mDeviceOwner != null) {
+                mDeviceOwner.userRestrictionsMigrated = true;
+            }
+            writeDeviceOwner();
         }
-        writeDeviceOwner();
     }
 
     /** Sets the remote bugreport uri and hash, and also writes to the file. */
     void setDeviceOwnerRemoteBugreportUriAndHash(String remoteBugreportUri,
             String remoteBugreportHash) {
-        if (mDeviceOwner != null) {
+        synchronized (mLock) {
-            mDeviceOwner.remoteBugreportUri = remoteBugreportUri;
+            if (mDeviceOwner != null) {
-            mDeviceOwner.remoteBugreportHash = remoteBugreportHash;
+                mDeviceOwner.remoteBugreportUri = remoteBugreportUri;
+                mDeviceOwner.remoteBugreportHash = remoteBugreportHash;
+            }
+            writeDeviceOwner();
         }
-        writeDeviceOwner();
     }
 
     /** Sets the user restrictions migrated flag, and also writes to the file.  */
     void setProfileOwnerUserRestrictionsMigrated(int userId) {
-        OwnerInfo profileOwner = mProfileOwners.get(userId);
+        synchronized (mLock) {
-        if (profileOwner != null) {
+            OwnerInfo profileOwner = mProfileOwners.get(userId);
-            profileOwner.userRestrictionsMigrated = true;
+            if (profileOwner != null) {
+                profileOwner.userRestrictionsMigrated = true;
+            }
+            writeProfileOwner(userId);
         }
-        writeProfileOwner(userId);
     }
 
-    private boolean readLegacyOwnerFile(File file) {
+    private boolean readLegacyOwnerFileLocked(File file) {
         if (!file.exists()) {
             // Already migrated or the device has no owners.
             return false;
@@ -383,7 +451,7 @@ class Owners {
     }
 
     void writeDeviceOwner() {
-        synchronized (this) {
+        synchronized (mLock) {
             if (DEBUG) {
                 Log.d(TAG, "Writing to device owner file");
             }
@@ -392,7 +460,7 @@ class Owners {
     }
 
     void writeProfileOwner(int userId) {
-        synchronized (this) {
+        synchronized (mLock) {
             if (DEBUG) {
                 Log.d(TAG, "Writing to profile owner file for user " + userId);
             }