From 07b38ba51e1b84c560cb745f93c1d3bb91d1d066 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 3 Oct 2017 12:05:24 +0100 Subject: [PATCH 1/3] Revert "tree-wide: use pid_is_valid() at more places" This reverts commit ee043777be58251e7441b4f04594e9e3792d7fb2. It broke almost everywhere it touched. The places that handn't been converted, were mostly followed by special handling for the invalid PID `0`. That explains why they tested for `pid < 0` instead of `pid <= 0`. I think that one was the first commit I reviewed, heh. --- src/basic/process-util.c | 8 ++++---- src/core/dbus-manager.c | 2 +- src/login/logind-dbus.c | 4 ++-- src/machine/machined-dbus.c | 2 +- src/systemctl/systemctl.c | 6 ++---- 5 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/basic/process-util.c b/src/basic/process-util.c index ab661a8f16..44ebf49f90 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -475,7 +475,7 @@ static int get_process_id(pid_t pid, const char *field, uid_t *uid) { assert(field); assert(uid); - if (!pid_is_valid(pid)) + if (pid < 0) return -EINVAL; p = procfs_file_alloca(pid, "status"); @@ -787,7 +787,7 @@ int getenv_for_pid(pid_t pid, const char *field, char **_value) { bool pid_is_unwaited(pid_t pid) { /* Checks whether a PID is still valid at all, including a zombie */ - if (!pid_is_valid(pid)) + if (pid < 0) return false; if (pid <= 1) /* If we or PID 1 would be dead and have been waited for, this code would not be running */ @@ -807,7 +807,7 @@ bool pid_is_alive(pid_t pid) { /* Checks whether a PID is still valid and not a zombie */ - if (!pid_is_valid(pid)) + if (pid < 0) return false; if (pid <= 1) /* If we or PID 1 would be a zombie, this code would not be running */ @@ -826,7 +826,7 @@ bool pid_is_alive(pid_t pid) { int pid_from_same_root_fs(pid_t pid) { const char *root; - if (!pid_is_valid(pid)) + if (pid < 0) return false; if (pid == 0 || pid == getpid_cached()) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index c91d56598b..f87b52a266 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -446,7 +446,7 @@ static int method_get_unit_by_pid(sd_bus_message *message, void *userdata, sd_bu r = sd_bus_message_read(message, "u", &pid); if (r < 0) return r; - if (!pid_is_valid((pid_t) pid)) + if (pid < 0) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid PID " PID_FMT, pid); if (pid == 0) { diff --git a/src/login/logind-dbus.c b/src/login/logind-dbus.c index 1aa6760665..d66022f802 100644 --- a/src/login/logind-dbus.c +++ b/src/login/logind-dbus.c @@ -344,7 +344,7 @@ static int method_get_session_by_pid(sd_bus_message *message, void *userdata, sd r = sd_bus_message_read(message, "u", &pid); if (r < 0) return r; - if (!pid_is_valid((pid_t) pid)) + if (pid < 0) return -EINVAL; if (pid == 0) { @@ -407,7 +407,7 @@ static int method_get_user_by_pid(sd_bus_message *message, void *userdata, sd_bu r = sd_bus_message_read(message, "u", &pid); if (r < 0) return r; - if (!pid_is_valid((pid_t) pid)) + if (pid < 0) return -EINVAL; if (pid == 0) { diff --git a/src/machine/machined-dbus.c b/src/machine/machined-dbus.c index 6618ec30f1..c9b92d2765 100644 --- a/src/machine/machined-dbus.c +++ b/src/machine/machined-dbus.c @@ -199,7 +199,7 @@ static int method_get_machine_by_pid(sd_bus_message *message, void *userdata, sd if (r < 0) return r; - if (!pid_is_valid((pid_t) pid)) + if (pid < 0) return -EINVAL; if (pid == 0) { diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 897fc48b98..c569663bab 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -3375,10 +3375,8 @@ static int logind_check_inhibitors(enum action a) { if (!sv) return log_oom(); - if (!pid_is_valid((pid_t) pid)) { - log_error("Invalid PID %" PRIu32 ".", pid); - return -ERANGE; - } + if ((pid_t) pid < 0) + return log_error_errno(ERANGE, "Bad PID %"PRIu32": %m", pid); if (!strv_contains(sv, IN_SET(a, From 72b3f82e1771e1070853a084a6108082c59ab0cb Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 3 Oct 2017 12:13:06 +0100 Subject: [PATCH 2/3] systemctl: use pid_is_valid() where appropriate This was the one valid site in commit ee043777be58251e7441b4f04594e9e3792d7fb2. The second part of this hunk, avoiding using `%m` when we didn't actually have `errno` set, seems like a nice enough cleanup to be worthwhile on it's own. Also use PID_FMT to improve the error message we print (pid_t is signed). --- src/systemctl/systemctl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index c569663bab..ca03fb49fb 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -3375,8 +3375,10 @@ static int logind_check_inhibitors(enum action a) { if (!sv) return log_oom(); - if ((pid_t) pid < 0) - return log_error_errno(ERANGE, "Bad PID %"PRIu32": %m", pid); + if (!pid_is_valid((pid_t) pid)) { + log_error("Invalid PID "PID_FMT".", (pid_t) pid); + return -ERANGE; + } if (!strv_contains(sv, IN_SET(a, From 6f876815c6e8a06f07e27b7658e04d6d595d9584 Mon Sep 17 00:00:00 2001 From: Alan Jenkins Date: Tue, 3 Oct 2017 12:26:02 +0100 Subject: [PATCH 3/3] logind: use pid_is_valid() where appropriate These two sites _do_ match the definition of pid_is_valid(); they don't provide any special handling for the invalid PID value 0. (They're used by dbus methods, so the PID value 0 is handled with reference to the dbus client creds, outside of these functions). --- src/login/logind-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/login/logind-core.c b/src/login/logind-core.c index 997c421e9a..ba538559f9 100644 --- a/src/login/logind-core.c +++ b/src/login/logind-core.c @@ -287,7 +287,7 @@ int manager_get_session_by_pid(Manager *m, pid_t pid, Session **session) { assert(m); - if (pid < 1) + if (!pid_is_valid(pid)) return -EINVAL; r = cg_pid_get_unit(pid, &unit); @@ -311,7 +311,7 @@ int manager_get_user_by_pid(Manager *m, pid_t pid, User **user) { assert(m); assert(user); - if (pid < 1) + if (!pid_is_valid(pid)) return -EINVAL; r = cg_pid_get_slice(pid, &unit);