From 41d2be0f0f824c002090c2fb663a0a9cdafca034 Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Fri, 9 Dec 2016 09:06:12 -0700 Subject: [PATCH] Fix race condition bug related to freezing apps. Consider the following situation: 1. Package is frozen. 2. We try forking the app while frozen, causing a ProcessRecord with PID 0 to be recorded in mProcessNames. As a result of the failed fork, removeProcessLocked() tears down that ProcessRecord, but a special case records it into mRemovedProcesses. 3. Package is unfrozen. 4. We try forking the app, and this time it proceeds normally now that we're unfrozen. The new valid ProcessRecord is recorded in mProcessNames. 5. activityIdleInternalLocked() triggers a clean-up pass of mRemovedProcesses. trimApplications() ends up cleaning up the stale reference from (2) above *by hash key* and not *by reference*, which causes us to remove the new valid ProcessRecord. This results in the valid ProcessRecord in (4) becoming an orphaned PID, which starts a chain reaction of havoc that ensues. This issue is fixed by checking the expected ProcessRecord by value before actually removing it, thus preventing orphaned PIDs. Test: builds, boots, over 600 installs without orphaned PIDs Bug: 28395549 Change-Id: I5ea1b31c3fd374ea7f5cc40ff35bb9195d9f3e2b --- .../server/am/ActivityManagerService.java | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/services/core/java/com/android/server/am/ActivityManagerService.java b/services/core/java/com/android/server/am/ActivityManagerService.java index 91d9ac7f51dd6..7c9497df3d89c 100644 --- a/services/core/java/com/android/server/am/ActivityManagerService.java +++ b/services/core/java/com/android/server/am/ActivityManagerService.java @@ -6278,8 +6278,19 @@ public final class ActivityManagerService extends ActivityManagerNative } private final ProcessRecord removeProcessNameLocked(final String name, final int uid) { - ProcessRecord old = mProcessNames.remove(name, uid); - if (old != null) { + return removeProcessNameLocked(name, uid, null); + } + + private final ProcessRecord removeProcessNameLocked(final String name, final int uid, + final ProcessRecord expecting) { + ProcessRecord old = mProcessNames.get(name, uid); + // Only actually remove when the currently recorded value matches the + // record that we expected; if it doesn't match then we raced with a + // newly created process and we don't want to destroy the new one. + if ((expecting == null) || (old == expecting)) { + mProcessNames.remove(name, uid); + } + if (old != null && old.uidRecord != null) { old.uidRecord.numProcs--; if (old.uidRecord.numProcs == 0) { // No more processes using this uid, tell clients it is gone. @@ -17043,7 +17054,7 @@ public final class ActivityManagerService extends ActivityManagerNative if (DEBUG_PROCESSES || DEBUG_CLEANUP) Slog.v(TAG_CLEANUP, "Removing non-persistent process during cleanup: " + app); if (!replacingPid) { - removeProcessNameLocked(app.processName, app.uid); + removeProcessNameLocked(app.processName, app.uid, app); } if (mHeavyWeightProcess == app) { mHandler.sendMessage(mHandler.obtainMessage(CANCEL_HEAVY_NOTIFICATION_MSG,