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

[Matthieu Herrb]

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.

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

> 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