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

On 12/07/2010 03:17 PM, Matthias Bolte wrote: Fixed by squashing this in: diff --git i/tests/commandtest.c w/tests/commandtest.c index 9dea6f3..e956205 100644 --- i/tests/commandtest.c +++ w/tests/commandtest.c @@ -633,7 +633,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) } if (!outbuf || *outbuf) { - puts("output string not allocated"); + puts("output buffer is not an allocated empty string"); goto cleanup; } VIR_FREE(outbuf); @@ -655,7 +655,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) } if (!outbuf || *outbuf || !errbuf || *errbuf) { - puts("output strings not allocated"); + puts("output buffers are not allocated empty strings"); goto cleanup; } Then you deserve a 'make check' failure, to force you to figure out why your system is hosed. :) Thanks for the review; pushing soon. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org