[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCHv3] command: improve behavior on no output



Guarantee that outbuf/errbuf are allocated on success, even if to the
empty string.  Caller always has to free the result, and empty output
check requires checking if *outbuf=='\0'.  Makes the API easier to use
safely.  Failure is best effort allocation (some paths, like
out-of-memory, cannot allocate a buffer, but most do), so caller must
free buffer on failure; but this now guarantees that VIR_FREE(buf)
will be safe.

* docs/internals/command.html.in: Update documentation.
* src/util/command.c (virCommandSetOutputBuffer)
(virCommandSetErrorBuffer, virCommandProcessIO) Guarantee empty
string on no output.
* tests/commandtest.c (test17): New test.
---

Everyone liked the safety of v2 over the speed of v1; and in looking
at it more, I found even more ways to make it safer.  Hence this v3.

 docs/internals/command.html.in |   12 ++++++--
 src/util/command.c             |   53 ++++++++++++++++++++++++++++--------
 tests/commandtest.c            |   58 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+), 15 deletions(-)

diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in
index 259e68e..95d2b81 100644
--- a/docs/internals/command.html.in
+++ b/docs/internals/command.html.in
@@ -365,9 +365,15 @@
 </pre>

     <p>
-      Once the command has finished executing, these buffers
-      will contain the output. It is the callers responsibility
-      to free these buffers.
+      Once the command has finished executing, these buffers will
+      contain the output.  Allocation is guaranteed if virCommandRun
+      or virCommandWait succeed (if there was no output, then the
+      buffer will contain an allocated empty string); if the command
+      failed, then the buffers usually contain a best-effort
+      allocation of collected information (however, on an
+      out-of-memory condition, the buffer may still be NULL).  The
+      caller is responsible for freeing registered buffers, since the
+      buffers are designed to persist beyond virCommandFree.
     </p>

     <h3><a name="directory">Setting working directory</a></h3>
diff --git a/src/util/command.c b/src/util/command.c
index d1d8f6d..da2572d 100644
--- a/src/util/command.c
+++ b/src/util/command.c
@@ -533,11 +533,15 @@ virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf)


 /*
- * Capture the child's stdout to a string buffer
+ * Capture the child's stdout to a string buffer.  *outbuf is
+ * guaranteed to be allocated after successful virCommandRun or
+ * virCommandWait, and is best-effort allocated after failed
+ * virCommandRun; caller is responsible for freeing *outbuf.
  */
 void
 virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
 {
+    *outbuf = NULL;
     if (!cmd || cmd->has_error)
         return;

@@ -553,11 +557,15 @@ virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)


 /*
- * Capture the child's stderr to a string buffer
+ * Capture the child's stderr to a string buffer.  *errbuf is
+ * guaranteed to be allocated after successful virCommandRun or
+ * virCommandWait, and is best-effort allocated after failed
+ * virCommandRun; caller is responsible for freeing *errbuf.
  */
 void
 virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf)
 {
+    *errbuf = NULL;
     if (!cmd || cmd->has_error)
         return;

@@ -747,6 +755,7 @@ virCommandProcessIO(virCommandPtr cmd)
     int infd = -1, outfd = -1, errfd = -1;
     size_t inlen = 0, outlen = 0, errlen = 0;
     size_t inoff = 0;
+    int ret = 0;

     /* With an input buffer, feed data to child
      * via pipe */
@@ -755,12 +764,27 @@ virCommandProcessIO(virCommandPtr cmd)
         infd = cmd->inpipe;
     }

-    /* With out/err buffer, the outfd/errfd
-     * have been filled with an FD for us */
-    if (cmd->outbuf)
+    /* With out/err buffer, the outfd/errfd have been filled with an
+     * FD for us.  Guarantee an allocated string with partial results
+     * even if we encounter a later failure, as well as freeing any
+     * results accumulated over a prior run of the same command.  */
+    if (cmd->outbuf) {
         outfd = cmd->outfd;
-    if (cmd->errbuf)
+        if (VIR_REALLOC_N(*cmd->outbuf, 1) < 0) {
+            virReportOOMError();
+            ret = -1;
+        }
+    }
+    if (cmd->errbuf) {
         errfd = cmd->errfd;
+        if (VIR_REALLOC_N(*cmd->errbuf, 1) < 0) {
+            virReportOOMError();
+            ret = -1;
+        }
+    }
+    if (ret == -1)
+        goto cleanup;
+    ret = -1;

     for (;;) {
         int i;
@@ -791,7 +815,7 @@ virCommandProcessIO(virCommandPtr cmd)
                 continue;
             virReportSystemError(errno, "%s",
                                  _("unable to poll on child"));
-            return -1;
+            goto cleanup;
         }

         for (i = 0; i < nfds ; i++) {
@@ -815,7 +839,7 @@ virCommandProcessIO(virCommandPtr cmd)
                         errno != EAGAIN) {
                         virReportSystemError(errno, "%s",
                                              _("unable to write to child input"));
-                        return -1;
+                        goto cleanup;
                     }
                 } else if (done == 0) {
                     if (fds[i].fd == outfd)
@@ -825,11 +849,10 @@ virCommandProcessIO(virCommandPtr cmd)
                 } else {
                     if (VIR_REALLOC_N(*buf, *len + done + 1) < 0) {
                         virReportOOMError();
-                        return -1;
+                        goto cleanup;
                     }
                     memcpy(*buf + *len, data, done);
                     *len += done;
-                    (*buf)[*len] = '\0';
                 }
             } else {
                 int done;
@@ -841,7 +864,7 @@ virCommandProcessIO(virCommandPtr cmd)
                         errno != EAGAIN) {
                         virReportSystemError(errno, "%s",
                                              _("unable to write to child input"));
-                        return -1;
+                        goto cleanup;
                     }
                 } else {
                     inoff += done;
@@ -856,7 +879,13 @@ virCommandProcessIO(virCommandPtr cmd)
         }
     }

-    return 0;
+    ret = 0;
+cleanup:
+    if (*cmd->outbuf)
+        (*cmd->outbuf)[outlen] = '\0';
+    if (*cmd->errbuf)
+        (*cmd->errbuf)[errlen] = '\0';
+    return ret;
 }


diff --git a/tests/commandtest.c b/tests/commandtest.c
index b7261e9..20e56bc 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -595,6 +595,63 @@ cleanup:
     return ret;
 }

+/*
+ * Test string handling when no output is present.
+ */
+static int test17(const void *unused ATTRIBUTE_UNUSED)
+{
+    virCommandPtr cmd = virCommandNew("/bin/true");
+    int ret = -1;
+    char *outbuf;
+    char *errbuf;
+
+    virCommandSetOutputBuffer(cmd, &outbuf);
+    if (outbuf != NULL) {
+        puts("buffer not sanitized at registration");
+        goto cleanup;
+    }
+
+    if (virCommandRun(cmd, NULL) < 0) {
+        virErrorPtr err = virGetLastError();
+        printf("Cannot run child %s\n", err->message);
+        goto cleanup;
+    }
+
+    if (!outbuf || *outbuf) {
+        puts("output string not allocated");
+        goto cleanup;
+    }
+    VIR_FREE(outbuf);
+    if ((outbuf = strdup("should not be leaked")) == NULL) {
+        puts("test framework failure");
+        goto cleanup;
+    }
+
+    virCommandSetErrorBuffer(cmd, &errbuf);
+    if (errbuf != NULL) {
+        puts("buffer not sanitized at registration");
+        goto cleanup;
+    }
+
+    if (virCommandRun(cmd, NULL) < 0) {
+        virErrorPtr err = virGetLastError();
+        printf("Cannot run child %s\n", err->message);
+        goto cleanup;
+    }
+
+    if (!outbuf || *outbuf || !errbuf || *errbuf) {
+        puts("output strings not allocated");
+        goto cleanup;
+    }
+
+    ret = 0;
+cleanup:
+    virCommandFree(cmd);
+    VIR_FREE(outbuf);
+    VIR_FREE(errbuf);
+    return ret;
+}
+
 static int
 mymain(int argc, char **argv)
 {
@@ -658,6 +715,7 @@ mymain(int argc, char **argv)
     DO_TEST(test14);
     DO_TEST(test15);
     DO_TEST(test16);
+    DO_TEST(test17);

     return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
 }
-- 
1.7.3.2


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]