From 703cf10c59cf5d98cefe3347e905a492b092f27a Mon Sep 17 00:00:00 2001
From: Hui Yu <huiyu@google.com>
Date: Fri, 10 Dec 2021 16:21:36 -0800
Subject: [PATCH] Synchronize on the SparseArray object in
 PendingTempAllowlists.

The field mPendingTempAllowlist in PendingTempAllowlists is a
SparseArray.

In ActivityManagerService, the lock protection for PendingTempAllowlists
object is:
 @CompositeRWLock({"this", "mProcLock"})
    final PendingTempAllowlists mPendingTempAllowlist

Which means the mPendingTempAllowlist object can be read when either
"this" lock or "mProcLock" lock is held. But the read-operation of
SparseArray such as indexOfKey() and size() etc, actually mutate the
SparseArray by calling SparseArray.gc(). This makes @CompositeRWLock not
to be compatible with SparseArray.

Since we can not make SparseArray thread-safe, also we want to maintain
the semantic of @CompositeRWLock, we can make
PendingTempAllowlists thread-safe at least.

Bug: 193788840
Test: Regression test.
Change-Id: Ie1c239ad27d1fd6b76676951b470605513848b20
---
 .../server/am/ActivityManagerService.java     | 14 +++++----
 .../server/am/PendingTempAllowlists.java      | 29 ++++++++++++++-----
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java
index ebce30ab38493..9d2b4e7a570f3 100644
--- a/services/core/java/com/android/server/am/ActivityManagerService.java
+++ b/services/core/java/com/android/server/am/ActivityManagerService.java
@@ -5823,7 +5823,7 @@ public class ActivityManagerService extends IActivityManager.Stub
 
         return Arrays.binarySearch(allowlist, appId) >= 0
                 || Arrays.binarySearch(mDeviceIdleTempAllowlist, appId) >= 0
-                || mPendingTempAllowlist.indexOfKey(uid) >= 0;
+                || mPendingTempAllowlist.get(uid) != null;
     }
 
     /**
@@ -15122,11 +15122,13 @@ public class ActivityManagerService extends IActivityManager.Stub
 
         // First copy out the pending changes...  we need to leave them in the map for now,
         // in case someone needs to check what is coming up while we don't have the lock held.
-        synchronized (mProcLock) {
-            N = mPendingTempAllowlist.size();
-            list = new PendingTempAllowlist[N];
-            for (int i = 0; i < N; i++) {
-                list[i] = mPendingTempAllowlist.valueAt(i);
+        synchronized (this) {
+            synchronized (mProcLock) {
+                N = mPendingTempAllowlist.size();
+                list = new PendingTempAllowlist[N];
+                for (int i = 0; i < N; i++) {
+                    list[i] = mPendingTempAllowlist.valueAt(i);
+                }
             }
         }
 
diff --git a/services/core/java/com/android/server/am/PendingTempAllowlists.java b/services/core/java/com/android/server/am/PendingTempAllowlists.java
index 75935c4f22fac..0263de7ee5864 100644
--- a/services/core/java/com/android/server/am/PendingTempAllowlists.java
+++ b/services/core/java/com/android/server/am/PendingTempAllowlists.java
@@ -16,6 +16,8 @@
 
 package com.android.server.am;
 
+import static android.os.Process.INVALID_UID;
+
 import android.util.SparseArray;
 
 /** Allowlists of uids to temporarily bypass Power Save mode. */
@@ -31,29 +33,42 @@ final class PendingTempAllowlists {
     }
 
     void put(int uid, ActivityManagerService.PendingTempAllowlist value) {
-        mPendingTempAllowlist.put(uid, value);
+        synchronized (mPendingTempAllowlist) {
+            mPendingTempAllowlist.put(uid, value);
+        }
         mService.mAtmInternal.onUidAddedToPendingTempAllowlist(uid, value.tag);
     }
 
     void removeAt(int index) {
-        final int uid = mPendingTempAllowlist.keyAt(index);
-        mPendingTempAllowlist.removeAt(index);
+        int uid = INVALID_UID;
+        synchronized (mPendingTempAllowlist) {
+            uid = mPendingTempAllowlist.keyAt(index);
+            mPendingTempAllowlist.removeAt(index);
+        }
         mService.mAtmInternal.onUidRemovedFromPendingTempAllowlist(uid);
     }
 
     ActivityManagerService.PendingTempAllowlist get(int uid) {
-        return mPendingTempAllowlist.get(uid);
+        synchronized (mPendingTempAllowlist) {
+            return mPendingTempAllowlist.get(uid);
+        }
     }
 
     int size() {
-        return mPendingTempAllowlist.size();
+        synchronized (mPendingTempAllowlist) {
+            return mPendingTempAllowlist.size();
+        }
     }
 
     ActivityManagerService.PendingTempAllowlist valueAt(int index) {
-        return mPendingTempAllowlist.valueAt(index);
+        synchronized (mPendingTempAllowlist) {
+            return mPendingTempAllowlist.valueAt(index);
+        }
     }
 
     int indexOfKey(int key) {
-        return mPendingTempAllowlist.indexOfKey(key);
+        synchronized (mPendingTempAllowlist) {
+            return mPendingTempAllowlist.indexOfKey(key);
+        }
     }
 }