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

[Marek Polacek <polacek at redhat dot com>]

On Thu, Feb 26, 2015 at 07:36:54AM +0100, Jakub Jelinek wrote:
> 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
> 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.
 
Yes, in other words, your patch would make a difference here:

int
main (void)
{
  struct S { int i; char a[1]; };
  struct S *t = (struct S *) __builtin_malloc (sizeof (struct S) + 10);
  t->a[2] = 1;
}

(bounds-1.c test case should test this, too.)

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

Martin, can you try whether this (untested) does what you're after?

--- gcc/c-family/c-ubsan.c
+++ gcc/c-family/c-ubsan.c
@@ -303,8 +303,9 @@ ubsan_instrument_bounds (location_t loc, tree array, tree *index,
 
   /* Detect flexible array members and suchlike.  */
   tree base = get_base_address (array);
-  if (base && (TREE_CODE (base) == INDIRECT_REF
-	       || TREE_CODE (base) == MEM_REF))
+  if (TREE_CODE (array) == COMPONENT_REF
+      && base && (TREE_CODE (base) == INDIRECT_REF
+		  || TREE_CODE (base) == MEM_REF))
     {
       tree next = NULL_TREE;
       tree cref = array;

I think it is a bug that we're playing games on something that is not
an element of a structure.

	Marek