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

[Richard W.M. Jones]

On Tue, May 09, 2023 at 01:36:13PM -0500, Eric Blake wrote:
> 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'.

I should also change the replacement function to return EOF
(also defined as -1 on MinGW).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW