On Thu, Feb 26, 2015 at 12:59:08AM -0800, Martin Uecker wrote:
>
> Thu, 26 Feb 2015 07:36:54 +0100
> Jakub Jelinek <ja...@redhat.com>:
> > On Wed, Feb 25, 2015 at 10:01:07PM -0800, Martin Uecker wrote:
> > > this patch removes a bogus check for flexible array members
> > > which prevents array references to be instrumented in some
> > > interesting cases. Arrays accessed through pointers are now
> > > instrumented correctly.
> > >
> > > The check was unnecessary because flexible arrays are not
> > > instrumented anyway because of
> > > TYPE_MAX_VALUE (domain) == NULL_TREE.
> >
> > No, it is not bogus nor unnecessary.
> > This isn't about just real flexible arrays, but similar constructs,
> > C++ doesn't have flexible array members, nor C89, so people use the
> > GNU extension of struct S { ... ; char a[0]; } instead, or
>
> The GNU extension is still allowed, i.e. not instrumented with
> the patch.
>
> > use char a[1]; as the last member and still expect to be able to access
> > s->a[i] for i > 0 say on heap allocations etc.
>
> And this is broken code. I would argue that a user who uses the
> ubsan *expects* this to be diagnosed. Atleast I was surprised
> that it didn't catch more out-of-bounds accesses.
I wouldn't say broken, it's being used pretty often. E.g.
https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
"In ISO C90, you would have to give contents a length of 1"
> If this is indeed intentional, we should:
>
> - document these exceptions
See the link above + docs say for -fsanitize=bounds that "Flexible array
members are not instrumented".
> - remove the misleading comment
Which one?
The /* Detect flexible array members and suchlike. */ IMHO says
exactly what's meant to be done.
> - have test cases also for sizes > 0
Yes, we should have that.
> - fix it not to prevent instrumentation of all array accesses
> through pointers (i.e. when the array is not a member of a struct)
Yes, see the patch in my previous mail.
> - have a configuration option (e.g. -fsanitize=strict-bounds)
Yeah, something like that would be useful to have. As Jakub
mentioned, we discussed this in the past. Probably GCC 6 stuff
though.
Marek
