Libguestfs - Jul 2014 - [PATCH v2] launch: Close file descriptors after fork (RHBZ#1123007).

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

This is version 2 of the patch which avoids incorrectly closing
stderr, so we can still see debug and error messages.

Rich.

This refactors existing code to close file descriptors in the recovery
process, and also adds code to close file descriptors between the
fork() and exec() of QEMU or User-Mode Linux.

The reason is to avoid leaking main process file descriptors where the
main process (or other libraries in the main process) are not setting
O_CLOEXEC at all or not setting it atomically.  Python is a particular
culprit.

See also this OpenStack Nova bug report:
https://bugs.launchpad.net/nova/+bug/1313477

Thanks: Qin Zhao for identifying and characterizing the problem in Nova.

This is version 2 of this commit.  This commit is identical to the
reverted commit 115fcc34325f965ac3723683e4462fc667dcd254 except that
we don't close stderr.
---
 src/guestfs-internal-frontend.h | 16 +++++++++++++++-
 src/launch-direct.c             | 17 +++++++++--------
 src/launch-uml.c                | 13 +++++--------
 3 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/src/guestfs-internal-frontend.h b/src/guestfs-internal-frontend.h
index acd7be5..1e59ad1 100644
--- a/src/guestfs-internal-frontend.h
+++ b/src/guestfs-internal-frontend.h
@@ -1,5 +1,5 @@
 /* libguestfs
- * Copyright (C) 2013 Red Hat Inc.
+ * Copyright (C) 2013-2014 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -164,4 +164,18 @@ extern GUESTFS_DLL_PUBLIC int guestfs___add_libvirt_dom
(guestfs_h *g, virDomain
 #  define program_name "libguestfs"
 #endif
 
+/* Close all file descriptors matching the condition. */
+#define close_file_descriptors(cond) do {                               \
+    int max_fd = sysconf (_SC_OPEN_MAX);                                \
+    int fd;                                                             \
+    if (max_fd == -1)                                                   \
+      max_fd = 1024;                                                    \
+    if (max_fd > 65536)                                                 \
+      max_fd = 65536;          /* bound the amount of work we do here */ \
+    for (fd = 0; fd < max_fd; ++fd) {                                   \
+      if (cond)                                                         \
+        close (fd);                                                     \
+    }                                                                   \
+  } while (0)
+
 #endif /* GUESTFS_INTERNAL_FRONTEND_H_ */
diff --git a/src/launch-direct.c b/src/launch-direct.c
index 3de869b..0c3589b 100644
--- a/src/launch-direct.c
+++ b/src/launch-direct.c
@@ -717,6 +717,13 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
         goto dup_failed;
 
       close (sv[1]);
+
+      /* Close any other file descriptors that we don't want to pass
+       * to qemu.  This prevents file descriptors which didn't have
+       * O_CLOEXEC set properly from leaking into the subprocess.  See
+       * RHBZ#1123007.
+       */
+      close_file_descriptors (fd > 2);
     }
 
     /* Dump the command line (after setting up stderr above). */
@@ -747,7 +754,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
   if (g->recovery_proc) {
     r = fork ();
     if (r == 0) {
-      int i, fd, max_fd;
+      int i;
       struct sigaction sa;
       pid_t qemu_pid = data->pid;
       pid_t parent_pid = getppid ();
@@ -767,13 +774,7 @@ launch_direct (guestfs_h *g, void *datav, const char *arg)
       /* Close all other file descriptors.  This ensures that we don't
        * hold open (eg) pipes from the parent process.
        */
-      max_fd = sysconf (_SC_OPEN_MAX);
-      if (max_fd == -1)
-        max_fd = 1024;
-      if (max_fd > 65536)
-        max_fd = 65536; /* bound the amount of work we do here */
-      for (fd = 0; fd < max_fd; ++fd)
-        close (fd);
+      close_file_descriptors (1);
 
       /* It would be nice to be able to put this in the same process
        * group as qemu (ie. setpgid (0, qemu_pid)).  However this is
diff --git a/src/launch-uml.c b/src/launch-uml.c
index 2a6ddaf..21525e3 100644
--- a/src/launch-uml.c
+++ b/src/launch-uml.c
@@ -333,6 +333,9 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
         goto dup_failed;
 
       close (csv[1]);
+
+      /* RHBZ#1123007 */
+      close_file_descriptors (fd > 2 && fd != dsv[1]);
     }
 
     /* Dump the command line (after setting up stderr above). */
@@ -360,7 +363,7 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
   if (g->recovery_proc) {
     r = fork ();
     if (r == 0) {
-      int i, fd, max_fd;
+      int i;
       struct sigaction sa;
       pid_t vmlinux_pid = data->pid;
       pid_t parent_pid = getppid ();
@@ -380,13 +383,7 @@ launch_uml (guestfs_h *g, void *datav, const char *arg)
       /* Close all other file descriptors.  This ensures that we don't
        * hold open (eg) pipes from the parent process.
        */
-      max_fd = sysconf (_SC_OPEN_MAX);
-      if (max_fd == -1)
-        max_fd = 1024;
-      if (max_fd > 65536)
-        max_fd = 65536; /* bound the amount of work we do here */
-      for (fd = 0; fd < max_fd; ++fd)
-        close (fd);
+      close_file_descriptors (1);
 
       /* It would be nice to be able to put this in the same process
        * group as vmlinux (ie. setpgid (0, vmlinux_pid)).  However
-- 
1.9.0

On Wednesday 30 July 2014 14:28:22 Richard W.M. Jones
wrote:
> This refactors existing code to close file descriptors in the recovery
> process, and also adds code to close file descriptors between the
> fork() and exec() of QEMU or User-Mode Linux.
> 
> The reason is to avoid leaking main process file descriptors where the
> main process (or other libraries in the main process) are not setting
> O_CLOEXEC at all or not setting it atomically.  Python is a
> particular culprit.
> 
> See also this OpenStack Nova bug report:
> https://bugs.launchpad.net/nova/+bug/1313477
> 
> Thanks: Qin Zhao for identifying and characterizing the problem in
> Nova.
> 
> This is version 2 of this commit.  This commit is identical to the
> reverted commit 115fcc34325f965ac3723683e4462fc667dcd254 except that
> we don't close stderr.
LGTM.

-- 
Pino Toscano