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 <[email protected]> > > --- > > 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 > > > > _______________________________________________ > > [email protected]: X.Org development > > Archives: http://lists.x.org/archives/xorg-devel > > Info: http://lists.x.org/mailman/listinfo/xorg-devel > > -- > Matthieu Herrb _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
