Re: [Patch] Fortran: Avoid double-free with parse error (PR96041, PR93423)
Hi Tobias, I see. Then OK for trunk by me. - Andre On Wed, 16 Sep 2020 10:35:53 +0200 Tobias Burnus wrote: > Hi Andre, > > On 9/16/20 9:58 AM, Andre Vehreschild wrote: > > + st->n.sym = NULL; > > > > Don't we need free or unlink the st node from the symtree, too? > > I did not see a way to simply remove a single symtree; as this > is the error case, I left the item in the symtree. > > The symtree itself is removed when the 'contains' is > processed by calling: >gfc_free_namespace (...) > which calls >free_sym_tree (ns->sym_root) > and that function is: >if (sym_tree == NULL) > return; >free_sym_tree (sym_tree->left); >free_sym_tree (sym_tree->right); >gfc_release_symbol (sym_tree->n.sym); >free (sym_tree); > > This "gfc_release_symbol" was actually the call which > handled our "bp" symbol – deleting n.sym->formal. > > The parent namespace was still present and called > resolve_formal – which den caused the ICE as its > n.sym->formal was before deleted in "our" namespace. > > Tobias > > - > Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany > Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, > Alexander Walter -- Andre Vehreschild * Email: vehre ad gmx dot de
Re: [Patch] Fortran: Avoid double-free with parse error (PR96041, PR93423)
Hi Andre, On 9/16/20 9:58 AM, Andre Vehreschild wrote: + st->n.sym = NULL; Don't we need free or unlink the st node from the symtree, too? I did not see a way to simply remove a single symtree; as this is the error case, I left the item in the symtree. The symtree itself is removed when the 'contains' is processed by calling: gfc_free_namespace (...) which calls free_sym_tree (ns->sym_root) and that function is: if (sym_tree == NULL) return; free_sym_tree (sym_tree->left); free_sym_tree (sym_tree->right); gfc_release_symbol (sym_tree->n.sym); free (sym_tree); This "gfc_release_symbol" was actually the call which handled our "bp" symbol – deleting n.sym->formal. The parent namespace was still present and called resolve_formal – which den caused the ICE as its n.sym->formal was before deleted in "our" namespace. Tobias - Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Re: [Patch] Fortran: Avoid double-free with parse error (PR96041, PR93423)
Hi Tobias,
diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index c612b492f3e..326e6f5db7a 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -9819,6 +9819,15 @@ gfc_match_submod_proc (void)
if (gfc_match_eos () != MATCH_YES)
{
+ /* Unset st->n.sym. Note: in reject_statement (), the symbol
changes are
+undone, such that the st->n.sym->formal points to the
original symbol;
+if now this namespace is finalized, the formal namespace is
freed,
+but it might be still needed in the parent namespace. */
+ gfc_symtree *st = gfc_find_symtree (gfc_current_ns->sym_root,
sym->name);
+ st->n.sym = NULL;
Don't we need free or unlink the st node from the symtree, too?
+ gfc_free_symbol (sym->tlink);
+ sym->tlink = NULL;
+ sym->refs--;
gfc_syntax_error (ST_MODULE_PROC);
return MATCH_ERROR;
}
Regards,
Andre
On Sat, 12 Sep 2020 23:00:12 +0200
Tobias Burnus wrote:
> The testcase for PR93423 did a double free, which caused
> an ICE. That's reported in PR96041.
>
> Slightly frustrated by the FAIL in the testsuite,
> I decided to debug and, hopefully, fix this.
>
> The problem is related to putting the symtree
> into a sub namespace of the symbol's ns. That's fixed up
> by copying things around – except in the error case where
> all those fixups are undone. Thus, when the symbol tree
> is deleted, the parent's sym->formal->sym is also deleted,
> causing an ICE in resolve_formal_arguments.
>
> Hopefully, I got this all right...
> I see still one memory leak for a symbol in module.c
> according to valgrind, but I don't know whether it is
> related to those symbols. (There are a lot of other leaks,
> mostly related to polymorphism (vtab etc.).)
>
> OK for the trunk?
>
> Tobias
>
--
Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
Tel.: +49 178 3837536 * ve...@gmx.de
Early ping — [Patch] Fortran: Avoid double-free with parse error (PR96041, PR93423)
Early Fortran-review ping. Solves: PR 96041 - [11 regression] ICE in gfortran.dg/pr93423.f90 after r11-1792 (opened 2020-07-03) The other PR revealed/caused the issue: PR 93423 - [8/9/10/11 Regression] ICE on invalid with argument list for module procedure "Backports will have to wait until PR96041 is resolved." (Comment 5) On 9/12/20 11:00 PM, Tobias Burnus wrote: The testcase for PR93423 did a double free, which caused an ICE. That's reported in PR96041. Slightly frustrated by the FAIL in the testsuite, I decided to debug and, hopefully, fix this. The problem is related to putting the symtree into a sub namespace of the symbol's ns. That's fixed up by copying things around – except in the error case where all those fixups are undone. Thus, when the symbol tree is deleted, the parent's sym->formal->sym is also deleted, causing an ICE in resolve_formal_arguments. Hopefully, I got this all right... I see still one memory leak for a symbol in module.c according to valgrind, but I don't know whether it is related to those symbols. (There are a lot of other leaks, mostly related to polymorphism (vtab etc.).) OK for the trunk? Tobias
[Patch] Fortran: Avoid double-free with parse error (PR96041, PR93423)
The testcase for PR93423 did a double free, which caused
an ICE. That's reported in PR96041.
Slightly frustrated by the FAIL in the testsuite,
I decided to debug and, hopefully, fix this.
The problem is related to putting the symtree
into a sub namespace of the symbol's ns. That's fixed up
by copying things around – except in the error case where
all those fixups are undone. Thus, when the symbol tree
is deleted, the parent's sym->formal->sym is also deleted,
causing an ICE in resolve_formal_arguments.
Hopefully, I got this all right...
I see still one memory leak for a symbol in module.c
according to valgrind, but I don't know whether it is
related to those symbols. (There are a lot of other leaks,
mostly related to polymorphism (vtab etc.).)
OK for the trunk?
Tobias
Fortran: Avoid double-free with parse error (PR96041, PR93423)
gcc/fortran/
PR fortran/96041
PR fortran/93423
* decl.c (gfc_match_submod_proc): Avoid later double-free
in the error case.
gcc/fortran/decl.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/gcc/fortran/decl.c b/gcc/fortran/decl.c
index c612b492f3e..326e6f5db7a 100644
--- a/gcc/fortran/decl.c
+++ b/gcc/fortran/decl.c
@@ -9819,6 +9819,15 @@ gfc_match_submod_proc (void)
if (gfc_match_eos () != MATCH_YES)
{
+ /* Unset st->n.sym. Note: in reject_statement (), the symbol changes are
+ undone, such that the st->n.sym->formal points to the original symbol;
+ if now this namespace is finalized, the formal namespace is freed,
+ but it might be still needed in the parent namespace. */
+ gfc_symtree *st = gfc_find_symtree (gfc_current_ns->sym_root, sym->name);
+ st->n.sym = NULL;
+ gfc_free_symbol (sym->tlink);
+ sym->tlink = NULL;
+ sym->refs--;
gfc_syntax_error (ST_MODULE_PROC);
return MATCH_ERROR;
}
