Re: [libvirt] PATCH: Allow specific FDs to be kept open in virExec()

26 Aug 2008

On Thu, Aug 21, 2008 at 02:58:30PM +0100, Daniel P. Berrange wrote:
...
With my recent patches to virExec(), all FDs except stdin/out/err are closed
before the child is exec'd to prevent accidental leaks. Of course I forgot
the one key place where we need to propagate FDs... The TAP devices passed
to QEMU.
This patch adds a 'keepfd' parameter to virExec() which allows the caller
to specify a bitset of file descriptors to keep open, and updates the 
callers to use this as required. The QEMU driver specifies any TAP fds
and the LXC driver specifies its PTY this way removing a previous hack.
QEMU networking works again with fix....
Changed to use fd_set from sys/select

Index: src/lxc_driver.c
===================================================================
RCS file: /data/cvs/libvirt/src/lxc_driver.c,v
retrieving revision 1.24
diff -u -p -r1.24 lxc_driver.c

--- src/lxc_driver.c	22 Aug 2008 15:35:37 -0000	1.24
+++ src/lxc_driver.c	26 Aug 2008 09:11:42 -0000
@@ -621,6 +621,10 @@ static int lxcControllerStart(virConnect
     const char **largv = NULL;
     pid_t child;
     int status;
+    fd_set keepfd;
+    char appPtyStr[30];
+
+    FD_ZERO(&keepfd);
 
 #define ADD_ARG_SPACE                                                   \
     do { \
@@ -644,11 +648,13 @@ static int lxcControllerStart(virConnect
             goto no_memory;                                             \
     } while (0)
 
+    snprintf(appPtyStr, sizeof(appPtyStr), "%d", appPty);
+
     ADD_ARG_LIT(vm->def->emulator);
     ADD_ARG_LIT("--name");
     ADD_ARG_LIT(vm->def->name);
     ADD_ARG_LIT("--console");
-    ADD_ARG_LIT("0");  /* Passing console master PTY as FD 0 */
+    ADD_ARG_LIT(appPtyStr);
     ADD_ARG_LIT("--background");
 
     for (i = 0 ; i < nveths ; i++) {
@@ -658,10 +664,12 @@ static int lxcControllerStart(virConnect
 
     ADD_ARG(NULL);
 
-    vm->stdin_fd = appPty; /* Passing console master PTY as FD 0 */
+    vm->stdin_fd = -1;
     vm->stdout_fd = vm->stderr_fd = logfd;
 
-    if (virExec(conn, largv, NULL, &child,
+    FD_SET(appPty, &keepfd);
+
+    if (virExec(conn, largv, NULL, &keepfd, &child,
                 vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd,
                 VIR_EXEC_NONE) < 0)
         goto cleanup;
Index: src/openvz_driver.c
===================================================================
RCS file: /data/cvs/libvirt/src/openvz_driver.c,v
retrieving revision 1.43
diff -u -p -r1.43 openvz_driver.c
--- src/openvz_driver.c	20 Aug 2008 20:48:36 -0000	1.43
+++ src/openvz_driver.c	26 Aug 2008 09:11:43 -0000
@@ -807,7 +807,8 @@ static int openvzListDomains(virConnectP
     char *endptr;
     const char *cmd[] = {VZLIST, "-ovpsid", "-H" , NULL};
 
-    ret = virExec(conn, cmd, NULL, &pid, -1, &outfd, &errfd, VIR_EXEC_NONE);
+    ret = virExec(conn, cmd, NULL, NULL,
+                  &pid, -1, &outfd, &errfd, VIR_EXEC_NONE);
     if(ret == -1) {
         openvzError(conn, VIR_ERR_INTERNAL_ERROR,
                _("Could not exec %s"), VZLIST);
@@ -844,7 +845,8 @@ static int openvzListDefinedDomains(virC
     const char *cmd[] = {VZLIST, "-ovpsid", "-H", "-S", NULL};
 
     /* the -S options lists only stopped domains */
-    ret = virExec(conn, cmd, NULL, &pid, -1, &outfd, &errfd, VIR_EXEC_NONE);
+    ret = virExec(conn, cmd, NULL, NULL,
+                  &pid, -1, &outfd, &errfd, VIR_EXEC_NONE);
     if(ret == -1) {
         openvzError(conn, VIR_ERR_INTERNAL_ERROR,
                _("Could not exec %s"), VZLIST);
Index: src/qemu_driver.c
===================================================================
RCS file: /data/cvs/libvirt/src/qemu_driver.c,v
retrieving revision 1.111
diff -u -p -r1.111 qemu_driver.c
--- src/qemu_driver.c	20 Aug 2008 20:48:36 -0000	1.111
+++ src/qemu_driver.c	26 Aug 2008 09:11:45 -0000
@@ -847,6 +847,9 @@ static int qemudStartVMDaemon(virConnect
     int *tapfds = NULL;
     int ntapfds = 0;
     int qemuCmdFlags;
+    fd_set keepfd;
+
+    FD_ZERO(&keepfd);
 
     if (virDomainIsActive(vm)) {
         qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR,
@@ -950,7 +953,10 @@ static int qemudStartVMDaemon(virConnect
     vm->stdout_fd = -1;
     vm->stderr_fd = -1;
 
-    ret = virExec(conn, argv, NULL, &vm->pid,
+    for (i = 0 ; i < ntapfds ; i++)
+        FD_SET(tapfds[i], &keepfd);
+
+    ret = virExec(conn, argv, NULL, &keepfd, &vm->pid,
                   vm->stdin_fd, &vm->stdout_fd, &vm->stderr_fd,
                   VIR_EXEC_NONBLOCK);
     if (ret == 0) {
@@ -1219,7 +1225,8 @@ dhcpStartDhcpDaemon(virConnectPtr conn,
     if (qemudBuildDnsmasqArgv(conn, network, &argv) < 0)
         return -1;
 
-    ret = virExec(conn, argv, NULL, &network->dnsmasqPid, -1, NULL, NULL, VIR_EXEC_NONBLOCK);
+    ret = virExec(conn, argv, NULL, NULL,
+                  &network->dnsmasqPid, -1, NULL, NULL, VIR_EXEC_NONBLOCK);
 
     for (i = 0; argv[i]; i++)
         VIR_FREE(argv[i]);
Index: src/storage_backend.c
===================================================================
RCS file: /data/cvs/libvirt/src/storage_backend.c,v
retrieving revision 1.19
diff -u -p -r1.19 storage_backend.c
--- src/storage_backend.c	20 Aug 2008 09:24:14 -0000	1.19
+++ src/storage_backend.c	26 Aug 2008 09:11:46 -0000
@@ -403,7 +403,8 @@ virStorageBackendRunProgRegex(virConnect
 
 
     /* Run the program and capture its output */
-    if (virExec(conn, prog, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) {
+    if (virExec(conn, prog, NULL, NULL,
+                &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) {
         goto cleanup;
     }
 
@@ -537,7 +538,8 @@ virStorageBackendRunProgNul(virConnectPt
         v[i] = NULL;
 
     /* Run the program and capture its output */
-    if (virExec(conn, prog, NULL, &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) {
+    if (virExec(conn, prog, NULL, NULL,
+                &child, -1, &fd, NULL, VIR_EXEC_NONE) < 0) {
         goto cleanup;
     }
 
Index: src/util.c
===================================================================
RCS file: /data/cvs/libvirt/src/util.c,v
retrieving revision 1.53
diff -u -p -r1.53 util.c
--- src/util.c	20 Aug 2008 19:42:36 -0000	1.53
+++ src/util.c	26 Aug 2008 09:11:47 -0000
@@ -132,6 +132,7 @@ int
 virExec(virConnectPtr conn,
         const char *const*argv,
         const char *const*envp,
+        const fd_set *keepfd,
         int *retpid,
         int infd, int *outfd, int *errfd,
         int flags) {
@@ -293,7 +294,9 @@ virExec(virConnectPtr conn,
         if (i != infd &&
             i != null &&
             i != childout &&
-            i != childerr)
+            i != childerr &&
+            (!keepfd ||
+             !FD_ISSET(i, keepfd)))
             close(i);
 
     if (flags & VIR_EXEC_DAEMON) {
@@ -403,7 +406,8 @@ virRun(virConnectPtr conn,
        int *status) {
     int childpid, exitstatus, ret;
 
-    if ((ret = virExec(conn, argv, NULL, &childpid, -1, NULL, NULL, VIR_EXEC_NONE)) < 0)
+    if ((ret = virExec(conn, argv, NULL, NULL,
+                       &childpid, -1, NULL, NULL, VIR_EXEC_NONE)) < 0)
         return ret;
 
     while ((ret = waitpid(childpid, &exitstatus, 0) == -1) && errno == EINTR);
Index: src/util.h
===================================================================
RCS file: /data/cvs/libvirt/src/util.h,v
retrieving revision 1.25
diff -u -p -r1.25 util.h
--- src/util.h	20 Aug 2008 19:42:36 -0000	1.25
+++ src/util.h	26 Aug 2008 09:11:47 -0000
@@ -26,6 +26,7 @@
 
 #include "util-lib.h"
 #include "verify.h"
+#include <sys/select.h>
 
 enum {
     VIR_EXEC_NONE   = 0,
@@ -36,6 +37,7 @@ enum {
 int virExec(virConnectPtr conn,
             const char *const*argv,
             const char *const*envp,
+            const fd_set *keepfd,
             int *retpid,
             int infd,
             int *outfd,


-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

    