Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)

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

> On Jul 13, 2023, at 4:31 PM, Kees Cook <keescook@chromium.org> wrote:
> 
> In the bug, the problem is that "p" isn't known to be allocated, if I'm
> reading that correctly?

I think that the major point in PR109557 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557):

for the following pointer p.3_1, 

p.3_1 = p;
_2 = __builtin_object_size (p.3_1, 0);

Question: why the size of p.3_1 cannot use the TYPE_SIZE of the pointee of p when the TYPE_SIZE can be determined at compile time?

Answer:  From just knowing the type of the pointee of p, the compiler cannot determine the size of the object.  

Therefore the bug has been closed. 

In your following testing 5:

> I'm not sure this is a reasonable behavior, but
> let me get into the specific test, which looks like this:
> 
> TEST(counted_by_seen_by_bdos)
> {
>        struct annotated *p;
>        int index = MAX_INDEX + unconst;
> 
>        p = alloc_annotated(index);
> 
>        REPORT_SIZE(p->array);
> /* 1 */ EXPECT_EQ(sizeof(*p), offsetof(typeof(*p), array));
>        /* Check array size alone. */
> /* 2 */ EXPECT_EQ(__builtin_object_size(p->array, 1), SIZE_MAX);
> /* 3 */ EXPECT_EQ(__builtin_dynamic_object_size(p->array, 1), p->foo * sizeof(*p->array));
>        /* Check check entire object size. */
> /* 4 */ EXPECT_EQ(__builtin_object_size(p, 1), SIZE_MAX);
> /* 5 */ EXPECT_EQ(__builtin_dynamic_object_size(p, 1), sizeof(*p) + p->foo * sizeof(*p->array));
> }
> 
> Test 5 should pass as well, since, again, p can be examined. Passing p
> to __bdos implies it is allocated and the __counted_by annotation can be
> examined.

Since the call to the routine “alloc_annotated" cannot be inlined, GCC does not see any allocation calls for the pointer p.
At the same time, due to the same reason as PR109986, GCC cannot determine the size of the object by just knowing the TYPE_SIZE
of the pointee of p.  

So, this is exactly the same issue as PR109557.  It’s an existing behavior per the current __buitlin_object_size algorithm.
I am still not very sure whether the situation in PR109557 can be improved or not, but either way, it’s a separate issue.

Please see the new testing case I added for PR109557, you will see that the above case 5 is a similar case as the new testing case in PR109557.

> 
> If "return p->array[index];" would be caught by the sanitizer, then
> it follows that __builtin_dynamic_object_size(p, 1) must also know the
> size. i.e. both must examine "p" and its trailing flexible array and
> __counted_by annotation.
> 
>> 
>> 2. The common issue for  the failed testing 3, 4, 9, 10 is:
>> 
>> for the following annotated structure: 
>> 
>> ====
>> struct annotated {
>>        unsigned long flags;
>>        size_t foo;
>>        int array[] __attribute__((counted_by (foo)));
>> };
>> 
>> 
>> struct annotated *p;
>> int index = 16;
>> 
>> p = malloc(sizeof(*p) + index * sizeof(*p->array));  // allocated real size 
>> 
>> p->foo = index + 2;  // p->foo was set by a different value than the real size of p->array as in test 9 and 10
> 
> Right, tests 9 and 10 are checking that the _smallest_ possible value of
> the array is used. (There are two sources of information: the allocation
> size and the size calculated by counted_by. The smaller of the two
> should be used when both are available.)

The counted_by attribute is used to annotate a Flexible array member on how many elements it will have.
However, if this information can not accurately reflect the real number of elements for the array allocated, 
What’s the purpose of such information? 

>> or
>> p->foo was not set to any value as in test 3 and 4
> 
> For tests 3 and 4, yes, this was my mistake. I have fixed up the tests
> now. Bill noticed the same issue. Sorry for the think-o!
> 
>> ====
>> 
>> i.e, the value of p->foo is NOT synced with the number of elements allocated for the array p->array.  
>> 
>> I think that this should be considered as an user error, and the documentation of the attribute should include
>> this requirement.  (In the LLVM’s RFC, such requirement was included in the programing model: 
>> https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18)
>> 
>> We can add a new warning option -Wcounted-by to report such user error if needed.
>> 
>> What’s your opinion on this?
> 
> I think it is correct to allow mismatch between allocation and
> counted_by as long as only the least of the two is used.

What’s your mean by “only the least of the two is used”?

> This may be
> desirable in a few situations. One example would be a large allocation
> that is slowly filled up by the program.

So, for such situation, whenever the allocation is filled up, the field that hold the “counted_by” attribute should be increased at the same time,
Then, the “counted_by” value always sync with the real allocation. 
> I.e. the counted_by member is
> slowly increased during runtime (but not beyond the true allocation size).

Then there should be source code to increase the “counted_by” field whenever the allocated space increased too. 
> 
> Of course allocation size is only available in limited situations, so
> the loss of that info is fine: we have counted_by for everything else.

The point is: allocation size should synced with the value of “counted_by”. LLVM’s RFC also have the similar requirement:
https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18

Qing
> 
> -- 
> Kees Cook