[libvirt] [PATCH 1/2] virCommand: document behavior on no output
Option 1: This patch (all callers have to worry about NULL buffers, but checking for output is a simple pointer check). Option 2: Guarantee that outbuf/errbuf are allocated, even if to the empty string. Caller always has to free the result, and empty output check requires checking if *outbuf=='\0'. Personally, I prefer option 2. Thoughts? * docs/internals/command.html.in: Update documentation. * src/util/command.c (virCommandSetOutputBuffer) (virCommandSetErrorBuffer) Guarantee NULL buffer on no output. --- docs/internals/command.html.in | 4 ++-- src/util/command.c | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in index c4fa800..ea9ec64 100644 --- a/docs/internals/command.html.in +++ b/docs/internals/command.html.in @@ -349,8 +349,8 @@ <p> Once the command has finished executing, these buffers - will contain the output. It is the callers responsibility - to free these buffers. + will contain the output, or be NULL if there was no output. It + is the callers responsibility to free these buffers. </p> <h3><a name="directory">Setting working directory</a></h3> diff --git a/src/util/command.c b/src/util/command.c index aa43f76..1923799 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -549,6 +549,7 @@ virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf) return; } + VIR_FREE(*outbuf); cmd->outbuf = outbuf; cmd->outfdptr = &cmd->outfd; } @@ -569,6 +570,7 @@ virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) return; } + VIR_FREE(*errbuf); cmd->errbuf = errbuf; cmd->errfdptr = &cmd->errfd; } -- 1.7.3.2
Option 2. Which one should I go with? By the way, my pending patch for converting openvz to use virCommand instead of popen is impacted (it has a potential null dereference if we go with option 1). * docs/internals/command.html.in: Update documentation. * src/util/command.c (virCommandSetOutputBuffer) (virCommandSetErrorBuffer, virCommandProcessIO) Guarantee empty string on no output. --- docs/internals/command.html.in | 8 +++++--- src/util/command.c | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in index ea9ec64..d2fb133 100644 --- a/docs/internals/command.html.in +++ b/docs/internals/command.html.in @@ -348,9 +348,11 @@ </pre> <p> - Once the command has finished executing, these buffers - will contain the output, or be NULL if there was no output. It - is the callers responsibility to free these buffers. + Once the command has finished executing, these buffers will + contain the output. If there was no output but virCommandRun + was successful, then they will be an empty string; if there was + an error, then they might still be NULL. It is the callers + responsibility to free these buffers. </p> <h3><a name="directory">Setting working directory</a></h3> diff --git a/src/util/command.c b/src/util/command.c index 1923799..4fb5048 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -860,6 +860,20 @@ virCommandProcessIO(virCommandPtr cmd) } } + /* If there was no output, the populate with the empty string. */ + if (cmd->outbuf && !*cmd->outbuf) { + if ((*cmd->outbuf = strdup("")) == NULL) { + virReportOOMError(); + return -1; + } + } + if (cmd->errbuf && !*cmd->errbuf) { + if ((*cmd->errbuf = strdup("")) == NULL) { + virReportOOMError(); + return -1; + } + } + return 0; } -- 1.7.3.2
On Fri, Dec 03, 2010 at 02:28:51PM -0700, Eric Blake wrote:
Option 2. Which one should I go with? By the way, my pending patch for converting openvz to use virCommand instead of popen is impacted (it has a potential null dereference if we go with option 1).
* docs/internals/command.html.in: Update documentation. * src/util/command.c (virCommandSetOutputBuffer) (virCommandSetErrorBuffer, virCommandProcessIO) Guarantee empty string on no output. --- docs/internals/command.html.in | 8 +++++--- src/util/command.c | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-)
The virCommand API is all about safety, so ACK to this one. Daniel
On 04/12/2010, at 8:28 AM, Eric Blake wrote:
Option 1: This patch (all callers have to worry about NULL buffers, but checking for output is a simple pointer check).
Option 2: Guarantee that outbuf/errbuf are allocated, even if to the empty string. Caller always has to free the result, and empty output check requires checking if *outbuf=='\0'.
Personally, I prefer option 2. Thoughts?
2 seems safer.
2010/12/4 Justin Clift <jclift@redhat.com>:
On 04/12/2010, at 8:28 AM, Eric Blake wrote:
Option 1: This patch (all callers have to worry about NULL buffers, but checking for output is a simple pointer check).
Option 2: Guarantee that outbuf/errbuf are allocated, even if to the empty string. Caller always has to free the result, and empty output check requires checking if *outbuf=='\0'.
Personally, I prefer option 2. Thoughts?
2 seems safer.
I vote for the second version too. Matthias
On Sun, Dec 05, 2010 at 12:55:51AM +0100, Matthias Bolte wrote:
2010/12/4 Justin Clift <jclift@redhat.com>:
On 04/12/2010, at 8:28 AM, Eric Blake wrote:
Option 1: This patch (all callers have to worry about NULL buffers, but checking for output is a simple pointer check).
Option 2: Guarantee that outbuf/errbuf are allocated, even if to the empty string. Caller always has to free the result, and empty output check requires checking if *outbuf=='\0'.
Personally, I prefer option 2. Thoughts?
2 seems safer.
I vote for the second version too.
me too :-) ACK on 2/2 Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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
2010/12/7 Eric Blake <eblake@redhat.com>:
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/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");
The error message doesn't fit to the *outbuf != \0 test.
+ 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");
The error message doesn't fit to the *{out|err}buf != \0 test. Also what about the case when running /bin/true prints to {std|err}out for some reason? :) ACK. Matthias
On 12/07/2010 03:17 PM, Matthias Bolte wrote:
+ if (!outbuf || *outbuf) { + puts("output string not allocated");
The error message doesn't fit to the *outbuf != \0 test.
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; }
Also what about the case when running /bin/true prints to {std|err}out for some reason? :)
Then you deserve a 'make check' failure, to force you to figure out why your system is hosed. :)
ACK.
Thanks for the review; pushing soon. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Justin Clift -
Matthias Bolte