From 01e6669c66213518d44544ae36940f5347c760f7 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Mon, 14 Nov 2022 16:45:47 -0800 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 Change-Id: Ieb9e6908df725447e3695ed66bb8bd30e4e38aa9 Signed-off-by: Bart Van Assche --- init/service.cpp | 55 ++++++++++++++++++++++++++++++++++-------- init/service.h | 5 ++-- init/service_utils.cpp | 8 +++++- init/service_utils.h | 4 ++- 4 files changed, 58 insertions(+), 14 deletions(-) diff --git a/init/service.cpp b/init/service.cpp index 00aa4d108..ccc719168 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -16,6 +16,7 @@ #include "service.h" +#include #include #include #include @@ -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& 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& 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 Service::Start() { return {}; } - InterprocessFifo cgroups_activated; - - if (Result result = cgroups_activated.Initialize(); !result.ok()) { - return result; - } + // cgroups_activated is used for communication from the parent to the child + // while setsid_finished is used for communication from the child process to + // the parent process. These two communication channels are separate because + // 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 result = CheckConsole(); !result.ok()) { return result; @@ -661,10 +665,12 @@ Result 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 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 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(*result) + << " <> " << static_cast(kSetSidFinished); + } + } + } + + setsid_finished.Close(); + NotifyStateChange("running"); reboot_on_failure.Disable(); return {}; diff --git a/init/service.h b/init/service.h index e7211ec7b..54bf63871 100644 --- a/init/service.h +++ b/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 CheckConsole(); void ConfigureMemcg(); - void RunService(const std::vector& descriptors, InterprocessFifo cgroups_activated); + void RunService(const std::vector& descriptors, InterprocessFifo cgroups_activated, + InterprocessFifo setsid_finished); 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 16eab9eb7..15bf96361 100644 --- a/init/service_utils.cpp +++ b/init/service_utils.cpp @@ -232,7 +232,7 @@ Result EnterNamespaces(const NamespaceInfo& info, const std::string& name, return {}; } -Result SetProcessAttributes(const ProcessAttributes& attr) { +Result 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 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"; } diff --git a/init/service_utils.h b/init/service_utils.h index 5af779c42..d4143aa0b 100644 --- a/init/service_utils.h +++ b/init/service_utils.h @@ -26,6 +26,7 @@ #include #include +#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 SetProcessAttributes(const ProcessAttributes& attr); +Result SetProcessAttributes(const ProcessAttributes& attr, InterprocessFifo setsid_finished); Result WritePidToFiles(std::vector* files);