From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id D5034385694E; Tue, 15 Aug 2023 15:18:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D5034385694E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=suse.cz Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id D281421892; Tue, 15 Aug 2023 15:18:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1692112714; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=aOM0FJBKmZftTslJtrwfQ88Wd89WmhLE5cLej6KgqbY=; b=f6yTUUL5BP26eFFyVcZMcemiy/qzyYjVCwU3LkZdMnb+wCAoTxFNIuah20K+Uh+EWqIcKz 0In7wkP/UR09MefECnoyq/QVcBjvkVrpK3oouqO5NWZo7dokYGAOnCZHbrgW1r/2QyUQv0 JPD/O2mWOF9P8zHfvU/qtErMw4EtfSQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1692112714; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=aOM0FJBKmZftTslJtrwfQ88Wd89WmhLE5cLej6KgqbY=; b=PEZX0hSQY8TloZCxzAQndq5JyoFbn+B4nWUyxElZDFw8ICmtRQuWBnTzTvIcUvcXKNkThp 7lsGDpzGrVA30LCg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id C6F8D13909; Tue, 15 Aug 2023 15:18:34 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id gyWAMEqX22S7bQAAMHmgww (envelope-from ); Tue, 15 Aug 2023 15:18:34 +0000 From: Martin Jambor To: Harald Anlauf Cc: fortran@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fortran: Avoid accessing gfc_charlen when not looking at BT_CHARACTER (PR 110677) In-Reply-To: References: User-Agent: Notmuch/0.37 (https://notmuchmail.org) Emacs/28.2 (x86_64-suse-linux-gnu) Date: Tue, 15 Aug 2023 17:18:34 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_SOFTFAIL,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: Hello, On Mon, Aug 14 2023, Harald Anlauf via Gcc-patches wrote: > 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 == BT_CHARACTER && comp->ts.u.cl >> && comp->ts.u.cl->length >> && comp->ts.u.cl->length->expr_type == 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): Thank you very much for the approval and the improvement. I have committed the following (after another round of testing). Martin Fortran: Avoid accessing gfc_charlen when not looking at BT_CHARACTER (PR 110677) 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 == BT_CHARACTER && comp->ts.u.cl && comp->ts.u.cl->length && comp->ts.u.cl->length->expr_type == 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. During review, Harald Anlauf noticed that length types also need to be checked and so I added also checks that he suggested to the condition. Co-authored-by: Harald Anlauf 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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc index e7c8d919bef..f51674f7faa 100644 --- a/gcc/fortran/resolve.cc +++ b/gcc/fortran/resolve.cc @@ -1396,11 +1396,14 @@ 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 == BT_CHARACTER && comp->ts.u.cl - && comp->ts.u.cl->length + if (cons->expr->ts.type == BT_CHARACTER + && comp->ts.type == BT_CHARACTER + && comp->ts.u.cl && comp->ts.u.cl->length && comp->ts.u.cl->length->expr_type == EXPR_CONSTANT && cons->expr->ts.u.cl && cons->expr->ts.u.cl->length && cons->expr->ts.u.cl->length->expr_type == EXPR_CONSTANT + && cons->expr->ts.u.cl->length->ts.type == BT_INTEGER + && comp->ts.u.cl->length->ts.type == BT_INTEGER && mpz_cmp (cons->expr->ts.u.cl->length->value.integer, comp->ts.u.cl->length->value.integer) != 0) { -- 2.41.0