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

[Martin Uecker via Gcc-patches]

I am sure this has been discussed before, but seeing that you
test for a specific formula, let me point out the following:

There at least three different size expression which could
make sense. Consider

short foo { int a; short b; char t[]; }; 

Most people seem to use

sizeof(struct foo) + N * sizeof(foo->t);

which for N == 3 yields 11 bytes on x86-64 because the formula
adds the padding of the original struct. There is an example
in the  C standard that uses this formula.


But he minimum size of an object which stores N elements is

max(sizeof (struct s), offsetof(struct s, t[n]))

which is 9 bytes. 

This is what clang uses for statically allocated objects with
initialization, while GCC uses the rule above (11 bytes). But 
bdos / bos  then returns the smaller size of 9 which is a bit
confusing.


https://godbolt.org/z/K1hvaK1ns

https://github.com/llvm/llvm-project/issues/62929
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109956


Then there is also the size of a similar array where the FAM
is replaced with an array of static size:

struct foo { int a; short b; char t[3]; }; 

This would make the most sense to me, but it has 12 bytes
because the padding is according to the usual alignment
rules.


Martin



Am Montag, dem 07.08.2023 um 09:16 -0700 schrieb Kees Cook:
> On Fri, Aug 04, 2023 at 07:44:28PM +0000, Qing Zhao wrote:
> > This is the 2nd version of the patch, per our discussion based on the
> > review comments for the 1st version, the major changes in this version
> > are:
> 
> Thanks for the update!
> 
> > 
> > 1. change the name "element_count" to "counted_by";
> > 2. change the parameter for the attribute from a STRING to an
> > Identifier;
> > 3. Add logic and testing cases to handle anonymous structure/unions;
> > 4. Clarify documentation to permit the situation when the allocation
> > size is larger than what's specified by "counted_by", at the same time,
> > it's user's error if allocation size is smaller than what's specified by
> > "counted_by";
> > 5. Add a complete testing case for using counted_by attribute in
> > __builtin_dynamic_object_size when there is mismatch between the
> > allocation size and the value of "counted_by", the expecting behavior
> > for each case and the explanation on why in the comments. 
> 
> All the "normal" test cases I have are passing; this is wonderful! :)
> 
> I'm still seeing unexpected situations when I've intentionally set
> counted_by to be smaller than alloc_size, but I assume it's due to not
> yet having the patch you mention below.
> 
> > As discussed, I plan to add two more separate patch sets after this initial
> > patch set is approved and committed.
> > 
> > set 1. A new warning option and a new sanitizer option for the user error
> >        when the allocation size is smaller than the value of "counted_by".
> > set 2. An improvement to __builtin_dynamic_object_size  for the following
> >        case:
> > 
> > struct A
> > {
> > size_t foo;
> > int array[] __attribute__((counted_by (foo)));
> > };
> > 
> > extern struct fix * alloc_buf ();
> > 
> > int main ()
> > {
> > struct fix *p = alloc_buf ();
> > __builtin_object_size(p->array, 0) == sizeof(struct A) + p->foo * 
> > sizeof(int);
> >   /* with the current algorithm, it’s UNKNOWN */ 
> > __builtin_object_size(p->array, 2) == sizeof(struct A) + p->foo * 
> > sizeof(int);
> >   /* with the current algorithm, it’s UNKNOWN */
> > }
> 
> Should the above be bdos instead of bos?
> 
> > Bootstrapped and regression tested on both aarch64 and X86, no issue.
> 
> I've updated the Linux kernel's macros for the name change and done
> build tests with my first pass at "easy" cases for adding counted_by:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=devel/counted_by&id=adc5b3cb48a049563dc673f348eab7b6beba8a9b
> 
> Everything is working as expected. :)
> 
> -Kees
> 

-- 
Univ.-Prof. Dr. rer. nat. Martin Uecker
Graz University of Technology
Institute of Biomedical Imaging