Merge "init: Fix a race condition in KillProcessGroup()" (3fca6e72) · Commits · e / os / android_system_core

init/service.cpp

+45 −10

Original line number	Diff line number	Diff line
		@@ -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;

		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 @@ 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

+3 −2

Original line number	Diff line number	Diff line
		@@ -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

+7 −1

Original line number	Diff line number	Diff line
		@@ -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

+3 −1

Original line number	Diff line number	Diff line
		@@ -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);