Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

[Qing Zhao <qing.zhao@oracle.com>]

> On Jan 26, 2024, at 3:04 AM, Martin Uecker <uecker@tugraz.at> wrote:
> 
> 
> I haven't looked at the patch, but it sounds you give the result
> the wrong type. Then patching up all use cases instead of the
> type seems wrong.

Yes, this is for resolving a very early gimplification issue as I reported last Nov:
https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638793.html

Since no-one responded at that time, I fixed the issue by replacing the ARRAY_REF
With a pointer indirection:
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639605.html

The reason for such change is:  return a flexible array member TYPE is not allowed
by C language (our gimplification follows this rule), so, we have to return a pointer TYPE instead. 

******The new internal function

 .ACCESS_WITH_SIZE (REF_TO_OBJ, REF_TO_SIZE, CLASS_OF_SIZE, SIZE_OF_SIZE, ACCESS_MODE, INDEX)

INTERNAL_FN (ACCESS_WITH_SIZE, ECF_LEAF | ECF_NOTHROW, NULL)

which returns the "REF_TO_OBJ" same as the 1st argument;

Both the return type and the type of the first argument of this function have been converted from 
the incomplete array type to the corresponding pointer type.

As a result, the original ARRAY_REF was converted to an INDIRECT_REF, the original INDEX of the ARRAY_REF was lost
when converting from ARRAY_REF to INDIRECT_REF, in order to keep the INDEX for bound sanitizer instrumentation, I added
The 6th argument “INDEX”.

What’s your comment and suggestion on this solution?

Thanks.

Qing


> 
> Martin
> 
> 
> Am Donnerstag, dem 25.01.2024 um 20:11 +0000 schrieb Qing Zhao:
>> Thanks a lot for the testing.
>> 
>> Yes, I can repeat the issue with the following small example:
>> 
>> #include <stdlib.h>
>> #include <stddef.h>
>> #include <stdio.h>
>> 
>> #define MAX(a, b)  ((a) > (b) ? (a) :  (b))
>> 
>> struct untracked {
>>       int size;
>>       int array[] __attribute__((counted_by (size)));
>> } *a;
>> struct untracked * alloc_buf (int index)
>> {
>>  struct untracked *p;
>>  p = (struct untracked *) malloc (MAX (sizeof (struct untracked),
>>                                        (offsetof (struct untracked, array[0])
>>                                         + (index) * sizeof (int))));
>>  p->size = index;
>>  return p;
>> }
>> 
>> int main()
>> {
>>  a = alloc_buf(10);
>>     printf ("same_type is %d\n",
>>  (__builtin_types_compatible_p(typeof (a->array), typeof (&(a->array)[0]))));
>>  return 0;
>> }
>> 
>> 
>> /home/opc/Install/latest-d/bin/gcc -O2 btcp.c
>> same_type is 1
>> 
>> Looks like that the “typeof” operator need to be handled specially in C FE
>> for the new internal function .ACCESS_WITH_SIZE. 
>> 
>> (I have specially handle the operator “offsetof” in C FE already).
>> 
>> Will fix this issue.
>> 
>> Thanks.
>> 
>> Qing
>> 
>>> On Jan 24, 2024, at 7:51 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 
>>> On Wed, Jan 24, 2024 at 12:29:51AM +0000, Qing Zhao wrote:
>>>> This is the 4th version of the patch.
>>> 
>>> Thanks very much for this!
>>> 
>>> I tripped over an unexpected behavioral change that the Linux kernel
>>> depends on:
>>> 
>>> __builtin_types_compatible_p() no longer treats an array marked with
>>> counted_by as different from that array's decayed pointer. Specifically,
>>> the kernel uses these macros:
>>> 
>>> 
>>> /*
>>> * Force a compilation error if condition is true, but also produce a
>>> * result (of value 0 and type int), so the expression can be used
>>> * e.g. in a structure initializer (or where-ever else comma expressions
>>> * aren't permitted).
>>> */
>>> #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
>>> 
>>> #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
>>> 
>>> /* &a[0] degrades to a pointer: a different type from an array */
>>> #define __must_be_array(a)   BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
>>> 
>>> 
>>> This gets used in various places to make sure we're dealing with an
>>> array for a macro:
>>> 
>>> #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>>> 
>>> 
>>> So this builds:
>>> 
>>> struct untracked {
>>>       int size;
>>>       int array[];
>>> } *a;
>>> 
>>> __must_be_array(a->array)
>>> => 0 (as expected)
>>> __builtin_types_compatible_p(typeof(a->array), typeof(&(a->array)[0]))
>>> => 0 (as expected, array vs decayed array pointer)
>>> 
>>> 
>>> But if counted_by is added, we get a build failure:
>>> 
>>> struct tracked {
>>>       int size;
>>>       int array[] __counted_by(size);
>>> } *b;
>>> 
>>> __must_be_array(b->array)
>>> => build failure (not expected)
>>> __builtin_types_compatible_p(typeof(b->array), typeof(&(b->array)[0]))
>>> => 1 (not expected, both pointers?)
>>> 
>>> 
>>> 
>>> 
>>> -- 
>>> Kees Cook
>> 
>