From ee11729d7f06197fd9770b3389118e2772b515a8 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 6 Dec 2010 16:36:34 -0700 Subject: [PATCH] command: enforce fd vs. buffer considerations * docs/internals/command.html.in: Better documentation of buffer vs. fd considerations. * src/util/command.c (virCommandRunAsync): Reject raw execution with string io. (virCommandRun): Reject execution with user-specified fds not visiting a regular file. --- docs/internals/command.html.in | 35 ++++++++++++++++++++------- src/util/command.c | 44 +++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in index c4fa8006cc..259e68e716 100644 --- a/docs/internals/command.html.in +++ b/docs/internals/command.html.in @@ -250,7 +250,7 @@ allow an open file handle to be passed into the child, while controlling whether that handle remains open in the parent or guaranteeing that the handle will be closed in the parent after - either virCommandRun or virCommandFree. + virCommandRun, virCommandRunAsync, or virCommandFree. 

@@ -266,9 +266,16 @@
       With this, both file descriptors sharedfd and childfd in the
       current process remain open as the same file descriptors in the
       child. Meanwhile, after the child is spawned, sharedfd remains
-      open in the parent, while childfd is closed.  For stdin/out/err
-      it is usually necessary to map a file handle. To attach file
-      descriptor 7 in the current process to stdin in the child:
+      open in the parent, while childfd is closed.
+    

+
+    

+      For stdin/out/err it is sometimes necessary to map a file
+      handle.  If a mapped file handle is a pipe fed or consumed by
+      the caller, then the caller should use virCommandDaemonize or
+      virCommandRunAsync rather than virCommandRun to avoid deadlock
+      (mapping a regular file is okay with virCommandRun).  To attach
+      file descriptor 7 in the current process to stdin in the child:
     

 
 
@@ -322,11 +329,21 @@
     
Feeding & capturing strings to/from the child

 
     

-      Often dealing with file handles for stdin/out/err
-      is unnecessarily complex. It is possible to specify
-      a string buffer to act as the data source for the
-      child's stdin, if there are no embedded NUL bytes,
-      and if the command will be run with virCommandRun:
+      Often dealing with file handles for stdin/out/err is
+      unnecessarily complex; an alternative is to let virCommandRun
+      perform the I/O and interact via string buffers. Use of a buffer
+      only works with virCommandRun, and cannot be mixed with pipe
+      file descriptors.  That is, the choice is generally between
+      managing all I/O in the caller (any fds not specified are tied
+      to /dev/null), or letting virCommandRun manage all I/O via
+      strings (unspecified stdin is tied to /dev/null, and unspecified
+      output streams get logged but are otherwise discarded).
+    

+
+    

+      It is possible to specify a string buffer to act as the data
+      source for the child's stdin, if there are no embedded NUL
+      bytes, and if the command will be run with virCommandRun:
     

 
 
diff --git a/src/util/command.c b/src/util/command.c
index c0520ec7e0..3dfd33d6fe 100644
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include "command.h"
@@ -872,6 +873,9 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
     char *outbuf = NULL;
     char *errbuf = NULL;
     int infd[2];
+    struct stat st;
+    bool string_io;
+    bool async_io = false;
 
     if (!cmd ||cmd->has_error == ENOMEM) {
         virReportOOMError();
@@ -883,6 +887,35 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
         return -1;
     }
 
+    /* Avoid deadlock, by requiring that any open fd not under our
+     * control must be visiting a regular file, or that we are
+     * daemonized and no string io is required.  */
+    string_io = cmd->inbuf || cmd->outbuf || cmd->errbuf;
+    if (cmd->infd != -1 &&
+        (fstat(cmd->infd, &st) < 0 || !S_ISREG(st.st_mode)))
+        async_io = true;
+    if (cmd->outfdptr && cmd->outfdptr != &cmd->outfd &&
+        (*cmd->outfdptr == -1 ||
+         fstat(*cmd->outfdptr, &st) < 0 || !S_ISREG(st.st_mode)))
+        async_io = true;
+    if (cmd->errfdptr && cmd->errfdptr != &cmd->errfd &&
+        (*cmd->errfdptr == -1 ||
+         fstat(*cmd->errfdptr, &st) < 0 || !S_ISREG(st.st_mode)))
+        async_io = true;
+    if (async_io) {
+        if (!(cmd->flags & VIR_EXEC_DAEMON) || string_io) {
+            virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
+                            _("cannot mix caller fds with blocking execution"));
+            return -1;
+        }
+    } else {
+        if ((cmd->flags & VIR_EXEC_DAEMON) && string_io) {
+            virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
+                            _("cannot mix string I/O with daemon"));
+            return -1;
+        }
+    }
+
     /* If we have an input buffer, we need
      * a pipe to feed the data to the child */
     if (cmd->inbuf) {
@@ -921,7 +954,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
         return -1;
     }
 
-    if (cmd->inbuf || cmd->outbuf || cmd->errbuf)
+    if (string_io)
         ret = virCommandProcessIO(cmd);
 
     if (virCommandWait(cmd, exitstatus) < 0)
@@ -1001,6 +1034,15 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid)
         return -1;
     }
 
+    /* Buffer management can only be requested via virCommandRun.  */
+    if ((cmd->inbuf && cmd->infd == -1) ||
+        (cmd->outbuf && cmd->outfdptr != &cmd->outfd) ||
+        (cmd->errbuf && cmd->errfdptr != &cmd->errfd)) {
+        virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
+                        _("cannot mix string I/O with asynchronous command"));
+        return -1;
+    }
+
     if (cmd->pid != -1) {
         virCommandError(VIR_ERR_INTERNAL_ERROR,
                         _("command is already running as pid %d"),