[Libguestfs] [PATCH nbdkit v3 6/6] server: debug: Escape debug strings

[Eric Blake]

On Tue, May 09, 2023 at 03:51:21PM +0100, Richard W.M. Jones wrote:
> Debug strings contain all kinds of information including some under
> user control.  Previously we simply sent everything to stderr, but
> this is potentially insecure, as well as not dealing well with
> non-printable characters.  Escape these strings when printing.
> ---
>  server/debug.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/server/debug.c b/server/debug.c
> index 177d9d5da..6cf1af316 100644
> --- a/server/debug.c
> +++ b/server/debug.c
> @@ -42,6 +42,7 @@
>  
>  #include "ansi-colours.h"
>  #include "open_memstream.h"
> +#include "utils.h"
>  
>  #include "internal.h"
>  
> @@ -76,36 +77,43 @@ debug_common (bool in_server, const char *fs, va_list args)
>  #ifndef WIN32
>    int tty;
>  #endif
> -  CLEANUP_FREE char *str = NULL;
> -  size_t len = 0;
> -  FILE *fp;
> +  CLEANUP_FREE char *str_inner = NULL;
> +  CLEANUP_FREE char *str_outer = NULL;
> +  FILE *fp_inner, *fp_outer;
> +  size_t len_inner = 0, len_outer = 0;
>  
> -  fp = open_memstream (&str, &len);
> -  if (fp == NULL)
> +  /* The "inner" string is the debug string before escaping. */
> +  fp_inner = open_memstream (&str_inner, &len_inner);
> +  if (fp_inner == NULL)
>      goto fail;
> +  errno = err;
> +  vfprintf (fp_inner, fs, args);
> +  close_memstream (fp_inner);

Missing the check for failure.

> +
> +  /* The "outer" string contains the prologue, escaped debug string and \n. */
> +  fp_outer = open_memstream (&str_outer, &len_outer);
> +  if (fp_outer == NULL) goto fail;
>  
>  #ifndef WIN32
>    tty = isatty (fileno (stderr));
> -  if (!in_server && tty) ansi_force_colour (ANSI_FG_BOLD_BLACK, fp);
> +  if (!in_server && tty) ansi_force_colour (ANSI_FG_BOLD_BLACK, fp_outer);
>  #endif
>  
> -  prologue (fp);
> -
> -  errno = err;
> -  vfprintf (fp, fs, args);
> +  prologue (fp_outer);
> +  c_string_quote (str_inner, fp_outer);
>  
>  #ifndef WIN32
> -  if (!in_server && tty) ansi_force_restore (fp);
> +  if (!in_server && tty) ansi_force_restore (fp_outer);
>  #endif
> -  fprintf (fp, "\n");
> -  if (close_memstream (fp) == -1)
> +  fprintf (fp_outer, "\n");
> +  if (close_memstream (fp_outer) == -1)

Affects multiple patches: close_memstream() (on Linux) fails with EOF,
which happens to be -1 on all sane libc, but which POSIX says can be
any negative value.  '< 0' or at least '== EOF' is probably safer than
'== -1'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org