[libgfortran] Memory issues related to PR78549

Message ID 36391954-3ca0-34ee-85d2-c9196a3aeee3@charter.net
State New
Headers show
Series
  • [libgfortran] Memory issues related to PR78549
Related show

Commit Message

Jerry Dec. 11, 2017, 3:55 a.m.
Hi all,

While doing addition testing for the subject mentioned PR I discovered numerous
un-freed memory allocations. I reported the problem in comment 30 of the PR.

The attached patch cleans this up by opening the internal unit structures during
program initialization and allowing the automatic closure similar to
pre-connected units. The internal stream structures are created and freed at the
beginning and end of each I/O operation.  I fix a few loose ends.

Regression tested on x86_64.

OK for trunk?  Also needed for same issue in 7.

Regards,

Jerry

2017-12-11  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libgfortran/78549
	* io/inquire.c (inquire_via_unit): Adjust test for existence for
	pre-connected internal units.
	* io/transfer.c (finalize_transfer): When done with a transfer
	to internal units, free the format buffer and close the stream.
	(st_read_done): Delete freeing the stream, now handled using
	sclose in finalize_transfer. (st_write_done): Likewise.
	* io/unit.c (get_unit): Return NULL for special reserved unit
	numbers, signifying not accessible to the user.
	(init_units): Insert the two special internal units into the
	unit treap. This makes these unit structures available without
	further allocations for later use by internal unit I/O. These
	units are automatically deleted by normal program termination.
	* unix.c (mem_close): Add a guard check to protect from double free.

Comments

Jerry Dec. 13, 2017, 3:18 a.m. | #1
Ping: If no objection, I will commit to trunk on Wednesday evening.

On 12/10/2017 07:55 PM, Jerry DeLisle wrote:
> Hi all,

> 

> While doing addition testing for the subject mentioned PR I discovered numerous

> un-freed memory allocations. I reported the problem in comment 30 of the PR.

> 

> The attached patch cleans this up by opening the internal unit structures during

> program initialization and allowing the automatic closure similar to

> pre-connected units. The internal stream structures are created and freed at the

> beginning and end of each I/O operation.  I fix a few loose ends.

> 

> Regression tested on x86_64.

> 

> OK for trunk?  Also needed for same issue in 7.

> 

> Regards,

> 

> Jerry

> 

> 2017-12-11  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

> 

> 	PR libgfortran/78549

> 	* io/inquire.c (inquire_via_unit): Adjust test for existence for

> 	pre-connected internal units.

> 	* io/transfer.c (finalize_transfer): When done with a transfer

> 	to internal units, free the format buffer and close the stream.

> 	(st_read_done): Delete freeing the stream, now handled using

> 	sclose in finalize_transfer. (st_write_done): Likewise.

> 	* io/unit.c (get_unit): Return NULL for special reserved unit

> 	numbers, signifying not accessible to the user.

> 	(init_units): Insert the two special internal units into the

> 	unit treap. This makes these unit structures available without

> 	further allocations for later use by internal unit I/O. These

> 	units are automatically deleted by normal program termination.

> 	* unix.c (mem_close): Add a guard check to protect from double free.

>
Thomas Koenig Dec. 13, 2017, 7:18 a.m. | #2
Hi Jerry,

> Ping: If no objection, I will commit to trunk on Wednesday evening.


OK for trunk (even before that :-)

Thanks for the patch!

Regards

	Thomas

Patch

diff --git a/libgfortran/io/inquire.c b/libgfortran/io/inquire.c
index 848a08f6157..6ba1224d77c 100644
--- a/libgfortran/io/inquire.c
+++ b/libgfortran/io/inquire.c
@@ -47,7 +47,10 @@  inquire_via_unit (st_parameter_inquire *iqp, gfc_unit *u)
     generate_error (&iqp->common, LIBERROR_INQUIRE_INTERNAL_UNIT, NULL);
 
   if ((cf & IOPARM_INQUIRE_HAS_EXIST) != 0)
-    *iqp->exist = (u != NULL) || (iqp->common.unit >= 0);
+    *iqp->exist = (u != NULL &&
+		   iqp->common.unit != GFC_INTERNAL_UNIT &&
+		   iqp->common.unit != GFC_INTERNAL_UNIT4)
+		|| (iqp->common.unit >= 0);
 
   if ((cf & IOPARM_INQUIRE_HAS_OPENED) != 0)
     *iqp->opened = (u != NULL);
diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index 4d7ca7abf7b..211dc349750 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -3985,6 +3985,19 @@  finalize_transfer (st_parameter_dt *dtp)
   next_record (dtp, 1);
 
  done:
+
+  if (dtp->u.p.unit_is_internal)
+    {
+      fbuf_destroy (dtp->u.p.current_unit);
+      if (dtp->u.p.current_unit
+	  && (dtp->u.p.current_unit->child_dtio  == 0)
+	  && dtp->u.p.current_unit->s)
+	{
+	  sclose (dtp->u.p.current_unit->s);
+	  dtp->u.p.current_unit->s = NULL;
+	}
+    }
+
 #ifdef HAVE_USELOCALE
   if (dtp->u.p.old_locale != (locale_t) 0)
     {
@@ -4094,8 +4107,6 @@  st_read_done (st_parameter_dt *dtp)
 	    {
 	      free (dtp->u.p.current_unit->filename);
 	      dtp->u.p.current_unit->filename = NULL;
-	      free (dtp->u.p.current_unit->s);
-	      dtp->u.p.current_unit->s = NULL;
 	      if (dtp->u.p.current_unit->ls)
 		free (dtp->u.p.current_unit->ls);
 	      dtp->u.p.current_unit->ls = NULL;
@@ -4165,8 +4176,6 @@  st_write_done (st_parameter_dt *dtp)
 	    {
 	      free (dtp->u.p.current_unit->filename);
 	      dtp->u.p.current_unit->filename = NULL;
-	      free (dtp->u.p.current_unit->s);
-	      dtp->u.p.current_unit->s = NULL;
 	      if (dtp->u.p.current_unit->ls)
 		free (dtp->u.p.current_unit->ls);
 	      dtp->u.p.current_unit->ls = NULL;
diff --git a/libgfortran/io/unit.c b/libgfortran/io/unit.c
index 66cd12dcdcd..2ca8525fbec 100644
--- a/libgfortran/io/unit.c
+++ b/libgfortran/io/unit.c
@@ -566,7 +566,11 @@  get_unit (st_parameter_dt *dtp, int do_create)
      is not allowed, such units must be created with
      OPEN(NEWUNIT=...).  */
   if (dtp->common.unit < 0)
-    return get_gfc_unit (dtp->common.unit, 0);
+    {
+      if (dtp->common.unit > NEWUNIT_START) /* Reserved units.  */
+	return NULL;
+      return get_gfc_unit (dtp->common.unit, 0);
+    }
 
   return get_gfc_unit (dtp->common.unit, do_create);
 }
@@ -701,6 +705,9 @@  init_units (void)
 
       __gthread_mutex_unlock (&u->lock);
     }
+  /* The default internal units.  */
+  u = insert_unit (GFC_INTERNAL_UNIT);
+  u = insert_unit (GFC_INTERNAL_UNIT4);
 }
 
 
diff --git a/libgfortran/io/unix.c b/libgfortran/io/unix.c
index 61e9f7997b2..a07a3c9cea8 100644
--- a/libgfortran/io/unix.c
+++ b/libgfortran/io/unix.c
@@ -962,8 +962,8 @@  mem_flush (unix_stream *s __attribute__ ((unused)))
 static int
 mem_close (unix_stream *s)
 {
-  free (s);
-
+  if (s)
+    free (s);
   return 0;
 }