From 15e5ecdcd7ea69e63387346bd235fbeb3e3597df Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@google.com>
Date: Wed, 19 Oct 2022 12:56:10 -0700
Subject: [PATCH] init: Fix a race condition in KillProcessGroup()

Multiple tests in CtsInitTestCases, e.g. RebootTest#StopServicesSIGKILL,
can trigger the following race condition:
* A service is started. This involves calling fork() and also to call
  RunService() in the child process. RunService() calls setpgid().
* Service::Stop() is called and calls KillProcessGroup().
  KillProcessGroup() calls kill(-pgid, SIGKILL) before the child process
  has called setpgid(). pgid is the process ID of the child process. The
  kill() call fails because setpgid() has not yet been called.

Fix this race condition by adding a setpgid() call in the parent process
and by waiting from the parent until the child has called setsid() if a
console is attached.

Bug: 213617178
Test: Cuttlefish + atest 'CtsInitTestCases'
Change-Id: I6931cd579e607c247b4f79a5b375455ca3d52e29
Signed-off-by: Bart Van Assche <bvanassche@google.com>
---
 init/service.cpp       | 36 ++++++++++++++++++++++++++++++++----
 init/service.h         |  2 +-
 init/service_utils.cpp |  8 ++++++--
 init/service_utils.h   |  5 +++++
 4 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/init/service.cpp b/init/service.cpp
index c260c0713..85ac2fc90 100644
--- a/init/service.cpp
+++ b/init/service.cpp
@@ -16,6 +16,7 @@
 
 #include "service.h"
 
+#include <errno.h>
 #include <fcntl.h>
 #include <inttypes.h>
 #include <linux/securebits.h>
@@ -532,7 +533,6 @@ void Service::RunService(const std::vector<Descriptor>& descriptors, Interproces
     if (!byte.ok()) {
         LOG(ERROR) << name_ << ": failed to read from notification channel: " << byte.error();
     }
-    fifo.Close();
     if (!*byte) {
         LOG(FATAL) << "Service '" << name_  << "' failed to start due to a fatal error";
         _exit(EXIT_FAILURE);
@@ -556,6 +556,12 @@ void Service::RunService(const std::vector<Descriptor>& descriptors, Interproces
     // priority. Aborts on failure.
     SetProcessAttributesAndCaps();
 
+    // If SetProcessAttributes() called setsid(), report this to the parent.
+    if (RequiresConsole(proc_attr_)) {
+        fifo.Write(2);
+    }
+    fifo.Close();
+
     if (!ExpandArgsAndExecv(args_, sigstop_)) {
         PLOG(ERROR) << "cannot execv('" << args_[0]
                     << "'). See the 'Debugging init' section of init's README.md for tips";
@@ -656,11 +662,8 @@ Result<void> Service::Start() {
 
     if (pid == 0) {
         umask(077);
-        fifo.CloseWriteFd();
         RunService(descriptors, std::move(fifo));
         _exit(127);
-    } else {
-        fifo.CloseReadFd();
     }
 
     if (pid < 0) {
@@ -717,6 +720,31 @@ Result<void> Service::Start() {
         return Error() << "Sending cgroups activated notification failed: " << result.error();
     }
 
+    // Call setpgid() from the parent process to make sure that this call has
+    // finished before the parent process calls kill(-pgid, ...).
+    if (proc_attr_.console.empty()) {
+        if (setpgid(pid, pid) < 0) {
+            switch (errno) {
+                case EACCES:   // Child has already performed execve().
+                case ESRCH:    // Child process no longer exists.
+                    break;
+                default:
+                    PLOG(ERROR) << "setpgid() from parent failed";
+            }
+        }
+    } else {
+        // The Read() call below will return an error if the child is killed.
+        if (Result<uint8_t> result = fifo.Read(); !result.ok() || *result != 2) {
+            if (!result.ok()) {
+                return Error() << "Waiting for setsid() failed: " << result.error();
+            } else {
+                return Error() << "Waiting for setsid() failed: " << *result << " <> 2";
+            }
+        }
+    }
+
+    fifo.Close();
+
     NotifyStateChange("running");
     reboot_on_failure.Disable();
     return {};
diff --git a/init/service.h b/init/service.h
index b2c9909ed..2c2778dd6 100644
--- a/init/service.h
+++ b/init/service.h
@@ -155,7 +155,7 @@ class Service {
     void ResetFlagsForStart();
     Result<void> CheckConsole();
     void ConfigureMemcg();
-    void RunService(const std::vector<Descriptor>& descriptors, InterprocessFifo cgroups_activated);
+    void RunService(const std::vector<Descriptor>& descriptors, InterprocessFifo fifo);
     void SetMountNamespace();
     static unsigned long next_start_order_;
     static bool is_exec_service_running_;
diff --git a/init/service_utils.cpp b/init/service_utils.cpp
index a14969e98..9585d056e 100644
--- a/init/service_utils.cpp
+++ b/init/service_utils.cpp
@@ -240,11 +240,15 @@ Result<void> SetProcessAttributes(const ProcessAttributes& attr) {
         }
     }
 
-    if (!attr.console.empty()) {
+    if (RequiresConsole(attr)) {
         setsid();
         OpenConsole(attr.console);
     } else {
-        if (setpgid(0, getpid()) == -1) {
+        // Without PID namespaces, this call duplicates the setpgid() call from
+        // the parent process. With PID namespaces, this setpgid() call sets the
+        // process group ID for a child of the init process in the PID
+        // namespace.
+        if (setpgid(0, 0) == -1) {
             return ErrnoError() << "setpgid failed";
         }
         SetupStdio(attr.stdio_to_kmsg);
diff --git a/init/service_utils.h b/init/service_utils.h
index 65a2012ff..c66f2b48a 100644
--- a/init/service_utils.h
+++ b/init/service_utils.h
@@ -89,6 +89,11 @@ struct ProcessAttributes {
     int priority;
     bool stdio_to_kmsg;
 };
+
+inline bool RequiresConsole(const ProcessAttributes& attr) {
+    return !attr.console.empty();
+}
+
 Result<void> SetProcessAttributes(const ProcessAttributes& attr);
 
 Result<void> WritePidToFiles(std::vector<std::string>* files);