Re: [PATCH] PR fortran/78240 -- kludge of the day

[Fritz Reese <fritzoreese at gmail dot com>]

On Thu, Nov 9, 2017 at 5:02 PM, Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
> The following patch fixes PR fortran/78240.  It seems
> to me to be inelegant, but it does pass regression
> testing. [...] OK to commit?

Upon closer analysis, the patch is insufficient to fix the PR. I will
explain below. At the bottom of this letter I explain and have
attached a new patch which fixes some more subtle issues in the code
which causes the PR.

> [...] The second part in resolve.c is needed
> to avoid a NULL pointer dereference when one forgets
> to use -fdec with Gerhard's code.
[...]
> --- gcc/fortran/resolve.c       (revision 254603)
> +++ gcc/fortran/resolve.c       (working copy)
> @@ -15284,7 +15290,7 @@ check_data_variable (gfc_data_variable *var, locus *wh
>        if (!gfc_array_size (e, &size))
>         {
>           gfc_error ("Nonconstant array section at %L in DATA statement",
> -                    &e->where);
> +                    where);
>           mpz_clear (offset);
>           return false;
>         }

This portion of the patch is correct and fixes one of the issues.

> [...] The kludgy portion occurs in decl.c.
> march_clist_expr is clearly expecting an array with
> constant dimension due to gcc_assert.  When -fdec
> is used and one takes Gerhard code (see testcase),
> the assertion is raised.  The patch checks for
> -fdec and failure of spec_size(), and then goes
> to cleanup.
[...]
> --- gcc/fortran/decl.c  (revision 254603)
> +++ gcc/fortran/decl.c  (working copy)
> @@ -735,6 +735,8 @@ match_clist_expr (gfc_expr **result, gfc_typespec *ts,
>
>        /* Validate sizes. */
>        gcc_assert (gfc_array_size (expr, &size));
> +      if (flag_dec && !spec_size (as, &repeat))
> +       goto cleanup;
>        gcc_assert (spec_size (as, &repeat));
>        cmp = mpz_cmp (size, repeat);
>        if (cmp < 0)

This portion of the patch is insufficient. By removing the assert on
spec_size, the remaining code improperly handles the memory of the two
mpz_integer variables 'size' and 'repeat'. Since gfc_array_size() and
spec_size() both allocate their mpz_integer* arguments,
match_clist_expr() should not allocate them (or clear them on error).
With the original patch, memory corruption occurs in several ways
(double-alloc of repeat in gfc_array_size, double-free of size on
failure of spec_size). Strange that regression testing did not reveal
the memory corruption (I see the corruptions in my own gcc after
manually applying the patch, at least).

I have attached a patch which better handles false return from
spec_size, which occurs when the array-spec for the variable being
initialized is not constant. Proper handling requires careful
management of the two mpz_integer size variables used in
gfc_array_size and spec_size. The patch also includes the fix in
resolve.c.

Thanks for pointing me to this issue and allowing me time to review
it. The new patch passes all regression tests including its two tests
(one for each issue above). OK to commit this one?

---
Fritz Reese



>From f2b8262a1a366b072493b129d38465ffa3d265bc Mon Sep 17 00:00:00 2001
From: Fritz Reese <fritzoreese@gmail.com>
Date: Mon, 13 Nov 2017 15:58:26 -0500
Subject: [PATCH] Fix ICE on non-constant array-specs in clist initializers.

        PR fortran/78240
        gcc/fortran/
        * decl.c (match_clist_expr): Replace gcc_assert with proper
        handling of bad result from spec_size().
        * resolve.c (check_data_variable): Avoid NULL dereference when passing
        locus to gfc_error.

        PR fortran/78240
        * gcc/testsuite/gfortran.dg/dec_structure_23.f90: New.
        * gcc/testsuite/gfortran.dg/pr78240.f90: New.
---
 gcc/fortran/decl.c                             | 37 +++++++++++++++++---------
 gcc/fortran/resolve.c                          |  2 +-
 gcc/testsuite/gfortran.dg/dec_structure_23.f90 | 19 +++++++++++++
 gcc/testsuite/gfortran.dg/pr78240.f90          | 12 +++++++++
 4 files changed, 56 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/dec_structure_23.f90
 create mode 100644 gcc/testsuite/gfortran.dg/pr78240.f90

Attachment: pr78240.patch
Description: Binary data