Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit efedd921 authored by Treehugger Robot's avatar Treehugger Robot Committed by Automerger Merge Worker
Browse files

Merge "init: Fix a race condition in KillProcessGroup()" am: 3fca6e72 am: adcfac01 

Original change: https://android-review.googlesource.com/c/platform/system/core/+/2302562



Change-Id: I26851cdeef767f59ffefe48bb5914e75b5f6bcb5
Signed-off-by: default avatarAutomerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
parents f1e8c9c0 adcfac01

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);