This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] enhance buffer overflow warnings (and c/53562)
On 11/02/2016 01:37 AM, Jakub Jelinek wrote:
On Tue, Nov 01, 2016 at 08:55:03PM -0600, Martin Sebor wrote:
struct S {
int a, b, c, d;
};
#define bos(p, t) __builtin_object_size (p, t)
#define memset0(p, i, n) __builtin___memset_chk (p, i, n, bos (p, 0))
#define memset1(p, i, n) __builtin___memset_chk (p, i, n, bos (p, 1))
void f0 (struct S *s)
{
memset0 (&s->d, 0, 1024); // no warning here (bos 0)
}
But we do not want the warning here, there is nothing wrong on it.
The caller may be
void
bar (void)
{
struct T { struct S header; char payload[1024 - offsetof (struct S, d)]; } t;
initialize (&t);
f0 (&t.header);
}
and the callee might rely on that.
Sure, they might and in that case the warning would be a false
positive. It wouldn't be the first such warning that wasn't 100%
free of them. But my testing with Binutils, GCC, and the Linux
kernel has exposed only 10 instances of new warnings and I don't
think I saw this idiom among them. But even if some were, or if
all of them were false positives I think that would be well within
the acceptable rates. Here are the numbers of warnings for
Binutils and the kernel:
113 -Wimplicit-fallthrough
38 -Wformat-length=
12 -Wunused-const-variable=
10 -Wstringop-overflow
2 -Wdangling-else
2 -Wframe-address
2 -Wint-in-bool-context
1 -Wbool-operation
Using some header structure at the
beginning and then conditionally on fields in that structure various
payloads occurs in many projects, starting with glibc, gcc, Linux kernel,
... The warning really must not be detached from reality.
That's an unfair assertion in light of the numbers above.
If you want a warning for suspicious calls, sure, but
1) it has to be clearly worded significantly differently from how do you
word it, so that users really understand you are warning about
suspicious code (though, I really believe it is very common and there
is really nothing the users can do about it)
2) it really shouldn't be enabled in -Wall, and I think not even in -Wextra
As you have argued yourself recently in our discussion of
-Wimplicit-fallthrough, warnings that aren't enabled by either
of these options don't generally benefit users because very few
turn them on explicitly. I agree with that argument although I
would be in favor of rolling out a new warning gradually over
two or more releases if it were known to be prone to high rates
of false positive. The -Wstringop-overflow warning clearly isn't
in that category so there's no need for it. My offer to do
additional testing is still good if you'd like to see more data.
As for the wording, I welcome suggestions for improvements.
Martin