On Thu, Feb 26, 2015 at 8:32 AM, Marek Polacek <pola...@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
