commit 15c8697e88b781c75cc1fd0beed2e9af94e5cfbb
author Cassie Wang <cassiewang@google.com> Tue Feb 09 13:43:25 2021 -0800
committer Cassie Wang <cassiewang@google.com> Thu Feb 11 14:30:59 2021 -0800
tree f47c88589e9d7e8c78c34cc17f6022924ca8665d
parent edf5ee881ce2a840f6c332f4d2127b8bf01928ad

Fix locking issues.

* Synchronize access to unlocked user cache. The cache will be checked
for each API call. Add synchronization blocks when reading/writing to
it.
* Synchronize and remove double locking for access to static array of
AppSearchImpl instances.

Bug: 179406838
Test: atest -m -c --rebuild-module-info CtsAppSearchTestCases
FrameworksCoreTests: android.app.appsearch
FrameworksServicesTests: com.android.server.appsearch
Change-Id: Ibaf0d6ff07b4e2f15f3479ff56e143a34ce453e3

service/java/com/android/server/appsearch/AppSearchManagerService.java

diff --git a/service/java/com/android/server/appsearch/AppSearchManagerService.java b/service/java/com/android/server/appsearch/AppSearchManagerService.java
index 271129b..2c1b299 100644
--- a/service/java/com/android/server/appsearch/AppSearchManagerService.java
+++ b/service/java/com/android/server/appsearch/AppSearchManagerService.java
@@ -41,6 +41,7 @@
 import android.util.ArraySet;
 import android.util.Log;
 
+import com.android.internal.annotations.GuardedBy;
 import com.android.internal.util.Preconditions;
 import com.android.server.LocalServices;
 import com.android.server.SystemService;
@@ -58,8 +59,11 @@
     private PackageManagerInternal mPackageManagerInternal;
     private ImplInstanceManager mImplInstanceManager;
 
-    // Cache of unlocked user ids so we don't have to query UserManager service each time.
-    private final Set<Integer> mUnlockedUserIds = new ArraySet<>();
+    // Cache of unlocked user ids so we don't have to query UserManager service each time. The
+    // "locked" suffix refers to the fact that access to the field should be locked; unrelated to
+    // the unlocked status of user ids.
+    @GuardedBy("mUnlockedUserIdsLocked")
+    private final Set<Integer> mUnlockedUserIdsLocked = new ArraySet<>();
 
     public AppSearchManagerService(Context context) {
         super(context);
@@ -74,7 +78,9 @@
 
     @Override
     public void onUserUnlocked(@NonNull TargetUser user) {
-        mUnlockedUserIds.add(user.getUserIdentifier());
+        synchronized (mUnlockedUserIdsLocked) {
+            mUnlockedUserIdsLocked.add(user.getUserIdentifier());
+        }
     }
 
     private class Stub extends IAppSearchManager.Stub {
@@ -503,9 +509,11 @@
         }
 
         private void verifyUserUnlocked(int callingUserId) {
-            if (!mUnlockedUserIds.contains(callingUserId)) {
-                throw new IllegalStateException(
-                        "User " + callingUserId + " is locked or not running.");
+            synchronized (mUnlockedUserIdsLocked) {
+                if (!mUnlockedUserIdsLocked.contains(callingUserId)) {
+                    throw new IllegalStateException(
+                            "User " + callingUserId + " is locked or not running.");
+                }
             }
         }
 

service/java/com/android/server/appsearch/ImplInstanceManager.java

diff --git a/service/java/com/android/server/appsearch/ImplInstanceManager.java b/service/java/com/android/server/appsearch/ImplInstanceManager.java
index 5ea2a02..82319d4 100644
--- a/service/java/com/android/server/appsearch/ImplInstanceManager.java
+++ b/service/java/com/android/server/appsearch/ImplInstanceManager.java
@@ -29,6 +29,7 @@
 import android.util.SparseArray;
 
 import com.android.internal.R;
+import com.android.internal.annotations.GuardedBy;
 import com.android.server.appsearch.external.localstorage.AppSearchImpl;
 
 import java.io.File;
@@ -43,7 +44,9 @@
 
     private static ImplInstanceManager sImplInstanceManager;
 
-    private final SparseArray<AppSearchImpl> mInstances = new SparseArray<>();
+    @GuardedBy("mInstancesLocked")
+    private final SparseArray<AppSearchImpl> mInstancesLocked = new SparseArray<>();
+
     private final String mGlobalQuerierPackage;
 
     private ImplInstanceManager(@NonNull String globalQuerierPackage) {
@@ -81,19 +84,16 @@
      * @return An initialized {@link AppSearchImpl} for this user
      */
     @NonNull
-    public AppSearchImpl getAppSearchImpl(@NonNull Context context, @UserIdInt int userId)
-            throws AppSearchException {
-        AppSearchImpl instance = mInstances.get(userId);
-        if (instance == null) {
-            synchronized (ImplInstanceManager.class) {
-                instance = mInstances.get(userId);
-                if (instance == null) {
-                    instance = createImpl(context, userId);
-                    mInstances.put(userId, instance);
-                }
+    public AppSearchImpl getAppSearchImpl(
+            @NonNull Context context, @UserIdInt int userId) throws AppSearchException {
+        synchronized (mInstancesLocked) {
+            AppSearchImpl instance = mInstancesLocked.get(userId);
+            if (instance == null) {
+                instance = createImpl(context, userId);
+                mInstancesLocked.put(userId, instance);
             }
+            return instance;
         }
-        return instance;
     }
 
     private AppSearchImpl createImpl(@NonNull Context context, @UserIdInt int userId)