command: avoid deadlock on EPIPE situation

It is possible to deadlock libvirt by having a domain with XML
longer than PIPE_BUF, and by writing a hook script that closes
stdin early.  This is because libvirt was keeping a copy of the
child's stdin read fd open, which means the write fd in the
parent will never see EPIPE (remember, libvirt should always be
run with SIGPIPE ignored, so we should never get a SIGPIPE signal).
Since there is no error, libvirt blocks waiting for a write to
complete, even though the only reader is also libvirt.  The
solution is to ensure that only the child can act as a reader
before the parent does any writes; and then dealing with the
fallout of dealing with EPIPE.

Thankfully, this is not a security hole - since the only way to
trigger the deadlock is to install a custom hook script, anyone
that already has privileges to install a hook script already has
privileges to do any number of other equally disruptive things
to libvirt; it would only be a security hole if an unprivileged
user could install a hook script to DoS a privileged user.

* src/util/command.c (virCommandRun): Close parent's copy of child
read fd earlier.
(virCommandProcessIO): Don't let EPIPE be fatal; the child may
be done parsing input.
* tests/commandhelper.c (main): Set up a SIGPIPE situation.
* tests/commandtest.c (test20): Trigger it.
* tests/commanddata/test20.log: New file.

diff --git a/src/util/command.c b/src/util/command.c
index 16defb107b05461d8743e852d291d93c66526de6..571ea5b61e4d69c686fd41fcd27a40dca6f31301 100644 (file)
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -1738,7 +1738,7 @@ virCommandProcessIO(virCommandPtr cmd, int *inpipe)
             break;
 
         if (poll(fds, nfds, -1) < 0) {
-            if ((errno == EAGAIN) || (errno == EINTR))
+            if (errno == EAGAIN || errno == EINTR)
                 continue;
             virReportSystemError(errno, "%s",
                                  _("unable to poll on child"));
@@ -1797,8 +1797,13 @@ virCommandProcessIO(virCommandPtr cmd, int *inpipe)
                 done = write(infd, cmd->inbuf + inoff,
                              inlen - inoff);
                 if (done < 0) {
-                    if (errno != EINTR &&
-                        errno != EAGAIN) {
+                    if (errno == EPIPE) {
+                        VIR_DEBUG("child closed stdin early, ignoring EPIPE "
+                                  "on fd %d", infd);
+                        if (VIR_CLOSE(*inpipe) < 0)
+                            VIR_DEBUG("ignoring failed close on fd %d", infd);
+                        infd = -1;
+                    } else if (errno != EINTR &&  errno != EAGAIN) {
                         virReportSystemError(errno, "%s",
                                              _("unable to write to child input"));
                         goto cleanup;
@@ -1885,6 +1890,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
     bool string_io;
     bool async_io = false;
     char *str;
+    int tmpfd;
 
     if (!cmd ||cmd->has_error == ENOMEM) {
         virReportOOMError();
@@ -1965,7 +1971,7 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
     cmd->flags |= VIR_EXEC_RUN_SYNC;
     if (virCommandRunAsync(cmd, NULL) < 0) {
         if (cmd->inbuf) {
-            int tmpfd = infd[0];
+            tmpfd = infd[0];
             if (VIR_CLOSE(infd[0]) < 0)
                 VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
             tmpfd = infd[1];
@@ -1976,6 +1982,9 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
         return -1;
     }
 
+    tmpfd = infd[0];
+    if (VIR_CLOSE(infd[0]) < 0)
+        VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
     if (string_io)
         ret = virCommandProcessIO(cmd, &infd[1]);
 
@@ -1994,15 +2003,11 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
     /* Reset any capturing, in case caller runs
      * this identical command again */
     if (cmd->inbuf) {
-        int tmpfd = infd[0];
-        if (VIR_CLOSE(infd[0]) < 0)
-            VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
         tmpfd = infd[1];
         if (VIR_CLOSE(infd[1]) < 0)
             VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
     }
     if (cmd->outbuf == &outbuf) {
-        int tmpfd ATTRIBUTE_UNUSED;
         tmpfd = cmd->outfd;
         if (VIR_CLOSE(cmd->outfd) < 0)
             VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
@@ -2011,7 +2016,6 @@ virCommandRun(virCommandPtr cmd, int *exitstatus)
         VIR_FREE(outbuf);
     }
     if (cmd->errbuf == &errbuf) {
-        int tmpfd ATTRIBUTE_UNUSED;
         tmpfd = cmd->errfd;
         if (VIR_CLOSE(cmd->errfd) < 0)
             VIR_DEBUG("ignoring failed close on fd %d", tmpfd);

diff --git a/tests/commanddata/test20.log b/tests/commanddata/test20.log
new file mode 100644 (file)
index 0000000..7d48121
--- /dev/null
+++ b/tests/commanddata/test20.log
@@ -0,0 +1,13 @@
+ARG:--close-stdin
+ENV:DISPLAY=:0.0
+ENV:HOME=/home/test
+ENV:HOSTNAME=test
+ENV:LANG=C
+ENV:LOGNAME=testTMPDIR=/tmp
+ENV:PATH=/usr/bin:/bin
+ENV:USER=test
+FD:0
+FD:1
+FD:2
+DAEMON:no
+CWD:/tmp

diff --git a/tests/commandhelper.c b/tests/commandhelper.c
index a80d191ca7defc85a07a5295c82352cab03caf0c..ec0c4583edd9241449576c4a3f1fa8acb9fd44a0 100644 (file)
--- a/tests/commandhelper.c
+++ b/tests/commandhelper.c
@@ -113,6 +113,12 @@ int main(int argc, char **argv) {
 
     VIR_FORCE_FCLOSE(log);
 
+    if (argc > 1 && STREQ(argv[1], "--close-stdin")) {
+        if (freopen("/dev/null", "r", stdin) != stdin)
+            goto error;
+        usleep(100*1000);
+    }
+
     char buf[1024];
     ssize_t got;
 

diff --git a/tests/commandtest.c b/tests/commandtest.c
index 30e7efb605632ebf3a6979a465f22e5c80d5437b..fa445d31c8eccec6282e31e02bfb6e79e875ba56 100644 (file)
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -36,6 +36,9 @@
 #include "command.h"
 #include "virfile.h"
 #include "virpidfile.h"
+#include "virterror_internal.h"
+
+#define VIR_FROM_THIS VIR_FROM_NONE
 
 #ifdef WIN32
 
@@ -784,6 +787,44 @@ cleanup:
     return ret;
 }
 
+/*
+ * Run program, no args, inherit all ENV, keep CWD.
+ * Ignore huge stdin data, to provoke SIGPIPE or EPIPE in parent.
+ */
+static int test20(const void *unused ATTRIBUTE_UNUSED)
+{
+    virCommandPtr cmd = virCommandNewArgList(abs_builddir "/commandhelper",
+                                             "--close-stdin", NULL);
+    char *buf;
+    int ret = -1;
+
+    struct sigaction sig_action;
+
+    sig_action.sa_handler = SIG_IGN;
+    sig_action.sa_flags = 0;
+    sigemptyset(&sig_action.sa_mask);
+
+    sigaction(SIGPIPE, &sig_action, NULL);
+
+    if (virAsprintf(&buf, "1\n%100000d\n", 2) < 0) {
+        virReportOOMError();
+        goto cleanup;
+    }
+    virCommandSetInputBuffer(cmd, buf);
+
+    if (virCommandRun(cmd, NULL) < 0) {
+        virErrorPtr err = virGetLastError();
+        printf("Cannot run child %s\n", err->message);
+        goto cleanup;
+    }
+
+    ret = checkoutput("test20");
+cleanup:
+    virCommandFree(cmd);
+    VIR_FREE(buf);
+    return ret;
+}
+
 static const char *const newenv[] = {
     "PATH=/usr/bin:/bin",
     "HOSTNAME=test",
@@ -868,6 +909,7 @@ mymain(void)
     DO_TEST(test17);
     DO_TEST(test18);
     DO_TEST(test19);
+    DO_TEST(test20);
 
     return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
 }