Re: [PATCH] - improve sprintf buffer overflow detection (middle-end/49905)

[Jeff Law <law at redhat dot com>]

I saw a few places in GCC itself where you increased buffer sizes.  Were 
any of those level 1 failures?

In c.opt:

> +C ObjC C++ ObjC++ Warning Alias (Wformat-length=, 1, 0)
Can't this take on values 0, 1, 2? And if so, is the "1" above correct?

In invoke.texi we have:

+-Wno-format-contains-nul -Wno-format-extra-args -Wformat-length=2 @gol
Note Wformat-length=2.

Then in the actual documentation it says the default level is 1.
> +in output truncation.  GCC counts the number of bytes that each format
> +string and directive within it writes into the provided buffer and, when
> +it detects that more bytes that fit in the destination buffer may be output,
> +it emits a warning.

s/that fit/than fit

In that same section is the text which says the default level is 1

I'm curious about the heuristics for level 1.  I guess if you get a 
warning at level 1, then you've got a situation where we know we'll 
write out of bounds.  level 2 is a may write out of bounds.  Which makes 
me wonder if rather than levels we should use symbolic names?

I guess my worry here is that if we don't message this right folks will 
make their code level 1 clean and think they're done.  When in reality 
all they've done is mitigate the most egregious problems.

Elsewhere in invoke.texi:

> +buffer in an inforational note following the warning.
informational I assume

> +the minimum buffer size.  For exampe, if @var{a} and @var{b} above can
example

In genmatch.c:
> +      char id[13];   /* Big enough for a 32-bit UINT_MAX.  */
In general we don't add comments to the end of a line like that.  If a 
comment is needed, put it before the declaration.  I'm not sure if a 
comment is needed here or not -- your call.

Similarly for various other places were you increased buffer lengths.


In passes.c:

> +
> +      // inform (0, "executing pass: %s", pass->name);
> +
>        if (execute_one_pass (pass) && pass->sub)
> -        execute_pass_list_1 (pass->sub);
> +       {
> +         // inform (0, "executing subpass: %s", pass->sub->name);
> +         execute_pass_list_1 (pass->sub);
> +       }
> +


The comments left over from debugging?

Looks like ~1/3 of the patch are tests.  Good stuff.  I'm going to 
assume those are correct.

I'll get into gimple-ssa-sprintf.c either later tonight or tomorrow. 
But so far nothing major, just nits.

Jeff