Re: [Libguestfs] [PATCH nbdkit v3 6/6] server: debug: Escape debug strings
On 5/9/23 22:28, Richard W.M. Jones wrote: > > Thanks for everyone's feedback. I pushed this series with several > enhancements as: > > 5fbf93a92..2f7ca818f Thankfully, this time I *did* pull before continuing with the review, and so I noticed the commits, and then read the rest of the thread too. Thanks! :) Laszlo ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit v3 6/6] server: debug: Escape debug strings
Thanks for everyone's feedback. I pushed this series with several enhancements as: 5fbf93a92..2f7ca818f 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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit v3 6/6] server: debug: Escape debug strings
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 (, ); > > - if (fp == NULL) > > + /* The "inner" string is the debug string before escaping. */ > > + fp_inner = open_memstream (_inner, _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 (_outer, _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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
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 (, ); > - if (fp == NULL) > + /* The "inner" string is the debug string before escaping. */ > + fp_inner = open_memstream (_inner, _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 (_outer, _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 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH nbdkit v3 6/6] server: debug: Escape debug strings
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 (, ); - if (fp == NULL) + /* The "inner" string is the debug string before escaping. */ + fp_inner = open_memstream (_inner, _inner); + if (fp_inner == NULL) goto fail; + errno = err; + vfprintf (fp_inner, fs, args); + close_memstream (fp_inner); + + /* The "outer" string contains the prologue, escaped debug string and \n. */ + fp_outer = open_memstream (_outer, _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) goto fail; - if (!str) + if (!str_outer) goto fail; /* Send it to stderr as atomically as possible. */ - fputs (str, stderr); + fputs (str_outer, stderr); errno = err; return; -- 2.39.2 ___ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
