From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.gmx.net (mout.gmx.net [212.227.15.15]) by sourceware.org (Postfix) with ESMTPS id 7F5723858417; Mon, 14 Aug 2023 19:29:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 7F5723858417 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmx.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.de; s=s31663417; t=1692041340; x=1692646140; i=anlauf@gmx.de; bh=iqS3QmWxKM1sFZ4DlN4flwHtdgdS5TBWYWPjROmolc4=; h=X-UI-Sender-Class:Date:Subject:To:References:From:In-Reply-To; b=sm0CeUblMlc4UhG4GqVQCd1ojje986F22cwi2uEchBI10HStDJipJihjaUH+co8G169iiVY XYLxF1mgLGunrcO9VxEublrUxfYz1ZHQGpiJNJtuFp/AJ6zP9EQ/4t/Akm2ak4xawnmRANfwB 0oDI7+SYklTUemCMfpgkeICrMXB7l5W8iFkJUXnKSC0s/NLtTVEgSmwxOi6orYHKAux5Q+4Da ilsogZ8wuWPAE7x51FUWZDgA4Y+gXR2Cmb4y88jA9nlVKPVnasWRfSqb2364Utl1u/Hl7K2j+ GYY5/N5JlbYgD0AI2gAC+YvZyTBrCPcObPrGuiHYjxzR8d7aKDGQ== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from [192.168.178.29] ([93.207.81.208]) by mail.gmx.net (mrgmx004 [212.227.17.190]) with ESMTPSA (Nemesis) id 1MIwzA-1qBMXO2Yqw-00KRIy; Mon, 14 Aug 2023 21:29:00 +0200 Message-ID: Date: Mon, 14 Aug 2023 21:28:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Fortran: Avoid accessing gfc_charlen when not looking at BT_CHARACTER (PR 110677) Content-Language: en-US To: Martin Jambor , GCC Patches , fortran@gcc.gnu.org Newsgroups: gmane.comp.gcc.patches,gmane.comp.gcc.fortran References: From: Harald Anlauf In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:3DYaiJnZumMInMNMNqs7BskIJ8SHIZoktOCkvyOZkrvmymVr0bA 623LOT3uyEen9ZO6YqDyf/QA8bpV5nN0LElNNqZLoZOc8xt2+Zw1y7dyBH0W54qJGtLpKNQ jToSC1MubV8tHehCJN0dj/ajNqM8iviT4NtXrkP+q15Zv1pPX8rmdlQ6nHWvJ/uFVhsHuuE G3/FkKkgd45QFjaR3W+zA== UI-OutboundReport: notjunk:1;M01:P0:rq1OV3/UGQQ=;m9X2HWDHWp1qcsCpe1oAlQ5PYGB lNXCM7uwzRzwOvs8MCGno2+Ya5V39IgfjmbNmAAVa60evC7r460l1P/xXk3+9vbXVM/7qt0K6 j/WJKvfTZsAtl5FSwMPpueULzKrD/64cVuazld0vcIiswJmksemw1+3tHRVbs22jyM8gRONrY ppyT1oapILUIY8b3kWheycLaUPWuavb/uDEIejmmM4gad8o4MjOXvvpYDqX95P3VuZkyjiVAr TpRtuUz442MYlbGI7GwcszztoiSX97PTyq9M5Zq5C7jVyF/yD8IFyxULnm5OAWtWgdB5BX1Jr L4AZDWIMfz9H81ZmCHfVWewV8T8LqJ1vdCo2tMTPp8Y0QoNN0oTrnFeyREbquTwHFNbPkt1W3 /qGCJeUOm45iX/NkVOjpBY+mE+o+EgsUKPUr0DgvLL5hjrqeRmnARvc5CaXhkLpM00sdtJofD 8Ml7u3GO9vzZkg65muJaREOShk7NiE8Jv4Nu733iyLjqlJXMnmEzECFH2bUhaQ0YjCUfJPi0r OcCMuyWCmTBEGhX+PPJNc5altD1iJ0ANg/mflyWADJ9nkkC1JLnrqskYw8wiT8w3Zr6UB668t p0gX36U4EmpBK5pABQuTRnP8V/QOiFTgyzWWVuKafhH6OhIdLlHHqCiSKNzDQFzkdorwsQaDM No8nc5PcrRgkqZG00me+bzTv8GlGY1v+zYlOUJOVDlUvgz0dTkNdpF3sfyA88weO6F0d1lLQY zkw9Y3W3csNDHOj1iOcd1daOMSmMjPF+oEEGVIV0dlV+2fIupqga4XX5wLLS7NUBfRSRfO2Y5 creDKghymqTmI9XVMkCUtbxbNjThGgmF79GCU8ojVoww5xeBpbe9AMt6Ny6FNdjSZ6aIr542Z gAxcflhEZnOqnt0inLwdtYe4OCmgQBqeDDZhQI0NQ0Iz7lYubYIRBRZUcsdEljF/dt2AzXgGy x6ShgqCaMjCRKSW4C5l1lNoQJVE= X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,RCVD_IN_MSPIKE_H4,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: Hi Martin, Am 14.08.23 um 19:39 schrieb Martin Jambor: > Hello, > > this patch addresses an issue uncovered by the undefined behavior > sanitizer. In function resolve_structure_cons in resolve.cc there is > a test starting with: > > if (cons->expr->ts.type =3D=3D BT_CHARACTER && comp->ts.u.cl > && comp->ts.u.cl->length > && comp->ts.u.cl->length->expr_type =3D=3D EXPR_CONSTANT > > and UBSAN complained of loads from comp->ts.u.cl->length->expr_type of > integer value 1818451807 which is outside of the value range expr_t > enum. If I understand the code correctly it the entire load was > unwanted because comp->ts.type in those cases is BT_CLASS and not > BT_CHARACTER. This patch simply adds a check to make sure it is only > accessed in those cases. > > I have verified that the UPBSAN failure goes away with this patch, it > also passes bootstrap and testing on x86_64-linux. OK for master? this looks good to me. Looking at that code block, there is a potential other UB a few lines below, where (hopefully integer) string lengths are to be passed to mpz_cmp. If the string length is ill-defined (e.g. non-integer), value.integer is undefined. We've seen this elsewhere, where on BE platforms that undefined value was interpreted as some large integer and giving failures on those platforms. One could similarly add the following checks here (on top of your patch): diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc index e7c8d919bef..43095406c16 100644 =2D-- a/gcc/fortran/resolve.cc +++ b/gcc/fortran/resolve.cc @@ -1401,6 +1401,8 @@ resolve_structure_cons (gfc_expr *expr, int init) && comp->ts.u.cl->length->expr_type =3D=3D EXPR_CONSTANT && cons->expr->ts.u.cl && cons->expr->ts.u.cl->length && cons->expr->ts.u.cl->length->expr_type =3D=3D EXPR_CONSTANT + && cons->expr->ts.u.cl->length->ts.type =3D=3D BT_INTEGER + && comp->ts.u.cl->length->ts.type =3D=3D BT_INTEGER && mpz_cmp (cons->expr->ts.u.cl->length->value.integer, comp->ts.u.cl->length->value.integer) !=3D 0) { It is up to you whether you want to add this. Thanks for the patch! Harald > Thanks, > > Martin > > > > gcc/fortran/ChangeLog: > > 2023-08-14 Martin Jambor > > PR fortran/110677 > * resolve.cc (resolve_structure_cons): Check comp->ts is character > type before accessing stuff through comp->ts.u.cl. > --- > gcc/fortran/resolve.cc | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc > index e7c8d919bef..5b4dfc5fcd2 100644 > --- a/gcc/fortran/resolve.cc > +++ b/gcc/fortran/resolve.cc > @@ -1396,8 +1396,9 @@ resolve_structure_cons (gfc_expr *expr, int init) > the one of the structure, ensure this if the lengths are known at > compile time and when we are dealing with PARAMETER or structure > constructors. */ > - if (cons->expr->ts.type =3D=3D BT_CHARACTER && comp->ts.u.cl > - && comp->ts.u.cl->length > + if (cons->expr->ts.type =3D=3D BT_CHARACTER > + && comp->ts.type =3D=3D BT_CHARACTER > + && comp->ts.u.cl && comp->ts.u.cl->length > && comp->ts.u.cl->length->expr_type =3D=3D EXPR_CONSTANT > && cons->expr->ts.u.cl && cons->expr->ts.u.cl->length > && cons->expr->ts.u.cl->length->expr_type =3D=3D EXPR_CONSTANT