From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailrelay.tugraz.at (mailrelay.tugraz.at [129.27.2.202]) by sourceware.org (Postfix) with ESMTPS id 926253857BA4 for ; Tue, 8 Aug 2023 14:54:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 926253857BA4 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=tugraz.at Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tugraz.at Received: from fbmtpc21.tugraz.at (fbmtpc21.tugraz.at [129.27.144.40]) by mailrelay.tugraz.at (Postfix) with ESMTPSA id 4RKx6v1GGbz3wBt; Tue, 8 Aug 2023 16:54:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tugraz.at; s=mailrelay; t=1691506487; bh=1tUaWaVfM0Kmbqrg8JsLI0mjnAFGJHPLkCMNY0lKYI0=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=kKH2C2PBa4ZGZAxMMZIabm2VRtErK4jNLBPF8zIk6Jhy6JnJQZ/02lNdTEFJxWxN/ lVKPc2oPnHocbXyGw3inwo1/iMcwHWMZ8hWEAJzlQkR75aIr+KPzDih+e/B9pdBcOh gvxTT7pmH4Nl8l1U/jpbNkmFmW+kCThDTgVAN6Dc= Message-ID: <5f76638c8cfca7611e955ef9fadacfd7f8dca0fb.camel@tugraz.at> Subject: Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896) From: Martin Uecker To: Kees Cook , Qing Zhao Cc: joseph@codesourcery.com, richard.guenther@gmail.com, jakub@redhat.com, gcc-patches@gcc.gnu.org, siddhesh@gotplt.org, isanbard@gmail.com Date: Tue, 08 Aug 2023 16:54:46 +0200 In-Reply-To: <202308070858.D2FB43E@keescook> References: <20230804194431.993958-1-qing.zhao@oracle.com> <202308070858.D2FB43E@keescook> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.38.3-1+deb11u2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-TUG-Backscatter-control: G/VXY7/6zeyuAY/PU2/0qw X-Spam-Scanner: SpamAssassin 3.003001 X-Spam-Score-relay: -0.4 X-Scanned-By: MIMEDefang 2.74 on 129.27.10.116 X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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