On 08/04/2016 03:13 PM, Martin Sebor wrote:
On 07/21/2016 03:48 PM, Jeff Law wrote:
Sorry about the delay in responding...
I saw a few places in GCC itself where you increased buffer sizes. Were
any of those level 1 failures?
Yes. With optimization, even level 1 uses range information when
it's available. The idea is that when the range of values of the
variable has been constrained (possibly to avoid a sprintf buffer
overflow) but wasn't constrained enough, it's a possible sign of
a bug.
In some cases such as when the resulting range is the result of
a cast from signed to unsigned (as in cp/mangle.c) it triggers
a diagnostic that wouldn't be issued in its absence. Here's
a small example:
char d [9];
void f (long *x)
{
// warning here
__builtin_sprintf (d, "%08lx", (unsigned long)*x);
}
void g (long *x)
{
// no warning here
__builtin_sprintf (d, "%08lx", *x);
}
It's probably possible to tweak the pass to try to detect this
case and avoid the warning, but I'm not sure it's worth it. What
do you think?
I don't think it's worth it to detect and avoid the warning right now.
That may change as the warning sees more widespread use after going onto
the trunk.
Right. I'm not sure what the right solution is. The biggest
difference between the levels is in the values they assume
unknown integers can take on. Level one assumes 1, level two
assumes a value that results in the longest output (such as
INT_MIN). This can cause false positives but in my experience
there aren't too many of those. I would be comfortable making
level 2 the default if we had good range information. Without
it, though, even some obviously safe cases tend to be diagnosed,
like this one:
char d [4];
void g (int i)
{
if (0 <= i && i < 1000)
__builtin_sprintf (d, "%i", i);
}
warning: ‘%i’ directive writing between 1 and 11 bytes into a region
of size 4 [-Wformat-length=]
__builtin_sprintf (d, "%i", i);
^~
note: using the range [‘1’, ‘-2147483648’] for directive argument
note: format output between 2 and 12 bytes into a destination of size 4
Maybe leave level 1 the default for now and bump it up to 2 when
the new and improved VRP pass is in?
This is probably as much of a user education problem as anything. I
definitely want us to be level 1 clean for gcc-7. I think we'll want to
evaluate the pain of getting to level 2 clean once Andrew's work is far
enough along to evaluate how many false positives it allows us to eliminate.
jeff
