[PATCH 0/2] Two small fixes to command*() functions in the daemon
The command*() functions are sane wrappers we use in the daemon to run external commands, and therefore very important. These two patches enhance these functions in useful ways. (Note these are an internal interface which we can change at any time). The first patch adds variations which take a flags parameter, and implements a useful flag (for dealing with parted). The second patch stops command*() from eating stderr when the stderror parameter is NULL and we're in debug mode. Currently vital logging and error information is being lost in certain circumstances, and this patch resolves this. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
Richard W.M. Jones wrote:
Subject: [PATCH 2/2] daemon: Always reflect command stderr to stderr when debugging.
When debugging (ie. LIBGUESTFS_DEBUG=1 & verbose flag set in daemon) always reflect any stderr output from commands that we run to stderr of the daemon, so it is visible.
Previously if stderror == NULL in command*, stderr output was just eaten and discarded which meant useful error messages could be lost. --- daemon/guestfsd.c | 25 ++++++++++++++++--------- 1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index bf06c73..370eea8 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -38,6 +38,8 @@ #include <printf.h>
#include "c-ctype.h" +#include "ignore-value.h" + #include "daemon.h"
static char *read_cmdline (void); @@ -742,15 +744,20 @@ commandrvf (char **stdoutput, char **stderror, int flags, } if (r == 0) { FD_CLR (se_fd[0], &rset); quit++; }
- if (r > 0 && stderror) { - se_size += r; - p = realloc (*stderror, se_size); - if (p == NULL) { - perror ("realloc"); - goto quit; - } - *stderror = p; - memcpy (*stderror + se_size - r, buf, r); + if (r > 0) { + if (verbose) + ignore_value (write (2, buf, r)); + + if (stderror) { + se_size += r; + p = realloc (*stderror, se_size); + if (p == NULL) { + perror ("realloc"); + goto quit; + } + *stderror = p; + memcpy (*stderror + se_size - r, buf, r);
Hi Rich, This change looks fine. However, there's an unrelated problem: if stderr is very large, incrementing se_size by "r" could result in a smaller total size (overflow). Then, the following memcpy would clobber the heap by writing a potentially very large amount of data into a much smaller (or even 0-length) buffer. One way to avoid it: size_t new_size = se_size + r; if (new_size < se_size) { perror ("standard error too large"); goto quit; } se_size = new_size; Or perhaps better: limit the buffer size to say 1MiB, and warn if truncation is required.
On Mon, Nov 09, 2009 at 11:35:55AM +0100, Jim Meyering wrote:
Richard W.M. Jones wrote:
+ if (stderror) { + se_size += r; + p = realloc (*stderror, se_size); + if (p == NULL) { + perror ("realloc"); + goto quit; + } + *stderror = p; + memcpy (*stderror + se_size - r, buf, r);
Hi Rich,
This change looks fine.
However, there's an unrelated problem: if stderr is very large, incrementing se_size by "r" could result in a smaller total size (overflow). Then, the following memcpy would clobber the heap by writing a potentially very large amount of data into a much smaller (or even 0-length) buffer.
One way to avoid it:
size_t new_size = se_size + r; if (new_size < se_size) { perror ("standard error too large"); goto quit; } se_size = new_size;
Or perhaps better: limit the buffer size to say 1MiB, and warn if truncation is required.
If I'm understand this correctly, se_size would have to be O(2GB) for this to be a problem? I guess we'd have run out of memory long before this because we only give the appliance ~500MB of memory, of which much less than that is usable. Or am I not understanding this right? In any case it won't hurt to add a check here. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
Richard W.M. Jones wrote:
On Mon, Nov 09, 2009 at 11:35:55AM +0100, Jim Meyering wrote:
Richard W.M. Jones wrote:
+ if (stderror) { + se_size += r; + p = realloc (*stderror, se_size); + if (p == NULL) { + perror ("realloc"); + goto quit; + } + *stderror = p; + memcpy (*stderror + se_size - r, buf, r);
Hi Rich,
This change looks fine.
However, there's an unrelated problem: if stderr is very large, incrementing se_size by "r" could result in a smaller total size (overflow). Then, the following memcpy would clobber the heap by writing a potentially very large amount of data into a much smaller (or even 0-length) buffer.
One way to avoid it:
size_t new_size = se_size + r; if (new_size < se_size) { perror ("standard error too large"); goto quit; } se_size = new_size;
Or perhaps better: limit the buffer size to say 1MiB, and warn if truncation is required.
If I'm understand this correctly, se_size would have to be O(2GB) for this to be a problem? I guess we'd have run out of memory long before this because we only give the appliance ~500MB of memory, of which much less than that is usable.
Right. For now, it's definitely only theoretical. And since r is never larger than 256 (sizeof buf), it'd take a long time to read that much data.
Or am I not understanding this right?
In any case it won't hurt to add a check here.
participants (2)
-
Jim Meyering -
Richard W.M. Jones