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. 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