From 630f891af58dd7ccfd9f390198c2fb4788653965 Mon Sep 17 00:00:00 2001 From: Jing Ji Date: Thu, 23 Jul 2020 23:09:43 -0700 Subject: [PATCH] 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 --- .../java/com/android/server/am/ProcessList.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java index 2e628642dac59..c851a88b6e68e 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 @@ public final class ProcessList { 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); }