From f151a15ce9949b46c932321ec289b97ae883e45f Mon Sep 17 00:00:00 2001
From: Jing Ji <jji@google.com>
Date: Tue, 2 May 2023 10:26:48 -0700
Subject: [PATCH] Fix the race condition between creating and killing process
 group

Framework takes care of the process group creation for the processes
forked by webview and app zygote. There is a small window
between the fork and the process group creation, if the process is
deemed to be killed during that window, the process group creation
would fail and crash the system. Now add a lock around that.

Bug: 270103958
Test: Manual
Change-Id: Id602d9cd5243b7726d88e2fa40fd3d3431086a64
---
 .../com/android/server/am/ProcessList.java    | 28 +++++++++++-----
 .../com/android/server/am/ProcessRecord.java  | 32 +++++++++++++++++--
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/services/core/java/com/android/server/am/ProcessList.java b/services/core/java/com/android/server/am/ProcessList.java
index d7b22a8a2da02..c393213c5e85d 100644
--- a/services/core/java/com/android/server/am/ProcessList.java
+++ b/services/core/java/com/android/server/am/ProcessList.java
@@ -2300,6 +2300,8 @@ public final class ProcessList {
 
             final Process.ProcessStartResult startResult;
             boolean regularZygote = false;
+            app.mProcessGroupCreated = false;
+            app.mSkipProcessGroupCreation = false;
             if (hostingRecord.usesWebviewZygote()) {
                 startResult = startWebView(entryPoint,
                         app.processName, uid, uid, gids, runtimeFlags, mountExternal,
@@ -2328,18 +2330,28 @@ public final class ProcessList {
                         isTopApp, app.getDisabledCompatChanges(), pkgDataInfoMap,
                         allowlistedAppDataInfoMap, bindMountAppsData, bindMountAppStorageDirs,
                         new String[]{PROC_START_SEQ_IDENT + app.getStartSeq()});
+                // By now the process group should have been created by zygote.
+                app.mProcessGroupCreated = true;
             }
 
             if (!regularZygote) {
                 // webview and app zygote don't have the permission to create the nodes
-                final int res = Process.createProcessGroup(uid, startResult.pid);
-                if (res < 0) {
-                    if (res == -OsConstants.ESRCH) {
-                        Slog.e(ActivityManagerService.TAG, "Unable to create process group for "
-                            + app.processName + " (" + startResult.pid + ")");
-                    } else {
-                        throw new AssertionError("Unable to create process group for "
-                            + app.processName + " (" + startResult.pid + ")");
+                synchronized (app) {
+                    if (!app.mSkipProcessGroupCreation) {
+                        // If we're not told to skip the process group creation, go create it.
+                        final int res = Process.createProcessGroup(uid, startResult.pid);
+                        if (res < 0) {
+                            if (res == -OsConstants.ESRCH) {
+                                Slog.e(ActivityManagerService.TAG,
+                                        "Unable to create process group for "
+                                        + app.processName + " (" + startResult.pid + ")");
+                            } else {
+                                throw new AssertionError("Unable to create process group for "
+                                    + app.processName + " (" + startResult.pid + ")");
+                            }
+                        } else {
+                            app.mProcessGroupCreated = true;
+                        }
                     }
                 }
             }
diff --git a/services/core/java/com/android/server/am/ProcessRecord.java b/services/core/java/com/android/server/am/ProcessRecord.java
index 0417b8cfa2e2e..4ec813ecd81cd 100644
--- a/services/core/java/com/android/server/am/ProcessRecord.java
+++ b/services/core/java/com/android/server/am/ProcessRecord.java
@@ -424,6 +424,16 @@ class ProcessRecord implements WindowProcessListener {
      */
     Runnable mSuccessorStartRunnable;
 
+    /**
+     * Whether or not the process group of this process has been created.
+     */
+    volatile boolean mProcessGroupCreated;
+
+    /**
+     * Whether or not we should skip the process group creation.
+     */
+    volatile boolean mSkipProcessGroupCreation;
+
     void setStartParams(int startUid, HostingRecord hostingRecord, String seInfo,
             long startUptime, long startElapsedTime) {
         this.mStartUid = startUid;
@@ -1192,8 +1202,26 @@ class ProcessRecord implements WindowProcessListener {
                 EventLog.writeEvent(EventLogTags.AM_KILL,
                         userId, mPid, processName, mState.getSetAdj(), reason);
                 Process.killProcessQuiet(mPid);
-                if (!asyncKPG) Process.sendSignalToProcessGroup(uid, mPid, OsConstants.SIGKILL);
-                ProcessList.killProcessGroup(uid, mPid);
+                final boolean killProcessGroup;
+                if (mHostingRecord != null
+                        && (mHostingRecord.usesWebviewZygote() || mHostingRecord.usesAppZygote())) {
+                    synchronized (ProcessRecord.this) {
+                        killProcessGroup = mProcessGroupCreated;
+                        if (!killProcessGroup) {
+                            // The process group hasn't been created, request to skip it.
+                            mSkipProcessGroupCreation = true;
+                        }
+                    }
+                } else {
+                    killProcessGroup = true;
+                }
+                if (killProcessGroup) {
+                    if (asyncKPG) {
+                        ProcessList.killProcessGroup(uid, mPid);
+                    } else {
+                        Process.sendSignalToProcessGroup(uid, mPid, OsConstants.SIGKILL);
+                    }
+                }
             } else {
                 mPendingStart = false;
             }