1:35 a.m.
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(a)redhat.com>
To: libguestfs(a)redhat.com,
Cc: ptoscano(a)redhat.com, berrange(a)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
