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

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
>

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 <burnus@net-b.de> 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
>

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

Hi Tobias,

I see. Then OK for trunk by me.

- Andre

On Wed, 16 Sep 2020 10:35:53 +0200
Tobias Burnus <tobias@codesourcery.com> 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