9:07 p.m.
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