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

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'.