Skip to content

Commit

Permalink
init: move killing of process groups to libprocessgroup
Browse files Browse the repository at this point in the history
libprocessgroup kills the cgroup associated with a given pid and uid,
but not the POSIX process group associated with it.  This means that
to kill both, two of the same signals must be sent, which may cause
some issues.

This change kills all POSIX process groups whose group leaders are
found within a cgroup.  It only then kills processes in the cgroup
that are not part of the POSIX process groups that have been killed.

Bug: 37853905
Bug: 62418791
Test: Boot, kill zygote, reboot
Change-Id: Id1d96935745899b4c454c36c351ec16a0b1d3827
  • Loading branch information
Tom Cherry committed Jun 7, 2017
1 parent d105aa8 commit 70a5ed4
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 11 deletions.
11 changes: 0 additions & 11 deletions init/service.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -209,17 +209,6 @@ void Service::NotifyStateChange(const std::string& new_state) const {
}

void Service::KillProcessGroup(int signal) {
// We ignore reporting errors of ESRCH as this commonly happens in the below case,
// 1) Terminate() is called, which sends SIGTERM to the process
// 2) The process successfully exits
// 3) ReapOneProcess() is called, which calls waitpid(-1, ...) which removes the pid entry.
// 4) Reap() is called, which sends SIGKILL, but the pid no longer exists.
// TODO: sigaction for SIGCHLD reports the pid of the exiting process,
// we should do this kill with that pid first before calling waitpid().
if (kill(-pid_, signal) == -1 && errno != ESRCH) {
PLOG(ERROR) << "kill(" << pid_ << ", " << signal << ") failed";
}

// If we've already seen a successful result from killProcessGroup*(), then we have removed
// the cgroup already and calling these functions a second time will simply result in an error.
// This is true regardless of which signal was sent.
Expand Down
40 changes: 40 additions & 0 deletions libprocessgroup/processgroup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#include <chrono>
#include <memory>
#include <mutex>
#include <set>
#include <thread>

#include <android-base/logging.h>
Expand Down Expand Up @@ -258,6 +260,12 @@ static int doKillProcessGroupOnce(uid_t uid, int initialPid, int signal) {
return -errno;
}

// We separate all of the pids in the cgroup into those pids that are also the leaders of
// process groups (stored in the pgids set) and those that are not (stored in the pids set).
std::set<pid_t> pgids;
pgids.emplace(initialPid);
std::set<pid_t> pids;

int ret;
pid_t pid;
int processes = 0;
Expand All @@ -269,8 +277,40 @@ static int doKillProcessGroupOnce(uid_t uid, int initialPid, int signal) {
LOG(WARNING) << "Yikes, we've been told to kill pid 0! How about we don't do that?";
continue;
}
pid_t pgid = getpgid(pid);
if (pgid == -1) PLOG(ERROR) << "getpgid(" << pid << ") failed";
if (pgid == pid) {
pgids.emplace(pid);
} else {
pids.emplace(pid);
}
}

// Erase all pids that will be killed when we kill the process groups.
for (auto it = pids.begin(); it != pids.end();) {
pid_t pgid = getpgid(pid);
if (pgids.count(pgid) == 1) {
it = pids.erase(it);
} else {
++it;
}
}

// Kill all process groups.
for (const auto pgid : pgids) {
LOG(VERBOSE) << "Killing process group " << -pgid << " in uid " << uid
<< " as part of process cgroup " << initialPid;

if (kill(-pgid, signal) == -1) {
PLOG(WARNING) << "kill(" << -pgid << ", " << signal << ") failed";
}
}

// Kill remaining pids.
for (const auto pid : pids) {
LOG(VERBOSE) << "Killing pid " << pid << " in uid " << uid << " as part of process cgroup "
<< initialPid;

if (kill(pid, signal) == -1) {
PLOG(WARNING) << "kill(" << pid << ", " << signal << ") failed";
}
Expand Down

0 comments on commit 70a5ed4

Please sign in to comment.