Re: [PATCH] Fortran: Avoid accessing gfc_charlen when not looking at BT_CHARACTER (PR 110677)

[Harald Anlauf <anlauf@gmx.de>]

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):

diff --git a/gcc/fortran/resolve.cc b/gcc/fortran/resolve.cc
index e7c8d919bef..43095406c16 100644
--- 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 == 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)
  	{

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  <mjambor@suse.cz>
>
> 	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 == 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