core: Serialize both pid and pidfd

If we try to deserialize only a pidfd that points to a process that
has been reaped, creating the pidref object will fail, which means that
we'll try to create a pidref object from the serialized pid that comes
next. If the pid has already been reused, this will succeed and we'll
now have a pidref that points to a different process.

Let's avoid this issue by serializing both the pidfd and the pid and
creating the pidref object directly from both. This means we'll reuse
the deserialized pidfd instead of opening a new one. We'll then immediately
notice the pidfd is dead and do the appropriate follow up depending on
the unit type.

diff --git a/src/core/scope.c b/src/core/scope.c
index 14d6c1f6a467411e0debd7b3834369a679e9dd3f..cfa2aeb03f6ffd058f4a8c38c087dc45f87c8d19 100644 (file)
--- a/src/core/scope.c
+++ b/src/core/scope.c
@@ -571,6 +571,8 @@ static int scope_deserialize_item(Unit *u, const char *key, const char *value, F
         } else if (streq(key, "pids")) {
                 _cleanup_(pidref_done) PidRef pidref = PIDREF_NULL;
 
+                /* We don't check if we already received the pid before here because unit_watch_pidref()
+                 * does this check internally and discards the new pidref if we already received it before. */
                 if (deserialize_pidref(fds, value, &pidref) >= 0) {
                         r = unit_watch_pidref(u, &pidref, /* exclusive= */ false);
                         if (r < 0)

diff --git a/src/core/service.c b/src/core/service.c
index 6d0281fcd703cf4f5a8374f5c0e7eba095ba0d6c..f6800c317032f6c62ad4d2e0cfec85199a32f06e 100644 (file)
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -3184,7 +3184,7 @@ static int service_deserialize_item(Unit *u, const char *key, const char *value,
         } else if (streq(key, "main-pid")) {
                 PidRef pidref;
 
-                if (deserialize_pidref(fds, value, &pidref) >= 0)
+                if (!pidref_is_set(&s->main_pid) && deserialize_pidref(fds, value, &pidref) >= 0)
                         (void) service_set_main_pidref(s, pidref);
 
         } else if (streq(key, "main-pid-known")) {

diff --git a/src/shared/serialize.c b/src/shared/serialize.c
index d1f41ce4c821b2db2a1776ffb32133824bc24d2a..735caf4978f1afeef77ac8ee3bb660d1d124361f 100644 (file)
--- a/src/shared/serialize.c
+++ b/src/shared/serialize.c
@@ -188,18 +188,20 @@ int serialize_pidref(FILE *f, FDSet *fds, const char *key, PidRef *pidref) {
         if (!pidref_is_set(pidref))
                 return 0;
 
-        /* We always serialize the pid, to keep downgrades mostly working (older versions will deserialize
-         * the pid and silently fail to deserialize the pidfd). If we also have a pidfd, we serialize it
-         * first and encode the fd number prefixed by "@" in the serialization. */
+        /* We always serialize the pid separately, to keep downgrades mostly working (older versions will
+         * deserialize the pid and silently fail to deserialize the pidfd). If we also have a pidfd, we
+         * serialize both the pid and pidfd, so that we can construct the exact same pidref after
+         * deserialization (this doesn't work with only the pidfd, as we can't retrieve the original pid
+         * from the pidfd anymore if the process is reaped). */
 
         if (pidref->fd >= 0) {
                 int copy = fdset_put_dup(fds, pidref->fd);
                 if (copy < 0)
                         return log_error_errno(copy, "Failed to add file descriptor to serialization set: %m");
 
-                r = serialize_item_format(f, key, "@%i", copy);
+                r = serialize_item_format(f, key, "@%i:" PID_FMT, copy, pidref->pid);
                 if (r < 0)
-                        return log_error_errno(r, "Failed to serialize PID file descriptor: %m");
+                        return r;
         }
 
         return serialize_item_format(f, key, PID_FMT, pidref->pid);
@@ -480,12 +482,39 @@ int deserialize_pidref(FDSet *fds, const char *value, PidRef *ret) {
 
         e = startswith(value, "@");
         if (e) {
-                int fd = deserialize_fd(fds, e);
+                _cleanup_free_ char *fdstr = NULL, *pidstr = NULL;
+                _cleanup_close_ int fd = -EBADF;
+
+                r = extract_many_words(&e, ":", /* flags = */ 0, &fdstr, &pidstr);
+                if (r < 0)
+                        return log_debug_errno(r, "Failed to deserialize pidref '%s': %m", e);
+                if (r == 0)
+                        return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Cannot deserialize pidref from empty string.");
+
+                assert(r <= 2);
 
+                fd = deserialize_fd(fds, fdstr);
                 if (fd < 0)
                         return fd;
 
-                r = pidref_set_pidfd_consume(ret, fd);
+                /* The serialization format changed after 255.4. In systemd <= 255.4 only pidfd is
+                 * serialized, but that causes problems when reconstructing pidref (see serialize_pidref for
+                 * details). After 255.4 the pid is serialized as well even if we have a pidfd, but we still
+                 * need to support older format as we might be upgrading from a version that still uses the
+                 * old format. */
+                if (pidstr) {
+                        pid_t pid;
+
+                        r = parse_pid(pidstr, &pid);
+                        if (r < 0)
+                                return log_debug_errno(r, "Failed to parse PID: %s", pidstr);
+
+                        *ret = (PidRef) {
+                                .pid = pid,
+                                .fd = TAKE_FD(fd),
+                        };
+                } else
+                        r = pidref_set_pidfd_consume(ret, TAKE_FD(fd));
         } else {
                 pid_t pid;