From 68d6a69a8bc2f25e935608344d5b7e2b52cde85f Mon Sep 17 00:00:00 2001

From: Ken Gaillot <kgaillot@redhat.com>

Date: Mon, 21 Oct 2019 19:02:47 -0500

Subject: [PATCH] Low: libcrmservice: don't close descriptors above current

 limit

This is irrelevant in normal use. However, valgrind can open high-numbered

file descriptors for its own use above the soft limit of the process being run

under valgrind. If that process forks a child that tries to close all open file

descriptors (e.g. the executor running an agent), the close fails because the

file descriptors are invalid, and (ironically) valgrind warns about that.

This allows 5a73027 to work under valgrind. Additionally, we extend the

efficient close method from that commit to pacemakerd's spawning of children.

---

 daemons/pacemakerd/pacemakerd.c | 10 ++----

 include/crm/common/internal.h   |  3 ++

 lib/common/io.c                 | 72 +++++++++++++++++++++++++++++++++++++++++

 lib/services/services_linux.c   | 29 +----------------

 4 files changed, 79 insertions(+), 35 deletions(-)

diff --git a/daemons/pacemakerd/pacemakerd.c b/daemons/pacemakerd/pacemakerd.c

index fdc1d9f..d8ff53d 100644

--- a/daemons/pacemakerd/pacemakerd.c

+++ b/daemons/pacemakerd/pacemakerd.c

@@ -13,6 +13,8 @@

 #include <pwd.h>

 #include <grp.h>

 #include <poll.h>

+#include <stdio.h>

+#include <stdbool.h>

 #include <sys/stat.h>

 #include <sys/types.h>

 #include <sys/time.h>

@@ -290,10 +292,8 @@ static char *opts_vgrind[] = { NULL, NULL, NULL, NULL, NULL };

 static gboolean

 start_child(pcmk_child_t * child)

 {

-    int lpc = 0;

     uid_t uid = 0;

     gid_t gid = 0;

-    struct rlimit oflimits;

     gboolean use_valgrind = FALSE;

     gboolean use_callgrind = FALSE;

     const char *devnull = "/dev/null";

@@ -396,11 +396,7 @@ start_child(pcmk_child_t * child)

             crm_perror(LOG_ERR, "Could not set user to %d (%s)", uid, child->uid);

         }

-        /* Close all open file descriptors */

-        getrlimit(RLIMIT_NOFILE, &oflimits);

-        for (lpc = 0; lpc < oflimits.rlim_cur; lpc++) {

-            close(lpc);

-        }

+        pcmk__close_fds_in_child(true);

         (void)open(devnull, O_RDONLY);  /* Stdin:  fd 0 */

         (void)open(devnull, O_WRONLY);  /* Stdout: fd 1 */

diff --git a/include/crm/common/internal.h b/include/crm/common/internal.h

index b2eec00..da2c7d7 100644

--- a/include/crm/common/internal.h

+++ b/include/crm/common/internal.h

@@ -13,6 +13,7 @@

 #include <glib.h>       /* for gboolean */

 #include <dirent.h>     /* for struct dirent */

 #include <unistd.h>     /* for getpid() */

+#include <stdbool.h>    /* for bool */

 #include <sys/types.h>  /* for uid_t and gid_t */

 #include <crm/common/logging.h>

@@ -33,6 +34,8 @@ int crm_write_sync(int fd, const char *contents);

 int crm_set_nonblocking(int fd);

 const char *crm_get_tmpdir(void);

+void pcmk__close_fds_in_child(bool);

+

 /* internal procfs utilities (from procfs.c) */

diff --git a/lib/common/io.c b/lib/common/io.c

index fa438dd..6cbab0a 100644

--- a/lib/common/io.c

+++ b/lib/common/io.c

@@ -16,6 +16,7 @@

 #include <sys/param.h>

 #include <sys/types.h>

 #include <sys/stat.h>

+#include <sys/resource.h>

 #include <stdio.h>

 #include <unistd.h>

@@ -501,3 +502,74 @@ crm_get_tmpdir()

     return (dir && (*dir == '/'))? dir : "/tmp";

 }

+

+/*!

+ * \internal

+ * \brief Close open file descriptors

+ *

+ * Close all file descriptors (except optionally stdin, stdout, and stderr),

+ * which is a best practice for a new child process forked for the purpose of

+ * executing an external program.

+ *

+ * \param[in] bool  If true, close stdin, stdout, and stderr as well

+ */

+void

+pcmk__close_fds_in_child(bool all)

+{

+    DIR *dir;

+    struct rlimit rlim;

+    rlim_t max_fd;

+    int min_fd = (all? 0 : (STDERR_FILENO + 1));

+

+    /* Find the current process's (soft) limit for open files. getrlimit()

+     * should always work, but have a fallback just in case.

+     */

+    if (getrlimit(RLIMIT_NOFILE, &rlim) == 0) {

+        max_fd = rlim.rlim_cur - 1;

+    } else {

+        long conf_max = sysconf(_SC_OPEN_MAX);

+

+        max_fd = (conf_max > 0)? conf_max : 1024;

+    }

+

+    /* /proc/self/fd (on Linux) or /dev/fd (on most OSes) contains symlinks to

+     * all open files for the current process, named as the file descriptor.

+     * Use this if available, because it's more efficient than a shotgun

+     * approach to closing descriptors.

+     */

+#if SUPPORT_PROCFS

+    dir = opendir("/proc/self/fd");

+    if (dir == NULL) {

+        dir = opendir("/dev/fd");

+    }

+#else

+    dir = opendir("/dev/fd");

+#endif

+    if (dir != NULL) {

+        struct dirent *entry;

+        int dir_fd = dirfd(dir);

+

+        while ((entry = readdir(dir)) != NULL) {

+            int lpc = atoi(entry->d_name);

+

+            /* How could one of these entries be higher than max_fd, you ask?

+             * It isn't possible in normal operation, but when run under

+             * valgrind, valgrind can open high-numbered file descriptors for

+             * its own use that are higher than the process's soft limit.

+             * These will show up in the fd directory but aren't closable.

+             */

+            if ((lpc >= min_fd) && (lpc <= max_fd) && (lpc != dir_fd)) {

+                close(lpc);

+            }

+        }

+        closedir(dir);

+        return;

+    }

+

+    /* If no fd directory is available, iterate over all possible descriptors.

+     * This is less efficient due to the overhead of many system calls.

+     */

+    for (int lpc = max_fd; lpc >= min_fd; lpc--) {

+        close(lpc);

+    }

+}

diff --git a/lib/services/services_linux.c b/lib/services/services_linux.c

index 464fc5b..6870273 100644

--- a/lib/services/services_linux.c

+++ b/lib/services/services_linux.c

@@ -444,9 +444,6 @@ services_handle_exec_error(svc_action_t * op, int error)

 static void

 action_launch_child(svc_action_t *op)

 {

-    int lpc;

-    DIR *dir;

-

     /* SIGPIPE is ignored (which is different from signal blocking) by the gnutls library.

      * Depending on the libqb version in use, libqb may set SIGPIPE to be ignored as well. 

      * We do not want this to be inherited by the child process. By resetting this the signal

@@ -476,31 +473,7 @@ action_launch_child(svc_action_t *op)

      */

     setpgid(0, 0);

-    // Close all file descriptors except stdin/stdout/stderr

-#if SUPPORT_PROCFS

-    dir = opendir("/proc/self/fd");

-#else

-    dir = opendir("/dev/fd");

-#endif

-    if (dir == NULL) { /* /proc or /dev/fd not available */

-	/* Iterate over all possible fds, might be slow */

-        for (lpc = getdtablesize() - 1; lpc > STDERR_FILENO; lpc--) {

-            close(lpc);

-        }

-    } else {

-        /* Iterate over fds obtained from /proc or /dev/fd */

-        struct dirent *entry;

-        int dir_fd = dirfd(dir);

-

-        while ((entry = readdir(dir)) != NULL) {

-            lpc = atoi(entry->d_name);

-            if (lpc > STDERR_FILENO && lpc != dir_fd) {

-                close(lpc);

-            }

-        }

-

-        closedir(dir);

-    }

+    pcmk__close_fds_in_child(false);

 #if SUPPORT_CIBSECRETS

     if (replace_secret_params(op->rsc, op->params) < 0) {

-- 

1.8.3.1