Merge "init: Fix a race condition in KillProcessGroup()"

init/service.cpp

@@ -16,6 +16,7 @@
 
 #include "service.h"
 
+#include <errno.h>
 #include <fcntl.h>
 #include <inttypes.h>
 #include <linux/securebits.h>
@@ -226,7 +227,7 @@ void Service::KillProcessGroup(int signal, bool report_oneshot) {
     }
 }
 
-void Service::SetProcessAttributesAndCaps() {
+void Service::SetProcessAttributesAndCaps(InterprocessFifo setsid_finished) {
     // Keep capabilites on uid change.
     if (capabilities_ && proc_attr_.uid) {
         // If Android is running in a container, some securebits might already
@@ -241,7 +242,7 @@ void Service::SetProcessAttributesAndCaps() {
         }
     }
 
-    if (auto result = SetProcessAttributes(proc_attr_); !result.ok()) {
+    if (auto result = SetProcessAttributes(proc_attr_, std::move(setsid_finished)); !result.ok()) {
         LOG(FATAL) << "cannot set attribute for " << name_ << ": " << result.error();
     }
 
@@ -507,7 +508,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 cgroups_activated, InterprocessFifo setsid_finished) {
     if (auto result = EnterNamespaces(namespaces_, name_, mount_namespace_); !result.ok()) {
         LOG(FATAL) << "Service '" << name_ << "' failed to set up namespaces: " << result.error();
     }
@@ -555,7 +556,7 @@ void Service::RunService(const std::vector<Descriptor>& descriptors,
 
     // As requested, set our gid, supplemental gids, uid, context, and
     // priority. Aborts on failure.
-    SetProcessAttributesAndCaps();
+    SetProcessAttributesAndCaps(std::move(setsid_finished));
 
     if (!ExpandArgsAndExecv(args_, sigstop_)) {
         PLOG(ERROR) << "cannot execv('" << args_[0]
@@ -598,11 +599,14 @@ Result<void> Service::Start() {
         return {};
     }
 
-    InterprocessFifo cgroups_activated;
+    // cgroups_activated is used for communication from the parent to the child
-
+    // while setsid_finished is used for communication from the child process to
-    if (Result<void> result = cgroups_activated.Initialize(); !result.ok()) {
+    // the parent process. These two communication channels are separate because
-        return result;
+    // combining these into a single communication channel would introduce a
-    }
+    // race between the Write() calls by the parent and by the child.
+    InterprocessFifo cgroups_activated, setsid_finished;
+    OR_RETURN(cgroups_activated.Initialize());
+    OR_RETURN(setsid_finished.Initialize());
 
     if (Result<void> result = CheckConsole(); !result.ok()) {
         return result;
@@ -661,10 +665,12 @@ Result<void> Service::Start() {
     if (pid == 0) {
         umask(077);
         cgroups_activated.CloseWriteFd();
-        RunService(descriptors, std::move(cgroups_activated));
+        setsid_finished.CloseReadFd();
+        RunService(descriptors, std::move(cgroups_activated), std::move(setsid_finished));
         _exit(127);
     } else {
         cgroups_activated.CloseReadFd();
+        setsid_finished.CloseWriteFd();
     }
 
     if (pid < 0) {
@@ -721,6 +727,35 @@ Result<void> Service::Start() {
         return Error() << "Sending cgroups activated notification failed: " << result.error();
     }
 
+    cgroups_activated.Close();
+
+    // Call setpgid() from the parent process to make sure that this call has
+    // finished before the parent process calls kill(-pgid, ...).
+    if (!RequiresConsole(proc_attr_)) {
+        if (setpgid(pid, pid) < 0) {
+            switch (errno) {
+                case EACCES:  // Child has already performed setpgid() followed by 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 = setsid_finished.Read();
+            !result.ok() || *result != kSetSidFinished) {
+            if (!result.ok()) {
+                return Error() << "Waiting for setsid() failed: " << result.error();
+            } else {
+                return Error() << "Waiting for setsid() failed: " << static_cast<uint32_t>(*result)
+                               << " <> " << static_cast<uint32_t>(kSetSidFinished);
+            }
+        }
+    }
+
+    setsid_finished.Close();
+
     NotifyStateChange("running");
     reboot_on_failure.Disable();
     return {};

init/service.h

@@ -151,11 +151,12 @@ class Service {
     void NotifyStateChange(const std::string& new_state) const;
     void StopOrReset(int how);
     void KillProcessGroup(int signal, bool report_oneshot = false);
-    void SetProcessAttributesAndCaps();
+    void SetProcessAttributesAndCaps(InterprocessFifo setsid_finished);
     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 cgroups_activated,
+                    InterprocessFifo setsid_finished);
     void SetMountNamespace();
     static unsigned long next_start_order_;
     static bool is_exec_service_running_;

init/service_utils.cpp

@@ -232,7 +232,7 @@ Result<void> EnterNamespaces(const NamespaceInfo& info, const std::string& name,
     return {};
 }
 
-Result<void> SetProcessAttributes(const ProcessAttributes& attr) {
+Result<void> SetProcessAttributes(const ProcessAttributes& attr, InterprocessFifo setsid_finished) {
     if (attr.ioprio_class != IoSchedClass_NONE) {
         if (android_set_ioprio(getpid(), attr.ioprio_class, attr.ioprio_pri)) {
             PLOG(ERROR) << "failed to set pid " << getpid() << " ioprio=" << attr.ioprio_class
@@ -242,8 +242,14 @@ Result<void> SetProcessAttributes(const ProcessAttributes& attr) {
 
     if (RequiresConsole(attr)) {
         setsid();
+        setsid_finished.Write(kSetSidFinished);
+        setsid_finished.Close();
         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) {
             return ErrnoError() << "setpgid failed";
         }

init/service_utils.h

@@ -26,6 +26,7 @@
 #include <android-base/unique_fd.h>
 #include <cutils/iosched_policy.h>
 
+#include "interprocess_fifo.h"
 #include "mount_namespace.h"
 #include "result.h"
 
@@ -36,6 +37,7 @@ namespace init {
 enum ServiceCode : uint8_t {
     kActivatingCgroupsFailed,
     kCgroupsActivated,
+    kSetSidFinished,
 };
 
 class Descriptor {
@@ -100,7 +102,7 @@ inline bool RequiresConsole(const ProcessAttributes& attr) {
     return !attr.console.empty();
 }
 
-Result<void> SetProcessAttributes(const ProcessAttributes& attr);
+Result<void> SetProcessAttributes(const ProcessAttributes& attr, InterprocessFifo setsid_finished);
 
 Result<void> WritePidToFiles(std::vector<std::string>* files);