Merge "init: Fix a race condition in KillProcessGroup()"
diff --git a/init/service.cpp b/init/service.cpp
index 00aa4d1..ccc7191 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>
@@ -226,7 +227,7 @@
}
}
-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 @@
}
}
- 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 @@
// 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 @@
// 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 @@
return {};
}
- InterprocessFifo cgroups_activated;
-
- if (Result<void> 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<void> result = CheckConsole(); !result.ok()) {
return result;
@@ -661,10 +665,12 @@
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 @@
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 {};
diff --git a/init/service.h b/init/service.h
index e7211ec..54bf638 100644
--- a/init/service.h
+++ b/init/service.h
@@ -151,11 +151,12 @@
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_;
diff --git a/init/service_utils.cpp b/init/service_utils.cpp
index 16eab9e..15bf963 100644
--- a/init/service_utils.cpp
+++ b/init/service_utils.cpp
@@ -232,7 +232,7 @@
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 @@
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 5af779c..d4143aa 100644
--- a/init/service_utils.h
+++ b/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 @@
enum ServiceCode : uint8_t {
kActivatingCgroupsFailed,
kCgroupsActivated,
+ kSetSidFinished,
};
class Descriptor {
@@ -100,7 +102,7 @@
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);