File bsc#1131353-bsc#1131356-0002-High-pacemakerd-vs... of Package pacemaker

Overview Repositories Revisions Requests Users Attributes Meta

File bsc#1131353-bsc#1131356-0002-High-pacemakerd-vs.-IPC-procfs-confused-deputy-authe.patch of Package pacemaker (Revision 0af0dce49461b1acff542bea8cb6fdde)

Currently displaying revision 0af0dce49461b1acff542bea8cb6fdde , Show latest

From 912f5d9ce983339e939e4cc55f27791f8c9baa18 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= <jpokorny@redhat.com>
Date: Mon, 15 Apr 2019 17:09:50 +0200
Subject: [PATCH 2/7] High: pacemakerd vs. IPC/procfs confused deputy
 authenticity issue (0/4)

[0/4: make crm_pid_active more precise as to when detections fail]

It would be bad if the function claimed the process is not active
when the only obstacle in the detection process was that none of the
detection methods worked for a plain lack of permissions to apply
them.  Also, do some other minor cleanup of the function and add its
documentation.  As an additional measure, log spamming is kept at
minimum for repeated queries about the same PID.
---
 include/crm/common/internal.h | 21 +++++++++++++
 lib/common/pid.c              | 68 +++++++++++++++++++++++++++----------------
 2 files changed, 64 insertions(+), 25 deletions(-)

diff --git a/include/crm/common/internal.h b/include/crm/common/internal.h
index f2944f54c..6775d1463 100644
--- a/include/crm/common/internal.h
+++ b/include/crm/common/internal.h
@@ -49,7 +49,28 @@ void crm_schema_cleanup(void);
 
 /* internal functions related to process IDs (from pid.c) */
 
+/*!
+ * \internal
+ * \brief Detect if process per PID and optionally exe path (component) exists
+ *
+ * \param[in] pid     PID of process assumed alive, disproving of which to try
+ * \param[in] daemon  exe path (component) to possibly match with procfs entry
+ *
+ * \return -1 on invalid PID specification, -2 when the calling process has no
+ *         (is refused an) ability to (dis)prove the predicate,
+ *         0 if the negation of the predicate is confirmed (check-through-kill
+ *         indicates so, or the subsequent check-through-procfs-match on
+ *         \p daemon when provided and procfs available at the standard path),
+ *         1 if it cannot be disproved (reliably [modulo race conditions]
+ *         when \p daemon provided, procfs available at the standard path
+ *         and the calling process has permissions to access the respective
+ *         procfs location, less so otherwise, since mere check-through-kill
+ *         is exercised without powers to exclude PID recycled in the interim).
+ *
+ * \note This function cannot be used to verify \e authenticity of the process.
+ */
 int crm_pid_active(long pid, const char *daemon);
+
 long crm_pidfile_inuse(const char *filename, long mypid, const char *daemon);
 long crm_read_pidfile(const char *filename);
 int crm_lock_pidfile(const char *filename, const char *name);
diff --git a/lib/common/pid.c b/lib/common/pid.c
index 803799e64..2439680c4 100644
--- a/lib/common/pid.c
+++ b/lib/common/pid.c
@@ -1,5 +1,7 @@
 /*
- * Copyright 2004-2018 Andrew Beekhof <andrew@beekhof.net>
+ * Copyright 2004-2019 the Pacemaker project contributors
+ *
+ * The version control history for this file may have further details.
  *
  * This source code is licensed under the GNU Lesser General Public License
  * version 2.1 or later (LGPLv2.1+) WITHOUT ANY WARRANTY.
@@ -20,17 +22,21 @@
 int
 crm_pid_active(long pid, const char *daemon)
 {
+    static int last_asked_pid = 0;  /* log spam prevention */
+#if SUPPORT_PROCFS
     static int have_proc_pid = 0;
+#else
+    static int have_proc_pid = -1;
+#endif
+    int rc = 0;
 
     if (have_proc_pid == 0) {
+        /* evaluation of /proc/PID/exe applicability via self-introspection */
         char proc_path[PATH_MAX], exe_path[PATH_MAX];
-
-        // Make sure pid hasn't been reused by another process
         snprintf(proc_path, sizeof(proc_path), "/proc/%lu/exe",
-                 (long unsigned int)getpid());
-
+                 (long unsigned int) getpid());
         have_proc_pid = 1;
-        if (readlink(proc_path, exe_path, PATH_MAX - 1) < 0) {
+        if (readlink(proc_path, exe_path, sizeof(exe_path) - 1) < 0) {
             have_proc_pid = -1;
         }
     }
@@ -38,40 +44,52 @@ crm_pid_active(long pid, const char *daemon)
     if (pid <= 0) {
         return -1;
 
-    } else if ((kill(pid, 0) < 0) && (errno == ESRCH)) {
-        return 0;
+    } else if ((rc = kill(pid, 0)) < 0 && errno == ESRCH) {
+        return 0;  /* no such PID detected */
 
-    } else if ((daemon == NULL) || (have_proc_pid == -1)) {
-        return 1;
+    } else if (rc < 0 && have_proc_pid == -1) {
+        if (last_asked_pid != pid) {
+            crm_info("Cannot examine PID %ld: %s", pid, strerror(errno));
+            last_asked_pid = pid;
+        }
+        return -2;  /* errno != ESRCH */
+
+    } else if (rc == 0 && (daemon == NULL || have_proc_pid == -1)) {
+        return 1;  /* kill as the only indicator, cannot double check */
 
     } else {
-        int rc = 0;
+        /* make sure PID hasn't been reused by another process
+           XXX: might still be just a zombie, which could confuse decisions */
+        bool checked_through_kill = (rc == 0);
         char proc_path[PATH_MAX], exe_path[PATH_MAX], myexe_path[PATH_MAX];
-
-        // Make sure pid hasn't been reused by another process
         snprintf(proc_path, sizeof(proc_path), "/proc/%ld/exe", pid);
 
-        rc = readlink(proc_path, exe_path, PATH_MAX - 1);
+        rc = readlink(proc_path, exe_path, sizeof(exe_path) - 1);
         if ((rc < 0) && (errno == EACCES)) {
-            crm_perror(LOG_INFO, "Could not read from %s", proc_path);
-            return 1;
+            if (last_asked_pid != pid) {
+                crm_info("Could not read from %s: %s", proc_path,
+                         strerror(errno));
+                last_asked_pid = pid;
+            }
+            return checked_through_kill ? 1 : -2;
         } else if (rc < 0) {
-            crm_perror(LOG_ERR, "Could not read from %s", proc_path);
-            return 0;
+            if (last_asked_pid != pid) {
+                crm_err("Could not read from %s: %s (%d)", proc_path,
+                        strerror(errno), errno);
+                last_asked_pid = pid;
+            }
+            return 0;  /* most likely errno == ENOENT */
         }
-
-        exe_path[rc] = 0;
+        exe_path[rc] = '\0';
 
         if (daemon[0] != '/') {
-            rc = snprintf(myexe_path, sizeof(proc_path), CRM_DAEMON_DIR"/%s",
+            rc = snprintf(myexe_path, sizeof(myexe_path), CRM_DAEMON_DIR"/%s",
                           daemon);
-            myexe_path[rc] = 0;
         } else {
-            rc = snprintf(myexe_path, sizeof(proc_path), "%s", daemon);
-            myexe_path[rc] = 0;
+            rc = snprintf(myexe_path, sizeof(myexe_path), "%s", daemon);
         }
 
-        if (strcmp(exe_path, myexe_path) == 0) {
+        if (rc > 0 && rc < sizeof(myexe_path) && !strcmp(exe_path, myexe_path)) {
             return 1;
         }
     }
-- 
2.16.4

Places

File bsc#1131353-bsc#1131356-0002-High-pacemakerd-vs.-IPC-procfs-confused-deputy-authe.patch of Package pacemaker (Revision 0af0dce49461b1acff542bea8cb6fdde)

Places