commit 630f891af58dd7ccfd9f390198c2fb4788653965
author Jing Ji <jji@google.com> Thu Jul 23 23:09:43 2020 -0700
committer Jing Ji <jji@google.com> Tue Jul 28 00:32:30 2020 +0000
tree 9b0a89cc528c6261781ad46bb62c4ea7ee0be526
parent 53c4fc325e6ec3a0aba2198bbf4050cf004f9f40

Fix race condition in UidRecord cleanup

It's a situation that:
1) An app process is killed;
2) A new instance of this process is created right after it
3) The bindeDied handling happens after 2)

During 2), while creating the new ProcessRecord, the old ProcessRecord
will be removed and this makes the total num of processes in this
UidRecord to be 0, then the new ProcessRecord will bump that number
by 1, in the meanwhile the new ProcessRecord will be kept in the
process name map,
During 3), while doing the cleanup for old ProcessRecord, it tries to
remove itself from the process name map, which actually has the
new ProcessRecord, so the total num of processes in the UidRecord
will become 0.

Now the fix is to make sure we are cleaning up the right ProcessRecord,
so in step 3) we will see it's already cleaned, thus we won't mess up
with the UidRecord.

Bug: 161466248
Test: see b/161466248#comment17 for detailed test steps
Change-Id: I2685e3f0162098b9b95f6b248b67aeb498e50c8e

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

diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java
index 2e62864..c851a88 100644
--- a/services/core/java/com/android/server/am/ProcessList.java
+++ b/services/core/java/com/android/server/am/ProcessList.java
@@ -2908,25 +2908,26 @@
         if ((expecting == null) || (old == expecting)) {
             mProcessNames.remove(name, uid);
         }
-        if (old != null && old.uidRecord != null) {
-            old.uidRecord.numProcs--;
-            old.uidRecord.procRecords.remove(old);
-            if (old.uidRecord.numProcs == 0) {
+        final ProcessRecord record = expecting != null ? expecting : old;
+        if (record != null && record.uidRecord != null) {
+            final UidRecord uidRecord = record.uidRecord;
+            uidRecord.numProcs--;
+            uidRecord.procRecords.remove(record);
+            if (uidRecord.numProcs == 0) {
                 // No more processes using this uid, tell clients it is gone.
                 if (DEBUG_UID_OBSERVERS) Slog.i(TAG_UID_OBSERVERS,
-                        "No more processes in " + old.uidRecord);
-                mService.enqueueUidChangeLocked(old.uidRecord, -1, UidRecord.CHANGE_GONE);
+                        "No more processes in " + uidRecord);
+                mService.enqueueUidChangeLocked(uidRecord, -1, UidRecord.CHANGE_GONE);
                 EventLogTags.writeAmUidStopped(uid);
                 mActiveUids.remove(uid);
                 mService.noteUidProcessState(uid, ActivityManager.PROCESS_STATE_NONEXISTENT,
                         ActivityManager.PROCESS_CAPABILITY_NONE);
             }
-            old.uidRecord = null;
+            record.uidRecord = null;
         }
         mIsolatedProcesses.remove(uid);
         mGlobalIsolatedUids.freeIsolatedUidLocked(uid);
         // Remove the (expected) ProcessRecord from the app zygote
-        final ProcessRecord record = expecting != null ? expecting : old;
         if (record != null && record.appZygote) {
             removeProcessFromAppZygoteLocked(record);
         }