11:33 a.m.
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
11:35 a.m.
New subject: [PATCH 1/2] daemon: Add flags argument to command*() functions.
--
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
11:36 a.m.
New subject: [PATCH 2/2] daemon: Always reflect command stderr to stderr when debugging.
--
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
4:35 a.m.
New subject: [PATCH 2/2] daemon: Always reflect command stderr to stderr when debugging.
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.
4:40 a.m.
New subject: [PATCH 2/2] daemon: Always reflect command stderr to stderr when debugging.
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
5:14 a.m.
New subject: [PATCH 2/2] daemon: Always reflect command stderr to stderr when debugging.
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.
5475
days inactive
5478
days old
5 comments
2 participants
participants (2)
-
Jim Meyering -
Richard W.M. Jones