Message ID | trinity-6ca0c1c7-5c7d-41e0-ac49-aefde33e340b-1626899770647@3c-app-gmx-bap01 |
---|---|
State | New |
Headers | show |
Series | PR fortrsn/101564 - ICE in resolve_allocate_deallocate, at fortran/resolve.c:8169 | expand |
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
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; + 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