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 309D03858D32 for ; Sun, 28 Jan 2024 10:09:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 309D03858D32 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=tugraz.at Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tugraz.at ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 309D03858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=129.27.2.202 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706436567; cv=none; b=ZSQPvH0b4A9M/oY0rhJYPqCJI78ztOd3tzx8Z7ypeqbAvs5P8NIL5OutqXS/Ep+6HmUeTOUPNJkWDTr01vwzlukgbxEn5hP5wWhG5qhQOulJm2DmX8oiod4qQazQrdVY/Wbz/Ngk3dv0MSJ1IdIZMqNzrfNtrs/lDQGq92jxC4U= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706436567; c=relaxed/simple; bh=z3g/gv5IvgkDKDnKix5EYvXznO1I/oPbBMYNV1qSceQ=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=I1mLHfb2Nn+I1RtK7bBBMqK/7Lg1oEICJhOJww3UsaSnboyn/98PHEWTFPWdYqrR51XFrbsuWvd2l6ekIBdLihJX0/nt/vht27e8dn8Sif56uRrLssD5f1BREL+OyNAB7ENGcM1Juoa5vJaqXKJOR/TUjdN29eG5g2LqAOvPGxM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [192.168.0.221] (84-115-220-186.cable.dynamic.surfer.at [84.115.220.186]) by mailrelay.tugraz.at (Postfix) with ESMTPSA id 4TN6bf3Wf7z3wkf; Sun, 28 Jan 2024 11:09:18 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tugraz.at; s=mailrelay; t=1706436560; bh=DQ10x+VC/gXCpmzIXgkTxBywU1fCCNbu55iRQDRPEUg=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=QEkLkIE7c56VaRoW5rAIx5yMiwB7Ylf4AXl9GNBrHydZq9erA8vBSfII9lgcj6Ley UZHzAKnfXx/BlG3wmB+t4QOTTccAFoAUEuLQOy85lWGlQqegZcnyPb74Z1LoALqaTi ZQ3mTH9es4gM8O4bXH0l//GzfaaEVpOPitfscxSk= Message-ID: <98f8a862e65dfc1ddd8227266ed41724f9422adb.camel@tugraz.at> Subject: Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896) From: Martin Uecker To: Qing Zhao Cc: Kees Cook , "joseph@codesourcery.com" , "richard.guenther@gmail.com" , "jakub@redhat.com" , "siddhesh@gotplt.org" , "isanbard@gmail.com" , "gcc-patches@gcc.gnu.org" Date: Sun, 28 Jan 2024 11:09:18 +0100 In-Reply-To: <848FCA7D-1CBC-435E-8B4A-8136B13CF318@oracle.com> References: <20240124002955.3387096-1-qing.zhao@oracle.com> <202401241624.4DC3D829@keescook> <08c3913ee0e75beb1fdfbce5487418fbd50de9e3.camel@tugraz.at> <848FCA7D-1CBC-435E-8B4A-8136B13CF318@oracle.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.46.4-2 MIME-Version: 1.0 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.117 X-Spam-Status: No, score=-0.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_ABUSEAT,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Am Freitag, dem 26.01.2024 um 14:33 +0000 schrieb Qing Zhao: >=20 > > On Jan 26, 2024, at 3:04 AM, Martin Uecker wrote: > >=20 > >=20 > > 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. >=20 > Yes, this is for resolving a very early gimplification issue as I reporte= d last Nov: > https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638793.html >=20 > Since no-one responded at that time, I fixed the issue by replacing the A= RRAY_REF > With a pointer indirection: > https://gcc.gnu.org/pipermail/gcc-patches/2023-December/639605.html >=20 > The reason for such change is: return a flexible array member TYPE is no= t allowed > by C language (our gimplification follows this rule), so, we have to retu= rn a pointer TYPE instead.=20 >=20 > ******The new internal function >=20 > .ACCESS_WITH_SIZE (REF_TO_OBJ, REF_TO_SIZE, CLASS_OF_SIZE, SIZE_OF_SIZE,= ACCESS_MODE, INDEX) >=20 > INTERNAL_FN (ACCESS_WITH_SIZE, ECF_LEAF | ECF_NOTHROW, NULL) >=20 > which returns the "REF_TO_OBJ" same as the 1st argument; >=20 > Both the return type and the type of the first argument of this function = have been converted from=20 > the incomplete array type to the corresponding pointer type. >=20 > 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 INDE= X for bound sanitizer instrumentation, I added > The 6th argument =E2=80=9CINDEX=E2=80=9D. >=20 > What=E2=80=99s your comment and suggestion on this solution? I am not entirely sure but changing types in the FE seems problematic because this breaks language semantics. And then adding special code everywhere to treat it specially in the FE does not seem a good way forward. If I understand correctly, returning an incomplete array=C2=A0 type is not allowed and then fails during gimplification. So I would suggest to make it return a pointer to the=C2=A0 incomplete array (and not the element type) but then wrap it with an indirection when inserting this code in the FE so that the full replacement has the correct type again (of the incomplete array). Alternatively, one could allow this during gimplification or add some conversion. Martin >=20 > Thanks. >=20 > Qing >=20 >=20 > >=20 > > Martin > >=20 > >=20 > > Am Donnerstag, dem 25.01.2024 um 20:11 +0000 schrieb Qing Zhao: > > > Thanks a lot for the testing. > > >=20 > > > Yes, I can repeat the issue with the following small example: > > >=20 > > > #include > > > #include > > > #include > > >=20 > > > #define MAX(a, b) ((a) > (b) ? (a) : (b)) > > >=20 > > > struct untracked { > > > int size; > > > int array[] __attribute__((counted_by (size))); > > > } *a; > > > struct untracked * alloc_buf (int index) > > > { > > > struct untracked *p; > > > p =3D (struct untracked *) malloc (MAX (sizeof (struct untracked), > > > (offsetof (struct untracked, a= rray[0]) > > > + (index) * sizeof (int)))); > > > p->size =3D index; > > > return p; > > > } > > >=20 > > > int main() > > > { > > > a =3D alloc_buf(10); > > > printf ("same_type is %d\n", > > > (__builtin_types_compatible_p(typeof (a->array), typeof (&(a->array)= [0])))); > > > return 0; > > > } > > >=20 > > >=20 > > > /home/opc/Install/latest-d/bin/gcc -O2 btcp.c > > > same_type is 1 > > >=20 > > > Looks like that the =E2=80=9Ctypeof=E2=80=9D operator need to be hand= led specially in C FE > > > for the new internal function .ACCESS_WITH_SIZE.=20 > > >=20 > > > (I have specially handle the operator =E2=80=9Coffsetof=E2=80=9D in C= FE already). > > >=20 > > > Will fix this issue. > > >=20 > > > Thanks. > > >=20 > > > Qing > > >=20 > > > > On Jan 24, 2024, at 7:51 PM, Kees Cook wrot= e: > > > >=20 > > > > On Wed, Jan 24, 2024 at 12:29:51AM +0000, Qing Zhao wrote: > > > > > This is the 4th version of the patch. > > > >=20 > > > > Thanks very much for this! > > > >=20 > > > > I tripped over an unexpected behavioral change that the Linux kerne= l > > > > depends on: > > > >=20 > > > > __builtin_types_compatible_p() no longer treats an array marked wit= h > > > > counted_by as different from that array's decayed pointer. Specific= ally, > > > > the kernel uses these macros: > > > >=20 > > > >=20 > > > > /* > > > > * 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 express= ions > > > > * aren't permitted). > > > > */ > > > > #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); }= ))) > > > >=20 > > > > #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), t= ypeof(b)) > > > >=20 > > > > /* &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])) > > > >=20 > > > >=20 > > > > This gets used in various places to make sure we're dealing with an > > > > array for a macro: > > > >=20 > > > > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be= _array(arr)) > > > >=20 > > > >=20 > > > > So this builds: > > > >=20 > > > > struct untracked { > > > > int size; > > > > int array[]; > > > > } *a; > > > >=20 > > > > __must_be_array(a->array) > > > > =3D> 0 (as expected) > > > > __builtin_types_compatible_p(typeof(a->array), typeof(&(a->array)[0= ])) > > > > =3D> 0 (as expected, array vs decayed array pointer) > > > >=20 > > > >=20 > > > > But if counted_by is added, we get a build failure: > > > >=20 > > > > struct tracked { > > > > int size; > > > > int array[] __counted_by(size); > > > > } *b; > > > >=20 > > > > __must_be_array(b->array) > > > > =3D> build failure (not expected) > > > > __builtin_types_compatible_p(typeof(b->array), typeof(&(b->array)[0= ])) > > > > =3D> 1 (not expected, both pointers?) > > > >=20 > > > >=20 > > > >=20 > > > >=20 > > > > --=20 > > > > Kees Cook > > >=20 > >=20 >=20