This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] ubsan: remove bogus check for flexible array members


On Thu, Feb 26, 2015 at 12:59:08AM -0800, Martin Uecker wrote:
> 
> Thu, 26 Feb 2015 07:36:54 +0100
> Jakub Jelinek <jakub@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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]