commit a22346a17a82fd27c459866ca4cf7ef71a0409ff
author Oli Lan <olilan@google.com> Wed Apr 05 11:46:18 2023 +0100
committer Oli Lan <olilan@google.com> Wed Apr 05 11:46:18 2023 +0100
tree 95a0b884974a9f82346aefa4ecf69c8eabb946c7
parent 901bee878feeebebf548b173f417b48713cba750

Fix potential deadlock in applyUserRestrictions methods.

This fixes a lock inversion issue where the user lock is being taken
out while the restrictions lock is held in UserManagerService.

The user lock is being taken out to retrieve user data in order to
call scheduleWriteUser, but when the write occurs, we don't actually
use that UserData as it is retrieved again. We only use the provided
UserData to get the user id. So, this CL changes scheduleWriteUser to
take the user id instead.

Bug: 273083107
Test: atest UserManagerTest
Change-Id: I11db509235dfc59642871c168bc2f3e1daabe821

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

diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java
index ab9d1cf..73be549 100644
--- a/services/core/java/com/android/server/pm/UserManagerService.java
+++ b/services/core/java/com/android/server/pm/UserManagerService.java
@@ -2592,7 +2592,7 @@
             }
         }
         if (scheduleWriteUser) {
-            scheduleWriteUser(userData);
+            scheduleWriteUser(userId);
         }
     }
 
@@ -2902,7 +2902,7 @@
                     != newBaseRestrictions);
 
             if (mBaseUserRestrictions.updateRestrictions(userId, newBaseRestrictions)) {
-                scheduleWriteUser(getUserDataNoChecks(userId));
+                scheduleWriteUser(userId);
             }
         }
 
@@ -2978,7 +2978,7 @@
     @GuardedBy("mRestrictionsLock")
     private void applyUserRestrictionsLR(@UserIdInt int userId) {
         updateUserRestrictionsInternalLR(null, userId);
-        scheduleWriteUser(getUserDataNoChecks(userId));
+        scheduleWriteUser(userId);
     }
 
     @GuardedBy("mRestrictionsLock")
@@ -4129,14 +4129,14 @@
         }
     }
 
-    private void scheduleWriteUser(UserData userData) {
+    private void scheduleWriteUser(@UserIdInt int userId) {
         if (DBG) {
             debug("scheduleWriteUser");
         }
         // No need to wrap it within a lock -- worst case, we'll just post the same message
         // twice.
-        if (!mHandler.hasMessages(WRITE_USER_MSG, userData)) {
-            Message msg = mHandler.obtainMessage(WRITE_USER_MSG, userData);
+        if (!mHandler.hasMessages(WRITE_USER_MSG, userId)) {
+            Message msg = mHandler.obtainMessage(WRITE_USER_MSG, userId);
             mHandler.sendMessageDelayed(msg, WRITE_USER_DELAY);
         }
     }
@@ -4152,7 +4152,7 @@
             // Something went wrong, schedule full rewrite.
             UserData userData = getUserDataNoChecks(userId);
             if (userData != null) {
-                scheduleWriteUser(userData);
+                scheduleWriteUser(userId);
             }
         });
     }
@@ -6363,7 +6363,7 @@
             userData.info.lastLoggedInTime = now;
         }
         userData.info.lastLoggedInFingerprint = PackagePartitions.FINGERPRINT;
-        scheduleWriteUser(userData);
+        scheduleWriteUser(userId);
     }
 
     /**
@@ -6533,7 +6533,7 @@
 
     private void setLastEnteredForegroundTimeToNow(@NonNull UserData userData) {
         userData.mLastEnteredForegroundTimeMillis = System.currentTimeMillis();
-        scheduleWriteUser(userData);
+        scheduleWriteUser(userData.info.id);
     }
 
     @Override
@@ -6832,7 +6832,7 @@
                 case WRITE_USER_MSG:
                     removeMessages(WRITE_USER_MSG, msg.obj);
                     synchronized (mPackagesLock) {
-                        int userId = ((UserData) msg.obj).info.id;
+                        int userId = (int) msg.obj;
                         UserData userData = getUserDataNoChecks(userId);
                         if (userData != null) {
                             writeUserLP(userData);