[PATCH xlsatoms] Count % signs, require at least 2 for a format string (#39614)

[Peter Hutterer]

On Mon, Jul 15, 2013 at 06:57:21PM +0200, Matthieu Herrb wrote:
> On Mon, Jul 15, 2013 at 02:07:35PM +1000, Peter Hutterer wrote:
> > xlsatoms -format "%s" sounds like a good idea, the resulting crash isn't.
> > I'm too lazy to check for all possible combinations that we allow here (it
> > is a printf-compatible string), so let's just check that we have two
> > specifiers %.
> > 
> > X.Org Bug 39614 <http://bugs.freedesktop.org/show_bug.cgi?id=39614>
> > 
> 
> Hi,
> 
> I'm not sure this is a useful fix. I think it should either be left as
> it currently is (and add some words in the man page saying that it's
> easy to crash it using incorrect format strings)
> Or write the full patch that checks that there is exactly one %d (or
> equivalent) and one %s (or equivalent) in that order in the format
> string.

I thought about writing the full patch but it's a bit more involved than
xlsatoms really warrants given that we'd not only have to support %d/u/x/...
but also all possible length modifiers. That's more likely to give us false
positives. and more time than I want to spend on this.

> Moreover your patch does not even catch the 2nd use of 'format'
> directly as printf argument in do_names().

right, that I should fix, but tbh I'm just as happy to ignore this.

Cheers,
   Peter
 
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > This isn't meant as a security fix, just as a mere sanity fix for a
> > simple-to-detect case.
> > 
> >  xlsatoms.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/xlsatoms.c b/xlsatoms.c
> > index eb4e21d..f21fadd 100644
> > --- a/xlsatoms.c
> > +++ b/xlsatoms.c
> > @@ -196,6 +196,20 @@ say_batch(xcb_connection_t *c, const char *format, xcb_get_atom_name_cookie_t *c
> >      char atom_name[1024];
> >      long i;
> >      int done = 0;
> > +    int format_specifiers = 0;
> > +
> > +    i = 0;
> > +
> > +    while(i < strlen(format) - 1) {
> > +        if (format[i] == '%' && format[++i] != '%')
> > +            format_specifiers++;
> > +        i++;
> > +    }
> > +
> > +    if (format_specifiers != 2) {
> > +        fprintf(stderr, "Invalid format specifier: '%s'. Need %%d and %%s.\n", format);
> > +        return 1;
> > +    }
> >  
> >      for (i = 0; i < count; i++)
> >  	cookie[i] = xcb_get_atom_name(c, i + low);
> > -- 
> > 1.8.2.1
> > 
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 
> -- 
> Matthieu Herrb