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
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
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
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
Maybe Matching Threads
- [PATCH] launch: Close file descriptors after fork (RHBZ#1123007).
- [PATCH] lib: direct, uml: Unblock SIGTERM in the hypervisor and recovery processes (RHBZ#1460338).
- [PATCH v2] launch: Close file descriptors after fork (RHBZ#1123007).
- [PATCH 1/2] Close all file descriptors in the recovery process.
- [PATCH] Add safe wrapper around waitpid which deals with EINTR correctly.