[Libguestfs] [PATCH 2/2] daemon: Always reflect command stderr to stderr when debugging.

[Jim Meyering]

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.