Re: [PATCH] fortran: Restore interface to its previous state on error [PR48776]

2023-08-30 Thread Mikael Morin via Gcc-patches

Le 28/08/2023 à 21:17, Harald Anlauf via Fortran a écrit :

Hi Mikael,

On 8/27/23 21:22, Mikael Morin via Gcc-patches wrote:

Hello,

this fixes an old error-recovery bug.
Tested on x86_64-pc-linux-gnu.

OK for master?


I have only a minor comment:

+/* Free the leading members of the gfc_interface linked list given in 
INTR

+   up to the END element (exclusive: the END element is not freed).
+   If END is not nullptr, it is assumed that END is in the linked 
list starting

+   with INTR.  */
+
+static void
+free_interface_elements_until (gfc_interface *intr, gfc_interface *end)
+{
+  gfc_interface *next;
+
+  for (; intr != end; intr = next)


Would it make sense to add a protection for intr == NULL, i.e.:

+  for (; intr && intr != end; intr = next)

Just to prevent a NULL pointer dereference in case there
is a corruption of the chain or something else went wrong.

This would happen in the case END is not a member of the INTR linked 
list.  In that case, the most forgiving would be not freeing any memory 
and just returning.  But it would require walking the list a second time 
to determine before proceeding if END is present, and let's not do work 
that is expected to be useless.


I will just do the change as you suggest it.


Otherwise it looks good to me.

It appears that your patch similarly fixes PR107923.  :-)


Good news. :-)
I will double check that none of the testcases there remain unfixed and 
close as duplicate.


I don't know how you manage to make your way through the hundreds of 
open PRs by the way.


Thanks for the review.


Thanks for the patch!

Harald






Re: [PATCH] fortran: Restore interface to its previous state on error [PR48776]

2023-08-28 Thread Harald Anlauf via Gcc-patches

Hi Mikael,

On 8/27/23 21:22, Mikael Morin via Gcc-patches wrote:

Hello,

this fixes an old error-recovery bug.
Tested on x86_64-pc-linux-gnu.

OK for master?


I have only a minor comment:


+/* Free the leading members of the gfc_interface linked list given in INTR
+   up to the END element (exclusive: the END element is not freed).
+   If END is not nullptr, it is assumed that END is in the linked list starting
+   with INTR.  */
+
+static void
+free_interface_elements_until (gfc_interface *intr, gfc_interface *end)
+{
+  gfc_interface *next;
+
+  for (; intr != end; intr = next)


Would it make sense to add a protection for intr == NULL, i.e.:

+  for (; intr && intr != end; intr = next)

Just to prevent a NULL pointer dereference in case there
is a corruption of the chain or something else went wrong.

Otherwise it looks good to me.

It appears that your patch similarly fixes PR107923.  :-)

Thanks for the patch!

Harald




[PATCH] fortran: Restore interface to its previous state on error [PR48776]

2023-08-27 Thread Mikael Morin via Gcc-patches
Hello,

this fixes an old error-recovery bug.
Tested on x86_64-pc-linux-gnu.

OK for master?

-- >8 --

Keep memory of the content of the current interface body being parsed
and restore it to its previous state if it has been modified at the time
a parse attempt fails.

This fixes memory errors and random segmentation faults caused by
dangling symbol pointers kept in interfaces' linked lists of symbols.
If a parsing attempt fails and symbols are freed, they should also be
removed from the current interface linked list.

As the list of symbol is a linked list, and parsing only adds new
symbols to the head of the list, all that is needed to track the
previous content of the list is a pointer to its previous head.
This adds such a pointer, and the restoration of the list of symbols
to that pointer on error.

PR fortran/48776

gcc/fortran/ChangeLog:

* gfortran.h (gfc_drop_interface_elements_before): New prototype.
(gfc_current_interface_head): Return a reference to the pointer.
* interface.cc (gfc_current_interface_head): Ditto.
(free_interface_elements_until): New function, generalizing
gfc_free_interface.
(gfc_free_interface): Use free_interface_elements_until.
(gfc_drop_interface_elements_before): New function.
* parse.cc
(current_interface_ptr, previous_interface_head): New static variables.
(current_interface_valid_p, get_current_interface_ptr): New functions.
(decode_statement): Initialize previous_interface_head.
(reject_statement): Restore current interface pointer to point to
previous_interface_head.

gcc/testsuite/ChangeLog:

* gfortran.dg/interface_procedure_1.f90: New test.
---
 gcc/fortran/gfortran.h|  3 +-
 gcc/fortran/interface.cc  | 43 ---
 gcc/fortran/parse.cc  | 54 +++
 .../gfortran.dg/interface_procedure_1.f90 | 23 
 4 files changed, 115 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/gfortran.dg/interface_procedure_1.f90

diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index fd47000a88e..0fabe7badde 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -3824,6 +3824,7 @@ bool gfc_ref_dimen_size (gfc_array_ref *, int dimen, 
mpz_t *, mpz_t *);
 
 /* interface.cc -- FIXME: some of these should be in symbol.cc */
 void gfc_free_interface (gfc_interface *);
+void gfc_drop_interface_elements_before (gfc_interface **, gfc_interface *);
 bool gfc_compare_derived_types (gfc_symbol *, gfc_symbol *);
 bool gfc_compare_types (gfc_typespec *, gfc_typespec *);
 bool gfc_check_dummy_characteristics (gfc_symbol *, gfc_symbol *,
@@ -3843,7 +3844,7 @@ void gfc_free_formal_arglist (gfc_formal_arglist *);
 bool gfc_extend_assign (gfc_code *, gfc_namespace *);
 bool gfc_check_new_interface (gfc_interface *, gfc_symbol *, locus);
 bool gfc_add_interface (gfc_symbol *);
-gfc_interface *gfc_current_interface_head (void);
+gfc_interface *_current_interface_head (void);
 void gfc_set_current_interface_head (gfc_interface *);
 gfc_symtree* gfc_find_sym_in_symtree (gfc_symbol*);
 bool gfc_arglist_matches_symbol (gfc_actual_arglist**, gfc_symbol*);
diff --git a/gcc/fortran/interface.cc b/gcc/fortran/interface.cc
index ea82056e9e3..c01df0460d7 100644
--- a/gcc/fortran/interface.cc
+++ b/gcc/fortran/interface.cc
@@ -78,18 +78,47 @@ along with GCC; see the file COPYING3.  If not see
 gfc_interface_info current_interface;
 
 
+/* Free the leading members of the gfc_interface linked list given in INTR
+   up to the END element (exclusive: the END element is not freed).
+   If END is not nullptr, it is assumed that END is in the linked list starting
+   with INTR.  */
+
+static void
+free_interface_elements_until (gfc_interface *intr, gfc_interface *end)
+{
+  gfc_interface *next;
+
+  for (; intr != end; intr = next)
+{
+  next = intr->next;
+  free (intr);
+}
+}
+
+
 /* Free a singly linked list of gfc_interface structures.  */
 
 void
 gfc_free_interface (gfc_interface *intr)
 {
-  gfc_interface *next;
+  free_interface_elements_until (intr, nullptr);
+}
 
-  for (; intr; intr = next)
-{
-  next = intr->next;
-  free (intr);
-}
+
+/* Update the interface pointer given by IFC_PTR to make it point to TAIL.
+   It is expected that TAIL (if non-null) is in the list pointed to by
+   IFC_PTR, hence the tail of it.  The members of the list before TAIL are
+   freed before the pointer reassignment.  */
+
+void
+gfc_drop_interface_elements_before (gfc_interface **ifc_ptr,
+   gfc_interface *tail)
+{
+  if (ifc_ptr == nullptr)
+return;
+
+  free_interface_elements_until (*ifc_ptr, tail);
+  *ifc_ptr = tail;
 }
 
 
@@ -4953,7 +4982,7 @@ gfc_add_interface (gfc_symbol *new_sym)
 }
 
 
-gfc_interface *
+gfc_interface *&
 gfc_current_interface_head (void)
 {
   switch (current_interface.type)
diff