From 599d979126a0c60f5cc2955aedc58431f7659270 Mon Sep 17 00:00:00 2001 From: "T.J. Mercier" Date: Thu, 26 Oct 2023 20:46:21 +0000 Subject: [PATCH] libprocessgroup: Remove max_processes from KillProcessGroup API The max_processes calculation is incorrect for KillProcessGroup because the set of processes in cgroup.procs can differ between the multiple reads in the implementation. Luckily the exact value isn't very important because it's just logged. Remove max_processes from the API and remove the warning about the new behavior in Android 11. Note that we still always LOG(INFO) that any cgroup is being killed. Bug: 301871933 Change-Id: I8e449f5089d4a48dbc1797b6d979539e87026f43 --- init/service.cpp | 19 +++++-------------- init/service.h | 2 +- .../include/processgroup/processgroup.h | 7 ++----- libprocessgroup/processgroup.cpp | 18 +++++------------- 4 files changed, 13 insertions(+), 33 deletions(-) diff --git a/init/service.cpp b/init/service.cpp index 5e900ee4d..208745257 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -194,7 +194,7 @@ void Service::NotifyStateChange(const std::string& new_state) const { } } -void Service::KillProcessGroup(int signal, bool report_oneshot) { +void Service::KillProcessGroup(int signal) { // 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. @@ -202,20 +202,11 @@ void Service::KillProcessGroup(int signal, bool report_oneshot) { if (!process_cgroup_empty_) { LOG(INFO) << "Sending signal " << signal << " to service '" << name_ << "' (pid " << pid_ << ") process group..."; - int max_processes = 0; int r; if (signal == SIGTERM) { - r = killProcessGroupOnce(uid(), pid_, signal, &max_processes); + r = killProcessGroupOnce(uid(), pid_, signal); } else { - r = killProcessGroup(uid(), pid_, signal, &max_processes); - } - - if (report_oneshot && max_processes > 0) { - LOG(WARNING) - << "Killed " << max_processes - << " additional processes from a oneshot process group for service '" << name_ - << "'. This is new behavior, previously child processes would not be killed in " - "this case."; + r = killProcessGroup(uid(), pid_, signal); } if (r == 0) process_cgroup_empty_ = true; @@ -265,7 +256,7 @@ void Service::SetProcessAttributesAndCaps(InterprocessFifo setsid_finished) { void Service::Reap(const siginfo_t& siginfo) { if (!(flags_ & SVC_ONESHOT) || (flags_ & SVC_RESTART)) { - KillProcessGroup(SIGKILL, false); + KillProcessGroup(SIGKILL); } else { // Legacy behavior from ~2007 until Android R: this else branch did not exist and we did not // kill the process group in this case. @@ -273,7 +264,7 @@ void Service::Reap(const siginfo_t& siginfo) { // The new behavior in Android R is to kill these process groups in all cases. The // 'true' parameter instructions KillProcessGroup() to report a warning message where it // detects a difference in behavior has occurred. - KillProcessGroup(SIGKILL, true); + KillProcessGroup(SIGKILL); } } diff --git a/init/service.h b/init/service.h index 9f09cefb8..35521b207 100644 --- a/init/service.h +++ b/init/service.h @@ -160,7 +160,7 @@ class Service { private: void NotifyStateChange(const std::string& new_state) const; void StopOrReset(int how); - void KillProcessGroup(int signal, bool report_oneshot = false); + void KillProcessGroup(int signal); void SetProcessAttributesAndCaps(InterprocessFifo setsid_finished); void ResetFlagsForStart(); Result CheckConsole(); diff --git a/libprocessgroup/include/processgroup/processgroup.h b/libprocessgroup/include/processgroup/processgroup.h index dbaeb9397..5c43224ab 100644 --- a/libprocessgroup/include/processgroup/processgroup.h +++ b/libprocessgroup/include/processgroup/processgroup.h @@ -67,14 +67,11 @@ void DropTaskProfilesResourceCaching(); // Return 0 and removes the cgroup if there are no longer any processes in it. // Returns -1 in the case of an error occurring or if there are processes still running // even after retrying for up to 200ms. -// If max_processes is not nullptr, it returns the maximum number of processes seen in the cgroup -// during the killing process. Note that this can be 0 if all processes from the process group have -// already been terminated. -int killProcessGroup(uid_t uid, int initialPid, int signal, int* max_processes = nullptr); +int killProcessGroup(uid_t uid, int initialPid, int signal); // Returns the same as killProcessGroup(), however it does not retry, which means // that it only returns 0 in the case that the cgroup exists and it contains no processes. -int killProcessGroupOnce(uid_t uid, int initialPid, int signal, int* max_processes = nullptr); +int killProcessGroupOnce(uid_t uid, int initialPid, int signal); // Sends the provided signal to all members of a process group, but does not wait for processes to // exit, or for the cgroup to be removed. Callers should also ensure that killProcessGroup is called diff --git a/libprocessgroup/processgroup.cpp b/libprocessgroup/processgroup.cpp index cc2565fad..3fa43f8ad 100644 --- a/libprocessgroup/processgroup.cpp +++ b/libprocessgroup/processgroup.cpp @@ -455,8 +455,7 @@ static int DoKillProcessGroupOnce(const char* cgroup, uid_t uid, int initialPid, return (!fd || feof(fd.get())) ? processes : -1; } -static int KillProcessGroup(uid_t uid, int initialPid, int signal, int retries, - int* max_processes) { +static int KillProcessGroup(uid_t uid, int initialPid, int signal, int retries) { CHECK_GE(uid, 0); CHECK_GT(initialPid, 0); @@ -468,16 +467,9 @@ static int KillProcessGroup(uid_t uid, int initialPid, int signal, int retries, std::chrono::steady_clock::time_point start = std::chrono::steady_clock::now(); - if (max_processes != nullptr) { - *max_processes = 0; - } - int retry = retries; int processes; while ((processes = DoKillProcessGroupOnce(cgroup, uid, initialPid, signal)) > 0) { - if (max_processes != nullptr && processes > *max_processes) { - *max_processes = processes; - } LOG(VERBOSE) << "Killed " << processes << " processes for processgroup " << initialPid; if (!CgroupsAvailable()) { // makes no sense to retry, because there are no cgroup_procs file @@ -540,12 +532,12 @@ static int KillProcessGroup(uid_t uid, int initialPid, int signal, int retries, } } -int killProcessGroup(uid_t uid, int initialPid, int signal, int* max_processes) { - return KillProcessGroup(uid, initialPid, signal, 40 /*retries*/, max_processes); +int killProcessGroup(uid_t uid, int initialPid, int signal) { + return KillProcessGroup(uid, initialPid, signal, 40 /*retries*/); } -int killProcessGroupOnce(uid_t uid, int initialPid, int signal, int* max_processes) { - return KillProcessGroup(uid, initialPid, signal, 0 /*retries*/, max_processes); +int killProcessGroupOnce(uid_t uid, int initialPid, int signal) { + return KillProcessGroup(uid, initialPid, signal, 0 /*retries*/); } int sendSignalToProcessGroup(uid_t uid, int initialPid, int signal) {