pidref: now that we have the cached pidfdid of our own process, use it

Note that this drops a lot of "const" qualifiers on PidRef arguments.
That's because pidref_is_self() suddenly might end changing the PidRef
because it acquires the pidfd ID.

We had this previously already with pidfd_equal(), but this amplifies
the problem.

I guess we C's "const" doesn't really work for stuff that contains
caches, that is just conceptually constant, but not actually.

diff --git a/src/basic/pidref.c b/src/basic/pidref.c
index ccfa2903b60efb6d4335d6a11ea8a0b5e2318fef..9b4922b16075e68f077a1cd6590b345e071e6e6c 100644 (file)
--- a/src/basic/pidref.c
+++ b/src/basic/pidref.c
@@ -83,14 +83,17 @@ bool pidref_equal(PidRef *a, PidRef *b) {
 }
 
 int pidref_set_pid(PidRef *pidref, pid_t pid) {
+        uint64_t pidfdid = 0;
         int fd;
 
         assert(pidref);
 
         if (pid < 0)
                 return -ESRCH;
-        if (pid == 0)
+        if (pid == 0) {
                 pid = getpid_cached();
+                (void) pidfd_get_inode_id_self_cached(&pidfdid);
+        }
 
         fd = pidfd_open(pid, 0);
         if (fd < 0) {
@@ -104,6 +107,7 @@ int pidref_set_pid(PidRef *pidref, pid_t pid) {
         *pidref = (PidRef) {
                 .fd = fd,
                 .pid = pid,
+                .fd_id = pidfdid,
         };
 
         return 0;
@@ -388,17 +392,32 @@ int pidref_verify(const PidRef *pidref) {
         return 1; /* We have a pidfd and it still points to the PID we have, hence all is *really* OK → return 1 */
 }
 
-bool pidref_is_self(const PidRef *pidref) {
-        if (!pidref)
+bool pidref_is_self(PidRef *pidref) {
+        if (!pidref_is_set(pidref))
                 return false;
 
         if (pidref_is_remote(pidref))
                 return false;
 
-        return pidref->pid == getpid_cached();
+        if (pidref->pid != getpid_cached())
+                return false;
+
+        /* PID1 cannot exit, hence no point in comparing pidfd IDs, they can never change */
+        if (pidref->pid == 1)
+                return true;
+
+        /* Also compare pidfd ID if we can get it */
+        if (pidref_acquire_pidfd_id(pidref) < 0)
+                return true;
+
+        uint64_t self_id;
+        if (pidfd_get_inode_id_self_cached(&self_id) < 0)
+                return true;
+
+        return pidref->fd_id == self_id;
 }
 
-int pidref_wait(const PidRef *pidref, siginfo_t *ret, int options) {
+int pidref_wait(PidRef *pidref, siginfo_t *ret, int options) {
         int r;
 
         if (!pidref_is_set(pidref))
@@ -424,7 +443,7 @@ int pidref_wait(const PidRef *pidref, siginfo_t *ret, int options) {
         return 0;
 }
 
-int pidref_wait_for_terminate(const PidRef *pidref, siginfo_t *ret) {
+int pidref_wait_for_terminate(PidRef *pidref, siginfo_t *ret) {
         int r;
 
         for (;;) {

diff --git a/src/basic/pidref.h b/src/basic/pidref.h
index a268af46037269bab4691eceb8ebd9026df76cbf..9e8a39ecfbbbb9225d2b746aebf57447f6b5daf6 100644 (file)
--- a/src/basic/pidref.h
+++ b/src/basic/pidref.h
@@ -39,7 +39,7 @@ struct PidRef {
                            their own pidfs and each process comes with a unique inode number */
 };
 
-#define PIDREF_NULL (const PidRef) { .fd = -EBADF }
+#define PIDREF_NULL (PidRef) { .fd = -EBADF }
 
 /* A special pidref value that we are using when a PID shall be automatically acquired from some surrounding
  * context, for example connection peer. Much like PIDREF_NULL it will be considered unset by
@@ -77,7 +77,7 @@ static inline int pidref_set_self(PidRef *pidref) {
         return pidref_set_pid(pidref, 0);
 }
 
-bool pidref_is_self(const PidRef *pidref);
+bool pidref_is_self(PidRef *pidref);
 
 void pidref_done(PidRef *pidref);
 PidRef* pidref_free(PidRef *pidref);
@@ -92,8 +92,8 @@ int pidref_kill(const PidRef *pidref, int sig);
 int pidref_kill_and_sigcont(const PidRef *pidref, int sig);
 int pidref_sigqueue(const PidRef *pidref, int sig, int value);
 
-int pidref_wait(const PidRef *pidref, siginfo_t *siginfo, int options);
-int pidref_wait_for_terminate(const PidRef *pidref, siginfo_t *ret);
+int pidref_wait(PidRef *pidref, siginfo_t *siginfo, int options);
+int pidref_wait_for_terminate(PidRef *pidref, siginfo_t *ret);
 
 static inline void pidref_done_sigkill_wait(PidRef *pidref) {
         if (!pidref_is_set(pidref))

diff --git a/src/basic/process-util.c b/src/basic/process-util.c
index a6ea346f78622f0414cc5d54ac279231c0a49f22..fbde9c35e67f585c0a0adcf96947c73061086450 100644 (file)
--- a/src/basic/process-util.c
+++ b/src/basic/process-util.c
@@ -1120,7 +1120,7 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) {
         return 0;
 }
 
-int pidref_is_my_child(const PidRef *pid) {
+int pidref_is_my_child(PidRef *pid) {
         int r;
 
         if (!pidref_is_set(pid))
@@ -1150,7 +1150,7 @@ int pid_is_my_child(pid_t pid) {
         return pidref_is_my_child(&PIDREF_MAKE_FROM_PID(pid));
 }
 
-int pidref_is_unwaited(const PidRef *pid) {
+int pidref_is_unwaited(PidRef *pid) {
         int r;
 
         /* Checks whether a PID is still valid at all, including a zombie */

diff --git a/src/basic/process-util.h b/src/basic/process-util.h
index cfb967c3bc445650986b66d8b4138332003a8b91..58fff2b1740958810cbedd74a7affe4aea7224c0 100644 (file)
--- a/src/basic/process-util.h
+++ b/src/basic/process-util.h
@@ -90,9 +90,9 @@ int getenv_for_pid(pid_t pid, const char *field, char **_value);
 int pid_is_alive(pid_t pid);
 int pidref_is_alive(const PidRef *pidref);
 int pid_is_unwaited(pid_t pid);
-int pidref_is_unwaited(const PidRef *pidref);
+int pidref_is_unwaited(PidRef *pidref);
 int pid_is_my_child(pid_t pid);
-int pidref_is_my_child(const PidRef *pidref);
+int pidref_is_my_child(PidRef *pidref);
 int pidref_from_same_root_fs(PidRef *a, PidRef *b);
 
 bool is_main_thread(void);

diff --git a/src/core/cgroup.c b/src/core/cgroup.c
index e959b307c6767f43ee217dcccd52ae9a1c769f1c..8e197a0303f91225b2c6ea7463e58072b790e94c 100644 (file)
--- a/src/core/cgroup.c
+++ b/src/core/cgroup.c
@@ -4482,7 +4482,7 @@ Unit *manager_get_unit_by_pidref_watching(Manager *m, const PidRef *pid) {
         return NULL;
 }
 
-Unit *manager_get_unit_by_pidref(Manager *m, const PidRef *pid) {
+Unit* manager_get_unit_by_pidref(Manager *m, PidRef *pid) {
         Unit *u;
 
         assert(m);

diff --git a/src/core/cgroup.h b/src/core/cgroup.h
index 807e56c6210ee22aefe7855e9200d899bc0527b4..08289163196fcd24ffa65c0c53f48050031ebf65 100644 (file)
--- a/src/core/cgroup.h
+++ b/src/core/cgroup.h
@@ -466,7 +466,7 @@ unsigned manager_dispatch_cgroup_realize_queue(Manager *m);
 Unit *manager_get_unit_by_cgroup(Manager *m, const char *cgroup);
 Unit *manager_get_unit_by_pidref_cgroup(Manager *m, const PidRef *pid);
 Unit *manager_get_unit_by_pidref_watching(Manager *m, const PidRef *pid);
-Unit* manager_get_unit_by_pidref(Manager *m, const PidRef *pid);
+Unit* manager_get_unit_by_pidref(Manager *m, PidRef *pid);
 Unit* manager_get_unit_by_pid(Manager *m, pid_t pid);
 
 uint64_t unit_get_ancestor_memory_min(Unit *u);

diff --git a/src/core/unit.c b/src/core/unit.c
index 99e36fe4558162d56621d837ffd0afc0a8ee69e0..7685ab1996b05429947e2587dce54a29739cbb50 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -6007,7 +6007,7 @@ bool unit_needs_console(Unit *u) {
         return exec_context_may_touch_console(ec);
 }
 
-int unit_pid_attachable(Unit *u, const PidRef *pid, sd_bus_error *error) {
+int unit_pid_attachable(Unit *u, PidRef *pid, sd_bus_error *error) {
         int r;
 
         assert(u);

diff --git a/src/core/unit.h b/src/core/unit.h
index 8a3b812a4bfa8ffd3b6b8b6499e3bbcd0b1db72c..45b7d72b7afd7cb7ebda98e842c471ffd83e6c02 100644 (file)
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -1004,7 +1004,7 @@ int unit_warn_leftover_processes(Unit *u, bool start);
 
 bool unit_needs_console(Unit *u);
 
-int unit_pid_attachable(Unit *unit, const PidRef *pid, sd_bus_error *error);
+int unit_pid_attachable(Unit *unit, PidRef *pid, sd_bus_error *error);
 
 static inline bool unit_has_job_type(Unit *u, JobType type) {
         return u && u->job && u->job->type == type;

diff --git a/src/test/test-pidref.c b/src/test/test-pidref.c
index 10033b5826a03f80268937f26bf5843f7db212f7..cb7feb034715fb0757fa39ae0f5d820817b7b764 100644 (file)
--- a/src/test/test-pidref.c
+++ b/src/test/test-pidref.c
@@ -7,8 +7,6 @@
 #include "stdio-util.h"
 #include "tests.h"
 
-#define PIDREF_NULL_NONCONST (PidRef) { .fd = -EBADF }
-
 TEST(pidref_is_set) {
         assert_se(!pidref_is_set(NULL));
         assert_se(!pidref_is_set(&PIDREF_NULL));
@@ -17,14 +15,14 @@ TEST(pidref_is_set) {
 
 TEST(pidref_equal) {
         assert_se(pidref_equal(NULL, NULL));
-        assert_se(pidref_equal(NULL, &PIDREF_NULL_NONCONST));
-        assert_se(pidref_equal(&PIDREF_NULL_NONCONST, NULL));
-        assert_se(pidref_equal(&PIDREF_NULL_NONCONST, &PIDREF_NULL_NONCONST));
+        assert_se(pidref_equal(NULL, &PIDREF_NULL));
+        assert_se(pidref_equal(&PIDREF_NULL, NULL));
+        assert_se(pidref_equal(&PIDREF_NULL, &PIDREF_NULL));
 
         assert_se(!pidref_equal(NULL, &PIDREF_MAKE_FROM_PID(1)));
         assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), NULL));
-        assert_se(!pidref_equal(&PIDREF_NULL_NONCONST, &PIDREF_MAKE_FROM_PID(1)));
-        assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_NULL_NONCONST));
+        assert_se(!pidref_equal(&PIDREF_NULL, &PIDREF_MAKE_FROM_PID(1)));
+        assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_NULL));
         assert_se(pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_MAKE_FROM_PID(1)));
         assert_se(!pidref_equal(&PIDREF_MAKE_FROM_PID(1), &PIDREF_MAKE_FROM_PID(2)));
 }
@@ -231,7 +229,7 @@ TEST(pidref_is_remote) {
         assert_se(!pidref_is_remote(&PIDREF_MAKE_FROM_PID(getpid_cached())));
         assert_se(!pidref_is_remote(&PIDREF_AUTOMATIC));
 
-        static const PidRef p = {
+        PidRef p = {
                 .pid = 1,
                 .fd = -EREMOTE,
                 .fd_id = 4711,