From dcc378e53cf90ac8f3698dcaac79b81d56e283d7 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bvanassche@google.com>
Date: Thu, 3 Nov 2022 15:15:25 +0000
Subject: [PATCH] Revert "init: Fix a race condition in KillProcessGroup()"

This reverts commit d8ef6f84d4df1b0cb9aab942ea221020bde61c6a.

Reason for revert: b/256874349

Change-Id: I86a1e03a0d2979db1c54abd3e78c32591fda98a1
---
 init/service.cpp       | 38 +++-----------------------------------
 init/service.h         |  3 +--
 init/service_utils.cpp |  6 +-----
 3 files changed, 5 insertions(+), 42 deletions(-)

diff --git a/init/service.cpp b/init/service.cpp
index caa909558..331cd880d 100644
--- a/init/service.cpp
+++ b/init/service.cpp
@@ -507,7 +507,7 @@ void Service::ConfigureMemcg() {
 
 // Enters namespaces, sets environment variables, writes PID files and runs the service executable.
 void Service::RunService(const std::vector<Descriptor>& descriptors,
-                         InterprocessFifo cgroups_activated, InterprocessFifo setsid_finished) {
+                         InterprocessFifo cgroups_activated) {
     if (auto result = EnterNamespaces(namespaces_, name_, mount_namespace_); !result.ok()) {
         LOG(FATAL) << "Service '" << name_ << "' failed to set up namespaces: " << result.error();
     }
@@ -557,12 +557,6 @@ void Service::RunService(const std::vector<Descriptor>& descriptors,
     // priority. Aborts on failure.
     SetProcessAttributesAndCaps();
 
-    // If SetProcessAttributes() called setsid(), report this to the parent.
-    if (!proc_attr_.console.empty()) {
-        setsid_finished.Write(2);
-    }
-    setsid_finished.Close();
-
     if (!ExpandArgsAndExecv(args_, sigstop_)) {
         PLOG(ERROR) << "cannot execv('" << args_[0]
                     << "'). See the 'Debugging init' section of init's README.md for tips";
@@ -604,7 +598,7 @@ Result<void> Service::Start() {
         return {};
     }
 
-    InterprocessFifo cgroups_activated, setsid_finished;
+    InterprocessFifo cgroups_activated;
 
     if (Result<void> result = cgroups_activated.Initialize(); !result.ok()) {
         return result;
@@ -614,13 +608,6 @@ Result<void> Service::Start() {
         return result;
     }
 
-    // Only check proc_attr_.console after the CheckConsole() call.
-    if (!proc_attr_.console.empty()) {
-        if (Result<void> result = setsid_finished.Initialize(); !result.ok()) {
-            return result;
-        }
-    }
-
     struct stat sb;
     if (stat(args_[0].c_str(), &sb) == -1) {
         flags_ |= SVC_DISABLED;
@@ -674,12 +661,10 @@ Result<void> Service::Start() {
     if (pid == 0) {
         umask(077);
         cgroups_activated.CloseWriteFd();
-        setsid_finished.CloseReadFd();
-        RunService(descriptors, std::move(cgroups_activated), std::move(setsid_finished));
+        RunService(descriptors, std::move(cgroups_activated));
         _exit(127);
     } else {
         cgroups_activated.CloseReadFd();
-        setsid_finished.CloseWriteFd();
     }
 
     if (pid < 0) {
@@ -736,23 +721,6 @@ 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) == -1) {
-            return ErrnoError() << "setpgid failed";
-        }
-    } else {
-        // The Read() call below will return an error if the child is killed.
-        if (Result<uint8_t> result = setsid_finished.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";
-            }
-        }
-    }
-
     NotifyStateChange("running");
     reboot_on_failure.Disable();
     return {};
diff --git a/init/service.h b/init/service.h
index 10a0790fa..b2c9909ed 100644
--- a/init/service.h
+++ b/init/service.h
@@ -155,8 +155,7 @@ class Service {
     void ResetFlagsForStart();
     Result<void> CheckConsole();
     void ConfigureMemcg();
-    void RunService(const std::vector<Descriptor>& descriptors, InterprocessFifo cgroups_activated,
-                    InterprocessFifo setsid_finished);
+    void RunService(const std::vector<Descriptor>& descriptors, InterprocessFifo cgroups_activated);
     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 56a80b533..a14969e98 100644
--- a/init/service_utils.cpp
+++ b/init/service_utils.cpp
@@ -244,11 +244,7 @@ Result<void> SetProcessAttributes(const ProcessAttributes& attr) {
         setsid();
         OpenConsole(attr.console);
     } else {
-        // 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) {
+        if (setpgid(0, getpid()) == -1) {
             return ErrnoError() << "setpgid failed";
         }
         SetupStdio(attr.stdio_to_kmsg);