From bff0534793ef6097fda6806b40c016f2757cabec Mon Sep 17 00:00:00 2001

Message-Id: <bff0534793ef6097fda6806b40c016f2757cabec@dist-git>

From: Michal Privoznik <mprivozn@redhat.com>

Date: Wed, 9 Oct 2019 14:27:44 +0200

Subject: [PATCH] virCommand: use procfs to learn opened FDs

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

When spawning a child process, between fork() and exec() we close

all file descriptors and keep only those the caller wants us to

pass onto the child. The problem is how we do that. Currently, we

get the limit of opened files and then iterate through each one

of them and either close() it or make it survive exec(). This

approach is suboptimal (although, not that much in default

configurations where the limit is pretty low - 1024). We have

/proc where we can learn what FDs we hold open and thus we can

selectively close only those.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

(cherry picked from commit 432faf259b696043ee5d7e8f657d855419a9a3fa)

https://bugzilla.redhat.com/show_bug.cgi?id=1759904

https://bugzilla.redhat.com/show_bug.cgi?id=1760470

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

Message-Id: <b9a09b65205ad8d2b001a55c329ec36c0e4f3183.1570623892.git.mprivozn@redhat.com>

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>

---

 src/util/vircommand.c | 86 +++++++++++++++++++++++++++++++++++++++----

 1 file changed, 78 insertions(+), 8 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c

index 02dbe0fc76..2d0c987fe2 100644

--- a/src/util/vircommand.c

+++ b/src/util/vircommand.c

@@ -492,27 +492,97 @@ virExecCommon(virCommandPtr cmd, gid_t *groups, int ngroups)

     return ret;

 }

+# ifdef __linux__

+/* On Linux, we can utilize procfs and read the table of opened

+ * FDs and selectively close only those FDs we don't want to pass

+ * onto child process (well, the one we will exec soon since this

+ * is called from the child). */

+static int

+virCommandMassCloseGetFDsLinux(virCommandPtr cmd ATTRIBUTE_UNUSED,

+                               virBitmapPtr fds)

+{

+    DIR *dp = NULL;

+    struct dirent *entry;

+    const char *dirName = "/proc/self/fd";

+    int rc;

+    int ret = -1;

+

+    if (virDirOpen(&dp, dirName) < 0)

+        return -1;

+

+    while ((rc = virDirRead(dp, &entry, dirName)) > 0) {

+        int fd;

+

+        if (virStrToLong_i(entry->d_name, NULL, 10, &fd) < 0) {

+            virReportError(VIR_ERR_INTERNAL_ERROR,

+                           _("unable to parse FD: %s"),

+                           entry->d_name);

+            goto cleanup;

+        }

+

+        if (virBitmapSetBit(fds, fd) < 0) {

+            virReportError(VIR_ERR_INTERNAL_ERROR,

+                           _("unable to set FD as open: %d"),

+                           fd);

+            goto cleanup;

+        }

+    }

+

+    if (rc < 0)

+        goto cleanup;

+

+    ret = 0;

+ cleanup:

+    VIR_DIR_CLOSE(dp);

+    return ret;

+}

+

+# else /* !__linux__ */

+

+static int

+virCommandMassCloseGetFDsGeneric(virCommandPtr cmd ATTRIBUTE_UNUSED,

+                                 virBitmapPtr fds)

+{

+    virBitmapSetAll(fds);

+    return 0;

+}

+# endif /* !__linux__ */

+

 static int

 virCommandMassClose(virCommandPtr cmd,

                     int childin,

                     int childout,

                     int childerr)

 {

+    VIR_AUTOPTR(virBitmap) fds = NULL;

     int openmax = sysconf(_SC_OPEN_MAX);

-    int fd;

-    int tmpfd;

+    int fd = -1;

-    if (openmax < 0) {

-        virReportSystemError(errno,  "%s",

-                             _("sysconf(_SC_OPEN_MAX) failed"));

+    /* In general, it is not safe to call malloc() between fork() and exec()

+     * because the child might have forked at the worst possible time, i.e.

+     * when another thread was in malloc() and thus held its lock. That is to

+     * say, POSIX does not mandate malloc() to be async-safe. Fortunately,

+     * glibc developers are aware of this and made malloc() async-safe.

+     * Therefore we can safely allocate memory here (and transitively call

+     * opendir/readdir) without a deadlock. */

+

+    if (!(fds = virBitmapNew(openmax)))

         return -1;

-    }

-    for (fd = 3; fd < openmax; fd++) {

+# ifdef __linux__

+    if (virCommandMassCloseGetFDsLinux(cmd, fds) < 0)

+        return -1;

+# else

+    if (virCommandMassCloseGetFDsGeneric(cmd, fds) < 0)

+        return -1;

+# endif

+

+    fd = virBitmapNextSetBit(fds, 2);

+    for (; fd >= 0; fd = virBitmapNextSetBit(fds, fd)) {

         if (fd == childin || fd == childout || fd == childerr)

             continue;

         if (!virCommandFDIsSet(cmd, fd)) {

-            tmpfd = fd;

+            int tmpfd = fd;

             VIR_MASS_CLOSE(tmpfd);

         } else if (virSetInherit(fd, true) < 0) {

             virReportSystemError(errno, _("failed to preserve fd %d"), fd);

-- 

2.23.0