From: Qing Zhao <qing.zhao@oracle.com>
To: Martin Uecker <uecker@tugraz.at>
Cc: Kees Cook <keescook@chromium.org>,
"joseph@codesourcery.com" <joseph@codesourcery.com>,
"richard.guenther@gmail.com" <richard.guenther@gmail.com>,
"jakub@redhat.com" <jakub@redhat.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
"siddhesh@gotplt.org" <siddhesh@gotplt.org>,
"isanbard@gmail.com" <isanbard@gmail.com>
Subject: Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
Date: Wed, 9 Aug 2023 20:34:41 +0000 [thread overview]
Message-ID: <F0A17376-E152-424F-BE4A-B3F68AF6A306@oracle.com> (raw)
In-Reply-To: <5f76638c8cfca7611e955ef9fadacfd7f8dca0fb.camel@tugraz.at>
> On Aug 8, 2023, at 10:54 AM, Martin Uecker <uecker@tugraz.at> wrote:
>
>
>
> 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.
As I checked the algorithm for bos in GCC, it uses a similar formula as the following to compute the object size:
offset(struct foo, t[0]) + N * sizeof(*foo->t);
Which seems correct to me. (Therefore bos returns 9 for this example).
>
>
> 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.
My understanding is, since a structure with FAM cannot be an element of an array,
the tailing padding might not be necessary?
Qing
>
>
> 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
next prev parent reply other threads:[~2023-08-09 20:34 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-04 19:44 Qing Zhao
2023-08-04 19:44 ` [V2][PATCH 1/3] Provide counted_by attribute to flexible array member field (PR108896) Qing Zhao
2023-08-04 19:44 ` [V2][PATCH 2/3] Use the counted_by atribute info in builtin object size [PR108896] Qing Zhao
2023-08-04 19:44 ` [V2][PATCH 3/3] Use the counted_by attribute information in bound sanitizer[PR108896] Qing Zhao
2023-08-07 16:16 ` [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896) Kees Cook
2023-08-07 16:33 ` Qing Zhao
2023-08-09 19:17 ` Kees Cook
2023-08-08 14:54 ` Martin Uecker
2023-08-08 16:18 ` Michael Matz
2023-08-08 19:58 ` Kees Cook
2023-08-09 16:05 ` Qing Zhao
2023-08-09 16:21 ` Michael Matz
2023-08-09 20:10 ` Qing Zhao
2023-08-10 6:58 ` Martin Uecker
2023-08-10 13:59 ` Qing Zhao
2023-08-10 14:38 ` Martin Uecker
2023-08-10 14:42 ` Jakub Jelinek
2023-08-10 14:47 ` Martin Uecker
2023-08-10 14:58 ` Siddhesh Poyarekar
2023-08-10 15:18 ` Martin Uecker
2023-08-10 16:28 ` Qing Zhao
2023-08-10 16:30 ` Siddhesh Poyarekar
2023-08-10 16:39 ` Jakub Jelinek
2023-08-10 17:06 ` Siddhesh Poyarekar
2023-08-16 21:42 ` Qing Zhao
2023-08-10 18:18 ` Qing Zhao
2023-08-10 14:02 ` Michael Matz
2023-08-10 13:54 ` Michael Matz
2023-08-09 20:34 ` Qing Zhao [this message]
2023-08-17 5:31 ` Kees Cook
2023-08-17 6:38 ` Kees Cook
2023-08-17 13:44 ` Qing Zhao
2023-08-17 16:54 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=F0A17376-E152-424F-BE4A-B3F68AF6A306@oracle.com \
--to=qing.zhao@oracle.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=isanbard@gmail.com \
--cc=jakub@redhat.com \
--cc=joseph@codesourcery.com \
--cc=keescook@chromium.org \
--cc=richard.guenther@gmail.com \
--cc=siddhesh@gotplt.org \
--cc=uecker@tugraz.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).