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

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2014-Jul-25 13:07 UTC

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

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.
---
 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 6bf0a94..3129018 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
@@ -161,4 +161,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 bb73d19..104809d 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..88c684b 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

Pino Toscano

2014-Jul-25 15:19 UTC

head link

Re: [Libguestfs] [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).

On Friday 25 July 2014 14:07:27 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.
LGTM.

-- 
Pino Toscano

Qin Zhao

2014-Jul-28 06:35 UTC

head link

Re: [Libguestfs] [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).

Hi Richard,

I see this patch is included into libguestfs >= 1.26.6 & libguestfs
>1.27.24. I feel that version is too high. Now RHEL 7.0 runs
libguestfs-1.22.6, RHEL 6.6 alpha and 6.5 runs libguestfs-1.20.11. Is that
possible to backport this patch, in order to make this problem fixed in
RHEL 7.0, 6.6, and 6.5?

Thanks & Best Regards
Qin Zhao (赵钦)
China System and Technology Lab, IBM
Tel: 86-10-82452339



From:	"Richard W.M. Jones" <rjones@redhat.com>
To:	libguestfs@redhat.com,
Cc:	ptoscano@redhat.com, berrange@redhat.com, Qin
            Zhao/China/IBM@IBMCN
Date:	2014/07/25 21:06
Subject:	[PATCH] launch: Close file descriptors after fork
            (RHBZ#1123007).



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.
---
 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 6bf0a94..3129018 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
@@ -161,4 +161,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 bb73d19..104809d 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..88c684b 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

Richard W.M. Jones

2014-Jul-28 08:42 UTC

head link

Re: [Libguestfs] [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).

On Mon, Jul 28, 2014 at 02:35:22PM +0800, Qin Zhao
wrote:> Hi Richard,
> 
> I see this patch is included into libguestfs >= 1.26.6 & libguestfs
>> 1.27.24. I feel that version is too high. Now RHEL 7.0 runs
> libguestfs-1.22.6, RHEL 6.6 alpha and 6.5 runs libguestfs-1.20.11. Is that
> possible to backport this patch, in order to make this problem fixed in
> RHEL 7.0, 6.6, and 6.5?
Please open two bugs for it against RHEL 6 & RHEL 7 using the links below:

https://bugzilla.redhat.com/enter_bug.cgi?product=Red%20Hat%20Enterprise%20Linux%206&component=libguestfs
https://bugzilla.redhat.com/enter_bug.cgi?product=Red%20Hat%20Enterprise%20Linux%207&component=libguestfs

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

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

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

Re: [Libguestfs] [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).

Re: [Libguestfs] [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).

Re: [Libguestfs] [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).