core: reduce the number of stalled PIDs from the watched processes list when possible

Some PIDs can remain in the watched list even though their processes have
exited since a long time. It can easily happen if the main process of a forking
service manages to spawn a child before the control process exits for example.

However when a pid is about to be mapped to a unit by calling unit_watch_pid(),
the caller usually knows if the pid should belong to this unit exclusively: if
we just forked() off a child, then we can be sure that its PID is otherwise
unused. In this case we take this opportunity to remove any stalled PIDs from
the watched process list.

If we learnt about a PID in any other form (for example via PID file, via
searching, MAINPID= and so on), then we can't assume anything.

Thanks Renaud Métrich for backporting this to RHEL.
Resolves: #1744972

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index b7ed07e65bac324de820dddbff3af5a855b9ce74..76eafdc082f078bd36e2390c16062c26dd101428 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -1926,7 +1926,7 @@ static int unit_watch_pids_in_path(Unit *u, const char *path) {
                 pid_t pid;
 
                 while ((r = cg_read_pid(f, &pid)) > 0) {
-                        r = unit_watch_pid(u, pid);
+                        r = unit_watch_pid(u, pid, false);
                         if (r < 0 && ret >= 0)
                                 ret = r;
                 }

diff --git a/src/core/dbus-scope.c b/src/core/dbus-scope.c
index 6725f627944bb7cba170657ea8e93aa61383e3f9..0bbf64fff118b87b6915a0bcca4c672bf472911a 100644 (file)
--- a/src/core/dbus-scope.c
+++ b/src/core/dbus-scope.c
@@ -106,7 +106,7 @@ static int bus_scope_set_transient_property(
                                 return r;
 
                         if (!UNIT_WRITE_FLAGS_NOOP(flags)) {
-                                r = unit_watch_pid(UNIT(s), pid);
+                                r = unit_watch_pid(UNIT(s), pid, false);
                                 if (r < 0 && r != -EEXIST)
                                         return r;
                         }

diff --git a/src/core/manager.c b/src/core/manager.c
index c83e296cf39d6502b09d43f49bb041154faa32d5..0eae7d46fb04b5eb871f4c5f74000162f20a6445 100644 (file)
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -2044,6 +2044,16 @@ void manager_clear_jobs(Manager *m) {
                 job_finish_and_invalidate(j, JOB_CANCELED, false, false);
 }
 
+void manager_unwatch_pid(Manager *m, pid_t pid) {
+        assert(m);
+
+        /* First let's drop the unit keyed as "pid". */
+        (void) hashmap_remove(m->watch_pids, PID_TO_PTR(pid));
+
+        /* Then, let's also drop the array keyed by -pid. */
+        free(hashmap_remove(m->watch_pids, PID_TO_PTR(-pid)));
+}
+
 static int manager_dispatch_run_queue(sd_event_source *source, void *userdata) {
         Manager *m = userdata;
         Job *j;

diff --git a/src/core/manager.h b/src/core/manager.h
index c7f4d66ecd7f11e4844f67e974a3f5c6dd68416a..fa47952d24ad55593b3986c3e9def688cbf56f1a 100644 (file)
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -406,6 +406,8 @@ int manager_get_dump_string(Manager *m, char **ret);
 
 void manager_clear_jobs(Manager *m);
 
+void manager_unwatch_pid(Manager *m, pid_t pid);
+
 unsigned manager_dispatch_load_queue(Manager *m);
 
 int manager_environment_add(Manager *m, char **minus, char **plus);

diff --git a/src/core/mount.c b/src/core/mount.c
index 2ac04e387427625810d282ed2ae10eaff0d38480..5878814b1b9fe62b9fa362d221c7e87a5289679a 100644 (file)
--- a/src/core/mount.c
+++ b/src/core/mount.c
@@ -677,7 +677,7 @@ static int mount_coldplug(Unit *u) {
             pid_is_unwaited(m->control_pid) &&
             MOUNT_STATE_WITH_PROCESS(new_state)) {
 
-                r = unit_watch_pid(UNIT(m), m->control_pid);
+                r = unit_watch_pid(UNIT(m), m->control_pid, false);
                 if (r < 0)
                         return r;
 
@@ -781,9 +781,8 @@ static int mount_spawn(Mount *m, ExecCommand *c, pid_t *_pid) {
         if (r < 0)
                 return r;
 
-        r = unit_watch_pid(UNIT(m), pid);
+        r = unit_watch_pid(UNIT(m), pid, true);
         if (r < 0)
-                /* FIXME: we need to do something here */
                 return r;
 
         *_pid = pid;

diff --git a/src/core/service.c b/src/core/service.c
index 614ba05d897d546696c012384c91e43944badd4d..310838a5f63c43b46bb428609530915c385be2ab 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -974,7 +974,7 @@ static int service_load_pid_file(Service *s, bool may_warn) {
         if (r < 0)
                 return r;
 
-        r = unit_watch_pid(UNIT(s), pid);
+        r = unit_watch_pid(UNIT(s), pid, false);
         if (r < 0) /* FIXME: we need to do something here */
                 return log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" for service: %m", pid);
 
@@ -1004,7 +1004,7 @@ static void service_search_main_pid(Service *s) {
         if (service_set_main_pid(s, pid) < 0)
                 return;
 
-        r = unit_watch_pid(UNIT(s), pid);
+        r = unit_watch_pid(UNIT(s), pid, false);
         if (r < 0)
                 /* FIXME: we need to do something here */
                 log_unit_warning_errno(UNIT(s), r, "Failed to watch PID "PID_FMT" from: %m", pid);
@@ -1135,7 +1135,7 @@ static int service_coldplug(Unit *u) {
                     SERVICE_RUNNING, SERVICE_RELOAD,
                     SERVICE_STOP, SERVICE_STOP_SIGABRT, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST,
                     SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL))) {
-                r = unit_watch_pid(UNIT(s), s->main_pid);
+                r = unit_watch_pid(UNIT(s), s->main_pid, false);
                 if (r < 0)
                         return r;
         }
@@ -1147,7 +1147,7 @@ static int service_coldplug(Unit *u) {
                    SERVICE_RELOAD,
                    SERVICE_STOP, SERVICE_STOP_SIGABRT, SERVICE_STOP_SIGTERM, SERVICE_STOP_SIGKILL, SERVICE_STOP_POST,
                    SERVICE_FINAL_SIGTERM, SERVICE_FINAL_SIGKILL)) {
-                r = unit_watch_pid(UNIT(s), s->control_pid);
+                r = unit_watch_pid(UNIT(s), s->control_pid, false);
                 if (r < 0)
                         return r;
         }
@@ -1545,8 +1545,8 @@ static int service_spawn(
         s->exec_fd_event_source = TAKE_PTR(exec_fd_source);
         s->exec_fd_hot = false;
 
-        r = unit_watch_pid(UNIT(s), pid);
-        if (r < 0) /* FIXME: we need to do something here */
+        r = unit_watch_pid(UNIT(s), pid, true);
+        if (r < 0)
                 return r;
 
         *_pid = pid;
@@ -3643,7 +3643,7 @@ static void service_notify_message(
                         }
                         if (r > 0) {
                                 service_set_main_pid(s, new_main_pid);
-                                unit_watch_pid(UNIT(s), new_main_pid);
+                                unit_watch_pid(UNIT(s), new_main_pid, false);
                                 notify_dbus = true;
                         }
                 }
@@ -3858,7 +3858,7 @@ static void service_bus_name_owner_change(
                         log_unit_debug(u, "D-Bus name %s is now owned by process " PID_FMT, name, pid);
 
                         service_set_main_pid(s, pid);
-                        unit_watch_pid(UNIT(s), pid);
+                        unit_watch_pid(UNIT(s), pid, false);
                 }
         }
 }

diff --git a/src/core/socket.c b/src/core/socket.c
index d488c64e914e5792a525f085ebbed93a48594a96..b034549634dbab87ba7a52e3a108cf535ab2f12d 100644 (file)
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -1816,7 +1816,7 @@ static int socket_coldplug(Unit *u) {
                    SOCKET_FINAL_SIGTERM,
                    SOCKET_FINAL_SIGKILL)) {
 
-                r = unit_watch_pid(UNIT(s), s->control_pid);
+                r = unit_watch_pid(UNIT(s), s->control_pid, false);
                 if (r < 0)
                         return r;
 
@@ -1902,9 +1902,8 @@ static int socket_spawn(Socket *s, ExecCommand *c, pid_t *_pid) {
         if (r < 0)
                 return r;
 
-        r = unit_watch_pid(UNIT(s), pid);
+        r = unit_watch_pid(UNIT(s), pid, true);
         if (r < 0)
-                /* FIXME: we need to do something here */
                 return r;
 
         *_pid = pid;
@@ -1973,7 +1972,7 @@ static int socket_chown(Socket *s, pid_t *_pid) {
                 _exit(EXIT_SUCCESS);
         }
 
-        r = unit_watch_pid(UNIT(s), pid);
+        r = unit_watch_pid(UNIT(s), pid, true);
         if (r < 0)
                 goto fail;
 

diff --git a/src/core/swap.c b/src/core/swap.c
index b644753a1cd8b1426bac042f94db8928a1f4963a..e717dbb54abc5861b4412ee37a83e53d9403f650 100644 (file)
--- a/src/core/swap.c
+++ b/src/core/swap.c
@@ -531,7 +531,7 @@ static int swap_coldplug(Unit *u) {
             pid_is_unwaited(s->control_pid) &&
             SWAP_STATE_WITH_PROCESS(new_state)) {
 
-                r = unit_watch_pid(UNIT(s), s->control_pid);
+                r = unit_watch_pid(UNIT(s), s->control_pid, false);
                 if (r < 0)
                         return r;
 
@@ -636,9 +636,8 @@ static int swap_spawn(Swap *s, ExecCommand *c, pid_t *_pid) {
         if (r < 0)
                 goto fail;
 
-        r = unit_watch_pid(UNIT(s), pid);
+        r = unit_watch_pid(UNIT(s), pid, true);
         if (r < 0)
-                /* FIXME: we need to do something here */
                 goto fail;
 
         *_pid = pid;

diff --git a/src/core/unit.c b/src/core/unit.c
index d298afb0d4ae26147105474e3bff6531b2778127..b0b1c77ef7bbcff07b56ecaa77f2acdf91c8f8b4 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -2500,7 +2500,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag
         unit_add_to_gc_queue(u);
 }
 
-int unit_watch_pid(Unit *u, pid_t pid) {
+int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) {
         int r;
 
         assert(u);
@@ -2508,6 +2508,12 @@ int unit_watch_pid(Unit *u, pid_t pid) {
 
         /* Watch a specific PID */
 
+        /* Caller might be sure that this PID belongs to this unit only. Let's take this
+         * opportunity to remove any stalled references to this PID as they can be created
+         * easily (when watching a process which is not our direct child). */
+        if (exclusive)
+                manager_unwatch_pid(u->manager, pid);
+
         r = set_ensure_allocated(&u->pids, NULL);
         if (r < 0)
                 return r;

diff --git a/src/core/unit.h b/src/core/unit.h
index e1a60da244ae0f6c4996a23e2fc21aad64354d82..68cc1869e4b1e1bee8cdc94f67699f758d4ac53c 100644 (file)
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -655,7 +655,7 @@ typedef enum UnitNotifyFlags {
 
 void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags);
 
-int unit_watch_pid(Unit *u, pid_t pid);
+int unit_watch_pid(Unit *u, pid_t pid, bool exclusive);
 void unit_unwatch_pid(Unit *u, pid_t pid);
 void unit_unwatch_all_pids(Unit *u);
 

diff --git a/src/test/test-watch-pid.c b/src/test/test-watch-pid.c
index cb43b35bc5d986e254824232079b4c3ee8e319d4..8c70175aed2880718e3ce5e84e75577b23ab017b 100644 (file)
--- a/src/test/test-watch-pid.c
+++ b/src/test/test-watch-pid.c
@@ -49,25 +49,25 @@ int main(int argc, char *argv[]) {
         assert_se(hashmap_isempty(m->watch_pids));
         assert_se(manager_get_unit_by_pid(m, 4711) == NULL);
 
-        assert_se(unit_watch_pid(a, 4711) >= 0);
+        assert_se(unit_watch_pid(a, 4711, false) >= 0);
         assert_se(manager_get_unit_by_pid(m, 4711) == a);
 
-        assert_se(unit_watch_pid(a, 4711) >= 0);
+        assert_se(unit_watch_pid(a, 4711, false) >= 0);
         assert_se(manager_get_unit_by_pid(m, 4711) == a);
 
-        assert_se(unit_watch_pid(b, 4711) >= 0);
+        assert_se(unit_watch_pid(b, 4711, false) >= 0);
         u = manager_get_unit_by_pid(m, 4711);
         assert_se(u == a || u == b);
 
-        assert_se(unit_watch_pid(b, 4711) >= 0);
+        assert_se(unit_watch_pid(b, 4711, false) >= 0);
         u = manager_get_unit_by_pid(m, 4711);
         assert_se(u == a || u == b);
 
-        assert_se(unit_watch_pid(c, 4711) >= 0);
+        assert_se(unit_watch_pid(c, 4711, false) >= 0);
         u = manager_get_unit_by_pid(m, 4711);
         assert_se(u == a || u == b || u == c);
 
-        assert_se(unit_watch_pid(c, 4711) >= 0);
+        assert_se(unit_watch_pid(c, 4711, false) >= 0);
         u = manager_get_unit_by_pid(m, 4711);
         assert_se(u == a || u == b || u == c);