This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] ubsan: remove bogus check for flexible array members
- From: Martin Uecker <uecker at eecs dot berkeley dot edu>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: gcc Mailing List <gcc-patches at gcc dot gnu dot org>, Marek Polacek <polacek at redhat dot com>, Joseph Myers <joseph at codesourcery dot com>
- Date: Thu, 26 Feb 2015 00:59:08 -0800
- Subject: Re: [PATCH] ubsan: remove bogus check for flexible array members
- Authentication-results: sourceware.org; auth=none
- References: <20150225220107 dot 5ce6e4a5 at lemur> <20150226063654 dot GY1746 at tucnak dot redhat dot com>
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.
If this is indeed intentional, we should:
- document these exceptions
- remove the misleading comment
- have test cases also for sizes > 0
- fix it not to prevent instrumentation of all array accesses
through pointers (i.e. when the array is not a member of a struct)
- have a configuration option (e.g. -fsanitize=strict-bounds)
> I think we were talking at some point about having a strict-bounds or
> something similar that would accept only real flexible array members and
> nothing else and be more pedantic, at the disadvantage of diagnosing tons
> of real-world code that is supported by GCC.
>
> Perhaps if the reference is just an array, not a struct containing
> flexible-array-member-like array, we could consider it strict always,
> but that is certainly not what your patch does.
Indeed, currently the patch tries to make -fsanitize=bounds detect what
the standard considers undefined behaviour. At the moment, I do not
quite see why ubsan should be so permissive as you say.
But we can also add a new option -fsanitize=strict-bounds. Then I could
use this for my (real-world) projects.
> > * c-family/c-ubsan.c (ubsan_instrument_bounds): Remove
>
> c-family/ shouldn't be in the ChangeLog entry, instead that part should go
> into c-family/ChangeLog.
> > --- a/gcc/c-family/c-ubsan.c
> > +++ b/gcc/c-family/c-ubsan.c
> > @@ -293,6 +293,7 @@ ubsan_instrument_bounds (location_t loc, tree array, tree *index,
> > tree type = TREE_TYPE (array);
> > tree domain = TYPE_DOMAIN (type);
> >
> > + /* This also takes care of flexilbe array members */
>
> Typo.
>
> > +#ifdef __cplusplus
> > +extern "C" void* malloc(unsigned long x);
> > +#else
> > +extern void* malloc(unsigned long x);
> > +#endif
>
> Why are you declaring malloc (wrongly), when it isn't used?
> Malloc argument is size_t aka __SIZE_TYPE__, not unsigned long.
Left over from removing tests already done elsewhere.
Thank you for your comments,
Martin