PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169

I have the impression that Gerhard is a hydra: one PR down, he submits
two new ones... :-(
Anyway, here's a straightforward fix for a NULL pointer dereference for
an invalid argument to STAT.  For an alternative patch by Steve see PR.

Regtested on x86_64-pc-linux-gnu.  OK for mainline / 11-branch when it
reopens?

Thanks,
Harald


Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument

gcc/fortran/ChangeLog:

	PR fortran/101564
	* resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
	dereference and shortcut for bad STAT argument to (DE)ALLOCATE.

gcc/testsuite/ChangeLog:

	PR fortran/101564
	* gfortran.dg/pr101564.f90: New test.

On 21.07.21 22:36, Harald Anlauf via Gcc-patches wrote:

> Anyway, here's a straightforward fix for a NULL pointer dereference for
> an invalid argument to STAT.  For an alternative patch by Steve see PR.
>
> Regtested on x86_64-pc-linux-gnu.  OK for mainline / 11-branch when it
> reopens?
..
> Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument
>
> gcc/fortran/ChangeLog:
>
>       PR fortran/101564
>       * resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
>       dereference and shortcut for bad STAT argument to (DE)ALLOCATE.
>
> gcc/testsuite/ChangeLog:
>
>       PR fortran/101564
>       * gfortran.dg/pr101564.f90: New test.
> diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
> index 45c3ad387ac..51d312116eb 100644
> --- a/gcc/fortran/resolve.c
> +++ b/gcc/fortran/resolve.c
> @@ -8165,6 +8165,9 @@ resolve_allocate_deallocate (gfc_code *code, const char *fcn)
>       gfc_error ("Stat-variable at %L must be a scalar INTEGER "
>                  "variable", &stat->where);
>
> +      if (stat->expr_type == EXPR_CONSTANT || stat->symtree == NULL)
> +     goto done_stat;
> +

I wonder whether this will catch all cases, e.g. stat->symtree != NULL
but using something else than '->n.sym'. I currently cannot spot
whether a user operator or a type-bound procedure is possible
in this case, but if so, n.sym->something is not well defined.

Additionally, I wonder whether that will work with:

integer, pointer :: ptr
integer function f()
   pointer :: f
   f = ptr
end
allocate(A, stat=f())

The f() is a variable and definable – but I am currently not sure it sets stat->symtree
and not only stat->value.function.esym, but I have not tested it.
(Answer: it does set it - at least there is an assert in gfc_check_vardef_context
that symtree != NULL for EXPR_FUNCTION.)


Can't we just as a 'if (!' + ') goto done_stat;' around:

       gfc_check_vardef_context (stat, false, false, false,
                                 _("STAT variable"));


Additionally, I have to admit that I do not understand the
following existing condition, which you did not touch:

       if ((stat->ts.type != BT_INTEGER
            && !(stat->ref && (stat->ref->type == REF_ARRAY
                               || stat->ref->type == REF_COMPONENT)))
           || stat->rank > 0)
         gfc_error ("Stat-variable at %L must be a scalar INTEGER "
                    "variable", &stat->where);

I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
but what's the reason for the refs?

My impression is that it is supposed to handle REF_INQUIRY
such as  x%kind – but that does not seem to handle x%y%kind.

It looks as if gfc_check_vardef_context needs an additional
check for REF_INQUIRY – and then the check above can be
simplified to the obvious version.


Can you check? That's

* use if (!gfc_check_vardef_context ()) goto done_stat;
* Add REF_INQUIRY check to gfc_check_vardef_context
* Simplify the check to !BT_INTEGER || rank != 0

And possibly add a testcase for stat=f() [valid]
and stat=x%y%kind [invalid] as well?

Thanks,

Tobias

>         for (p = code->ext.alloc.list; p; p = p->next)
>       if (p->expr->symtree->n.sym->name == stat->symtree->n.sym->name)
>         {
> @@ -8192,6 +8195,8 @@ resolve_allocate_deallocate (gfc_code *code, const char *fcn)
>         }
>       }
>
> +done_stat:
> +
>     /* Check the errmsg variable.  */
>     if (errmsg)
>       {
> diff --git a/gcc/testsuite/gfortran.dg/pr101564.f90 b/gcc/testsuite/gfortran.dg/pr101564.f90
> new file mode 100644
> index 00000000000..1e7c9911ce6
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/pr101564.f90
> @@ -0,0 +1,9 @@
> +! { dg-do compile }
> +! PR fortran/101564 - ICE in resolve_allocate_deallocate
> +
> +program p
> +  integer, allocatable :: x(:)
> +  integer              :: stat
> +  allocate (x(2), stat=stat)
> +  deallocate (x,  stat=stat%kind) ! { dg-error "(STAT variable)" }
> +end
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Hi Tobias,

I am afraid we're really opening a can of worms here....

> Additionally, I wonder whether that will work with:
> 
> integer, pointer :: ptr
> integer function f()
>    pointer :: f
>    f = ptr
> end
> allocate(A, stat=f())
> 
> The f() is a variable and definable – but I am currently not sure it sets stat->symtree
> and not only stat->value.function.esym, but I have not tested it.

I think a "working" testcase for this could be:

program p
  implicit none
  integer, target  :: ptr
  integer, pointer :: A
  allocate (A, stat=f())
  print *, ptr
contains
  function f()
    integer, pointer :: f
    f => ptr
  end function f
end

This works as expected with Intel and AOCC, but gives a
syntax error with every gfortran tested because of match.c:

alloc_opt_list:

      m = gfc_match (" stat = %v", &tmp);

where the comment before gfc_match states:

   %v  Matches a variable expression (an lvalue)

although it matches only variables and not every type of lvalue.
We therefore never get to the interesting checks elsewhere...

> Additionally, I have to admit that I do not understand the
> following existing condition, which you did not touch:
> 
>        if ((stat->ts.type != BT_INTEGER
>             && !(stat->ref && (stat->ref->type == REF_ARRAY
>                                || stat->ref->type == REF_COMPONENT)))
>            || stat->rank > 0)
>          gfc_error ("Stat-variable at %L must be a scalar INTEGER "
>                     "variable", &stat->where);
> 
> I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
> but what's the reason for the refs?

Well, that needs to be answered by Steve (see commit 3759634).

[...]

> And possibly add a testcase for stat=f() [valid]
> and stat=x%y%kind [invalid] as well?

Well, I need to go back to the drawing board then...

Thanks,
Harald

Hi Harald,

On 22.07.21 21:50, Harald Anlauf wrote:
> I am afraid we're really opening a can of worms here....
which is not too bad if there are only two earthworms in there ;-)
>> Additionally, I wonder whether that will work with:
> I think a "working" testcase for this could be:
>
> program p
>    implicit none
>    integer, target  :: ptr
>    integer, pointer :: A
>    allocate (A, stat=f())
>    print *, ptr
> contains
>    function f()
>      integer, pointer :: f
>      f => ptr
>    end function f
> end
Indeed that I meant.
> This works as expected with Intel and AOCC, but gives a
> syntax error with every gfortran tested because of match.c:
>
> alloc_opt_list:
>        m = gfc_match (" stat = %v", &tmp);

I think we can simply change that one to %e; the definable
check should ensure that any non variable (in the Fortran sense)
is rejected.

And we should update the comment for %v / match_variable to state
that it does not include function references.

In some cases, like with OpenMP, we still do not want to match
functions, hence, changing match_variable is probably not what we
want to do. Additionally, for all %v replaced by %e we need to
ensure that there is a definable check. (Which should be there
already as INTENT(IN) or named constants or ... are also invalid.)

Also affected: Some I/O items, a bunch of other stat=%v and
errmsg=%v.

Talking about errmsg: In the same function, the same check is
done for errmsg as for stat – hence, the patch should update
also errmsg.

>> Additionally, I have to admit that I do not understand the
>> following existing condition, which you did not touch:
>>
>>         if ((stat->ts.type != BT_INTEGER
>>              && !(stat->ref && (stat->ref->type == REF_ARRAY
>>                                 || stat->ref->type == REF_COMPONENT)))
>>             || stat->rank > 0)
>>           gfc_error ("Stat-variable at %L must be a scalar INTEGER "
>>                      "variable", &stat->where);
>>
>> I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
>> but what's the reason for the refs?
> Well, that needs to be answered by Steve (see commit 3759634).

(https://gcc.gnu.org/g:3759634f3208cbc1226bec19d22cbff989a287c3 (svn
r145331))

The reason for the ref checks is unclear and seem to be wrong. The added
testcases also only use 'x' (real) and n or i (integer) as input, i.e.
they do not exercise this. I did not look for the patch email for reasoning.

Thanks,

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Hi Tobias,

> > This works as expected with Intel and AOCC, but gives a
> > syntax error with every gfortran tested because of match.c:
> >
> > alloc_opt_list:
> >        m = gfc_match (" stat = %v", &tmp);
> 
> I think we can simply change that one to %e; the definable
> check should ensure that any non variable (in the Fortran sense)
> is rejected.
> 
> And we should update the comment for %v / match_variable to state
> that it does not include function references.

I've updated this for ALLOCATE/DEALLOCATE and STAT/ERRMSG, see
attached patch.  This required updating the error messages of
two existing files in the testsuite.

> Also affected: Some I/O items, a bunch of other stat=%v and
> errmsg=%v.

We should rather open a separate PR on auditing the related uses
of gfc_match.

> Talking about errmsg: In the same function, the same check is
> done for errmsg as for stat – hence, the patch should update
> also errmsg.

Done.

> >> Additionally, I have to admit that I do not understand the
> >> following existing condition, which you did not touch:
> >>
> >>         if ((stat->ts.type != BT_INTEGER
> >>              && !(stat->ref && (stat->ref->type == REF_ARRAY
> >>                                 || stat->ref->type == REF_COMPONENT)))
> >>             || stat->rank > 0)
> >>           gfc_error ("Stat-variable at %L must be a scalar INTEGER "
> >>                      "variable", &stat->where);
> >>
> >> I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
> >> but what's the reason for the refs?
> > Well, that needs to be answered by Steve (see commit 3759634).
> 
> (https://gcc.gnu.org/g:3759634f3208cbc1226bec19d22cbff989a287c3 (svn
> r145331))
> 
> The reason for the ref checks is unclear and seem to be wrong. The added
> testcases also only use 'x' (real) and n or i (integer) as input, i.e.
> they do not exercise this. I did not look for the patch email for reasoning.

Well, there is some text in the standard that I added in front of
the for loops, and this code is now exercised in the new testcase.

Regtested on x86_64-pc-linux-gnu.  OK?

Thanks,
Harald

Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument

gcc/fortran/ChangeLog:

	PR fortran/101564
	* match.c (gfc_match): Fix comment for %v code.
	(gfc_match_allocate, gfc_match_deallocate): Replace use of %v code
	by %e in gfc_match to allow for function references as STAT and
	ERRMSG arguments.
	* resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
	dereferences and shortcut for bad STAT and ERRMSG argument to
	(DE)ALLOCATE.

gcc/testsuite/ChangeLog:

	PR fortran/101564
	* gfortran.dg/allocate_stat_3.f90: New test.
	* gfortran.dg/allocate_stat.f90: Adjust error messages.
	* gfortran.dg/implicit_11.f90: Adjust error messages.

Hi Harald,

On 26.07.21 23:55, Harald Anlauf wrote:
> I've updated this for ALLOCATE/DEALLOCATE and STAT/ERRMSG, see
> attached patch.  This required updating the error messages of
> two existing files in the testsuite.
Thanks.
>> Also affected: Some I/O items, a bunch of other stat=%v and
>> errmsg=%v.
> We should rather open a separate PR on auditing the related uses
> of gfc_match.

I concur – I just wanted to quickly check how many %v are there –
besides %v, there are also direct calls to gfc_match_variable.

Can you open a PR?

>>>>          if ((stat->ts.type != BT_INTEGER
>>>>               && !(stat->ref && (stat->ref->type == REF_ARRAY
>>>>                                  || stat->ref->type == REF_COMPONENT)))
>>>>              || stat->rank > 0)
>>>>            gfc_error ("Stat-variable at %L must be a scalar INTEGER "
>>>>                       "variable", &stat->where);
>>>> I mean the ts.type != BT_INTEGER and stat->rank != 0 is clear,
>>>> but what's the reason for the refs?
>>> Well, that needs to be answered by Steve (see commit 3759634).
>>>
>>> (https://gcc.gnu.org/g:3759634f3208cbc1226bec19d22cbff989a287c3 (svn
>>> r145331))
>>>
>>> The reason for the ref checks is unclear and seem to be wrong. The added
>>> testcases also only use 'x' (real) and n or i (integer) as input, i.e.
>>> they do not exercise this. I did not look for the patch email for reasoning.
> Well, there is some text in the standard that I added in front of
> the for loops, and this code is now exercised in the new testcase.

The loops are clear – but the
  '!stat->ref || (...ref->type != ARRAY || ref->type != COMPONENT))'
is still not clear to me.

   * * *

Can you add the (working) test:
   allocate (A, stat=y%stat%kind)  ! { dg-error "cannot be a constant" }
   deallocate (A, stat=y%stat%kind)  ! { dg-error "cannot be a constant" }
to your testcase gcc/testsuite/gfortran.dg/allocate_stat_3.f90 ?


And also the following one, which does not get diagnosed and, hence,
later gives an ICE during gimplification.

   type tc
     character (len=:), allocatable :: str
   end type tc
...
   type(tc) :: z
...
   allocate(character(len=13) :: z%str)
   allocate (A, stat=z%str%len)
   deallocate (A, stat=z%str%len)

To fix it, I think the solution is to do the following:
* In gfc_check_vardef_context, handle also REF_INQUIRY; in the
     for (ref = e->ref; ref && check_intentin; ref = ref->next)
   loop, I think there should be a
     if (ref->type == REF_INQUIRY)
       {
         if (context)
          gfc_error ("Type parameter inquiry for %qs in "
                     "variable definition context (%s) at %L",
                     name, context, &e->where);
         return false;
       }
(untested)

I assume (but have not tested it) that will give
two error messages for:
   allocate (A, errmsg=z%str%len)
   deallocate (A, errmsg=z%str%len)
one for the new type-param-inquiry check and one for
   != BT_CHARACTER
if you want to prevent the double error, consider to
replace
    gfc_check_vardef_context (...);
by
    if (!gfc_check_vardef_context (...))
      goto done_errmsg;

> Regtested on x86_64-pc-linux-gnu.  OK?
LGTM - except for the two testcase additions proposed above
and fixing the ICE. If you are happy with my changes and they
work, feel free add them and commit without further review.
In either case, I have the feeling we are nearly there. :-)

Thanks for the patch and the review-modification-review-... patience!

Tobias

> Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument
>
> gcc/fortran/ChangeLog:
>
>       PR fortran/101564
>       * match.c (gfc_match): Fix comment for %v code.
>       (gfc_match_allocate, gfc_match_deallocate): Replace use of %v code
>       by %e in gfc_match to allow for function references as STAT and
>       ERRMSG arguments.
>       * resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
>       dereferences and shortcut for bad STAT and ERRMSG argument to
>       (DE)ALLOCATE.
>
> gcc/testsuite/ChangeLog:
>
>       PR fortran/101564
>       * gfortran.dg/allocate_stat_3.f90: New test.
>       * gfortran.dg/allocate_stat.f90: Adjust error messages.
>       * gfortran.dg/implicit_11.f90: Adjust error messages.
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Hi Tobias,

> > We should rather open a separate PR on auditing the related uses
> > of gfc_match.
> 
> I concur – I just wanted to quickly check how many %v are there –
> besides %v, there are also direct calls to gfc_match_variable.
> 
> Can you open a PR?

this is now https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101652

> The loops are clear – but the
>   '!stat->ref || (...ref->type != ARRAY || ref->type != COMPONENT))'
> is still not clear to me.

Ah, I was really missing the point and looking in the wrong place.
Actually, I also do not understand the reason for this version of
the check, and it also leads to a strange accepts-invalid for
certain non-integer STAT variables.  Removing the stat->ref part
fixes that without introducing any regression in the testsuite.
(There was an analogous part in the check for ERRMSG.)

> Can you add the (working) test:
>    allocate (A, stat=y%stat%kind)  ! { dg-error "cannot be a constant" }
>    deallocate (A, stat=y%stat%kind)  ! { dg-error "cannot be a constant" }
> to your testcase gcc/testsuite/gfortran.dg/allocate_stat_3.f90 ?

Done.

> And also the following one, which does not get diagnosed and, hence,
> later gives an ICE during gimplification.
> 
>    type tc
>      character (len=:), allocatable :: str
>    end type tc
> ...
>    type(tc) :: z
> ...
>    allocate(character(len=13) :: z%str)
>    allocate (A, stat=z%str%len)
>    deallocate (A, stat=z%str%len)
> 
> To fix it, I think the solution is to do the following:
> * In gfc_check_vardef_context, handle also REF_INQUIRY; in the
>      for (ref = e->ref; ref && check_intentin; ref = ref->next)
>    loop, I think there should be a
>      if (ref->type == REF_INQUIRY)
>        {
>          if (context)
>           gfc_error ("Type parameter inquiry for %qs in "
>                      "variable definition context (%s) at %L",
>                      name, context, &e->where);
>          return false;
>        }
> (untested)

This almost worked, needing only a restriction to %KIND and %LEN.
Note that %RE and %IM are usually definable.

> I assume (but have not tested it) that will give
> two error messages for:
>    allocate (A, errmsg=z%str%len)
>    deallocate (A, errmsg=z%str%len)
> one for the new type-param-inquiry check and one for
>    != BT_CHARACTER
> if you want to prevent the double error, consider to
> replace
>     gfc_check_vardef_context (...);
> by
>     if (!gfc_check_vardef_context (...))
>       goto done_errmsg;

Yes, that is reasonable.  Done.

> > Regtested on x86_64-pc-linux-gnu.  OK?
> LGTM - except for the two testcase additions proposed above
> and fixing the ICE. If you are happy with my changes and they
> work, feel free add them and commit without further review.
> In either case, I have the feeling we are nearly there. :-)

I have added the updated "final" version of the patch to give
everybody another 24h to have a look, and will commit if nobody
complains.

> Thanks for the patch and the review-modification-review-... patience!

Well, I believe this was really a worthwile review process,
with fixing a few issues on the way before Gerhard finds them...

Thanks,
Harald


Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument

gcc/fortran/ChangeLog:

	PR fortran/101564
	* expr.c (gfc_check_vardef_context): Add check for KIND and LEN
	parameter inquiries.
	* match.c (gfc_match): Fix comment for %v code.
	(gfc_match_allocate, gfc_match_deallocate): Replace use of %v code
	by %e in gfc_match to allow for function references as STAT and
	ERRMSG arguments.
	* resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
	dereferences and shortcut for bad STAT and ERRMSG argument to
	(DE)ALLOCATE.  Remove bogus parts of checks for STAT and ERRMSG.

gcc/testsuite/ChangeLog:

	PR fortran/101564
	* gfortran.dg/allocate_stat_3.f90: New test.
	* gfortran.dg/allocate_stat.f90: Adjust error messages.
	* gfortran.dg/implicit_11.f90: Likewise.
	* gfortran.dg/inquiry_type_ref_3.f90: Likewise.

Hi Harald,

On 27.07.21 23:42, Harald Anlauf wrote:
> This almost worked, needing only a restriction to %KIND and %LEN.
> Note that %RE and %IM are usually definable.
Well spotted :-)
> Regtested on x86_64-pc-linux-gnu.  OK?
>> LGTM - except [...] feel free add them and commit without further review.
>> [...]
> I have added the updated "final" version of the patch to give
> everybody another 24h to have a look, and will commit if nobody
> complains.
LGTM - thanks again.

> [...] with fixing a few issues on the way before Gerhard finds them...

:-)

Tobias

> Fortran: ICE in resolve_allocate_deallocate for invalid STAT argument
>
> gcc/fortran/ChangeLog:
>
>       PR fortran/101564
>       * expr.c (gfc_check_vardef_context): Add check for KIND and LEN
>       parameter inquiries.
>       * match.c (gfc_match): Fix comment for %v code.
>       (gfc_match_allocate, gfc_match_deallocate): Replace use of %v code
>       by %e in gfc_match to allow for function references as STAT and
>       ERRMSG arguments.
>       * resolve.c (resolve_allocate_deallocate): Avoid NULL pointer
>       dereferences and shortcut for bad STAT and ERRMSG argument to
>       (DE)ALLOCATE.  Remove bogus parts of checks for STAT and ERRMSG.
>
> gcc/testsuite/ChangeLog:
>
>       PR fortran/101564
>       * gfortran.dg/allocate_stat_3.f90: New test.
>       * gfortran.dg/allocate_stat.f90: Adjust error messages.
>       * gfortran.dg/implicit_11.f90: Likewise.
>       * gfortran.dg/inquiry_type_ref_3.f90: Likewise.
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955