ubsan: remove bogus check for flexible array members

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?


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

	Marek

On Thu, Feb 26, 2015 at 8:32 AM, Marek Polacek <polacek@redhat.com> wrote:
> 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.)

Btw, I've seen struct S { int i; char a[4]; }; as well.

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

Err - this doesn't detect

int
main (void)
{
  int *t = (int *) __builtin_malloc (sizeof (int) * 10);
  int (*a)[1] = (int (*)[1])t;
  (*a)[2] = 1;
}

that is a trailing array VLA.

What I've definitely seen is

int
main (void)
{
  int *t = (int *) __builtin_malloc (sizeof (int) * 9);
  int (*a)[3][3] = (int (*)[3][3])t;
  (*a)[0][9] = 1;
}

that is, assume that the array dimension with the fast running
index "wraps" over into the next (hello SPEC CPU 2006!).

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

Not sure about this.

Richard.

>         Marek

On Thu, Feb 26, 2015 at 11:08:04AM +0100, Richard Biener wrote:
> > --- 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
> 
> Err - this doesn't detect
> 
> int
> main (void)
> {
>   int *t = (int *) __builtin_malloc (sizeof (int) * 10);
>   int (*a)[1] = (int (*)[1])t;
>   (*a)[2] = 1;
> }
> 
> that is a trailing array VLA.
> 
> What I've definitely seen is
> 
> int
> main (void)
> {
>   int *t = (int *) __builtin_malloc (sizeof (int) * 9);
>   int (*a)[3][3] = (int (*)[3][3])t;
>   (*a)[0][9] = 1;
> }
 
I think we should error on those.
With my patch we'd emit the same -fsanitize=bounds runtime errors as clang
does.

> that is, assume that the array dimension with the fast running
> index "wraps" over into the next (hello SPEC CPU 2006!).
 
I think they're invoking UB then.

> > +      && 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.
> 
> Not sure about this.

The comment says that we're trying to detect a flexible array member
there - and those can't be outside struct.  I certainly hadn't anything
else in my mind when I was writing that ;).

	Marek