From 92bfe09724392a4b27f919b287b08bb6945d94b1 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 May 2021 08:55:39 +0200 Subject: [PATCH 1/2] dhcp: assert that pid_t is signed for NMDhcpClient Probably pid_t is always signed, because kill() documents that negative values have a special meaning (technically, C would automatically cast negative signed values to an unsigned pid_t type too). Anyway, NMDhcpClient at several places uses -1 as special value for "no pid". At the same time, it checks for valid PIDs with "pid > 1". That only works if pid_t is signed. Add a static assertion for that. --- src/core/dhcp/nm-dhcp-client.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/core/dhcp/nm-dhcp-client.c b/src/core/dhcp/nm-dhcp-client.c index 0dc21be630..da99cbb644 100644 --- a/src/core/dhcp/nm-dhcp-client.c +++ b/src/core/dhcp/nm-dhcp-client.c @@ -83,6 +83,11 @@ G_DEFINE_ABSTRACT_TYPE(NMDhcpClient, nm_dhcp_client, G_TYPE_OBJECT) /*****************************************************************************/ +/* we use pid=-1 for invalid PIDs. Ensure that pid_t can hold negative values. */ +G_STATIC_ASSERT(!(((pid_t) -1) > 0)); + +/*****************************************************************************/ + pid_t nm_dhcp_client_get_pid(NMDhcpClient *self) { From 80ced3f1fb4754558aa19f257a1ad921e0f92616 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 May 2021 08:57:18 +0200 Subject: [PATCH 2/2] dhcpcd: fix killing all processes With kill(), the PID -1 means to send a signal to all processes. nm_dhcp_client_get_pid() can return -1, if no PID is set. This must be handled. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/726 Fixes: a2abd15fe001 ('DHCP: Support dhcpcd-9.x') --- src/core/dhcp/nm-dhcp-dhcpcd.c | 40 ++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-dhcpcd.c b/src/core/dhcp/nm-dhcp-dhcpcd.c index 8c31aed0cf..64187b8eb3 100644 --- a/src/core/dhcp/nm-dhcp-dhcpcd.c +++ b/src/core/dhcp/nm-dhcp-dhcpcd.c @@ -169,27 +169,29 @@ stop(NMDhcpClient *client, gboolean release) int sig, errsv; pid = nm_dhcp_client_get_pid(client); - sig = release ? SIGALRM : SIGTERM; - _LOGD("sending %s to dhcpcd pid %d", sig == SIGALRM ? "SIGALRM" : "SIGTERM", pid); + if (pid > 1) { + sig = release ? SIGALRM : SIGTERM; + _LOGD("sending %s to dhcpcd pid %d", sig == SIGALRM ? "SIGALRM" : "SIGTERM", pid); - /* dhcpcd-9.x features privilege separation. - * It's not our job to track all these processes so we rely on dhcpcd - * to always cleanup after itself. - * Because it also re-parents itself to PID 1, the process cannot be - * reaped or waited for. - * As such, just send the correct signal. - */ - if (kill(pid, sig) == -1) { - errsv = errno; - _LOGE("failed to kill dhcpcd %d:%s", errsv, strerror(errsv)); + /* dhcpcd-9.x features privilege separation. + * It's not our job to track all these processes so we rely on dhcpcd + * to always cleanup after itself. + * Because it also re-parents itself to PID 1, the process cannot be + * reaped or waited for. + * As such, just send the correct signal. + */ + if (kill(pid, sig) == -1) { + errsv = errno; + _LOGE("failed to kill dhcpcd %d:%s", errsv, strerror(errsv)); + } + + /* When this function exits NM expects the PID to be -1. + * This means we also need to stop watching the pid. + * If we need to know the exit status then we need to refactor NM + * to allow a non -1 to mean we're waiting to exit still. + */ + nm_dhcp_client_stop_watch_child(client, pid); } - - /* When this function exits NM expects the PID to be -1. - * This means we also need to stop watching the pid. - * If we need to know the exit status then we need to refactor NM - * to allow a non -1 to mean we're waiting to exit still. - */ - nm_dhcp_client_stop_watch_child(client, pid); } /*****************************************************************************/