[Patch, Fortran] PR5469

[Patch, Fortran] PR5469

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

l_push_char allocates memory which is freed with free_line. However, 
currently, the memory is not always freed when calling generate_error.

If one aborts, that's fine. However, generate_error can also set the 
iostat variable. Thus, one should ensure that the memory is always freed.

I wouldn't mind if someone, who knows libgfortran/io better than I, 
could confirm that the patch is okay. I don't want too free memory which 
is later used.

Build and regtested on x86-64-linux.
OK for the trunk?

Tobias


PS: Test case see PR. (I don't think it makes sense to include it in the 
test suite.)

PPS: I see the following failures when regtesting; the LTO and the 
realloc one I know, but I haven't see the reassoc_4.f before. It cannot 
be due to my patch and when I run it manually, it also works:

FAIL: gfortran.dg/lto/pr45586 f_lto_pr45586_0.o-f_lto_pr45586_0.o link, 
-O0 -flto  (internal compiler error)
FAIL: gfortran.dg/realloc_on_assign_5.f03  -O0  execution test
FAIL: gfortran.dg/reassoc_4.f  -O   scan-tree-dump-times reassoc1 "[0-9] 
\\* " 22

[-- Attachment #2: io_free_line.diff --]
[-- Type: text/x-patch, Size: 3875 bytes --]

2012-11-26  Tobias Burnus  <burnus@net-b.de>

	PR fortran/5469
	* io/list_read (parse_repeat, read_integer, read_character,
	parse_real, read_real, check_type, list_formatted_read_scalar,
	finish_list_read): Call list_free.

diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index 403e719..49615b3 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -617,6 +617,7 @@ parse_repeat (st_parameter_dt *dtp)
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return 1;
     }
@@ -905,11 +906,14 @@ read_integer (st_parameter_dt *dtp, int length)
   free_saved (dtp);  
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return;
     }
   else if (c != '\n')
     eat_line (dtp);
+
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad integer for item %d in list input",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1078,7 +1082,6 @@ read_character (st_parameter_dt *dtp, int length __attribute__ ((unused)))
       unget_char (dtp, c);
       eat_separator (dtp);
       dtp->u.p.saved_type = BT_CHARACTER;
-      free_line (dtp);
     }
   else 
     {
@@ -1087,10 +1090,12 @@ read_character (st_parameter_dt *dtp, int length __attribute__ ((unused)))
 		  dtp->u.p.item_count);
       generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
     }
+  free_line (dtp);
   return;
 
  eof:
   free_saved (dtp);
+  free_line (dtp);
   hit_eof (dtp);
 }
 
@@ -1283,11 +1288,14 @@ parse_real (st_parameter_dt *dtp, void *buffer, int length)
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return 1;
     }
   else if (c != '\n')
     eat_line (dtp);
+
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad floating point number for item %d",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1387,11 +1395,14 @@ eol_4:
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return;
     }
   else if (c != '\n')   
     eat_line (dtp);
+
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad complex value in item %d of list input",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1762,12 +1773,14 @@ read_real (st_parameter_dt *dtp, void * dest, int length)
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return;
     }
   else if (c != '\n')
     eat_line (dtp);
 
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad real number in item %d of list input",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1784,6 +1797,7 @@ check_type (st_parameter_dt *dtp, bt type, int len)
 
   if (dtp->u.p.saved_type != BT_UNKNOWN && dtp->u.p.saved_type != type)
     {
+      free_line (dtp);
       snprintf (message, MSGLEN, "Read type %s where %s was expected for item %d",
 		  type_name (dtp->u.p.saved_type), type_name (type),
 		  dtp->u.p.item_count);
@@ -1797,6 +1811,7 @@ check_type (st_parameter_dt *dtp, bt type, int len)
 
   if (dtp->u.p.saved_length != len)
     {
+      free_line (dtp);
       snprintf (message, MSGLEN,
 		  "Read kind %d %s where kind %d is required for item %d",
 		  dtp->u.p.saved_length, type_name (dtp->u.p.saved_type), len,
@@ -1970,7 +1985,10 @@ list_formatted_read_scalar (st_parameter_dt *dtp, bt type, void *p,
 
 cleanup:
   if (err == LIBERROR_END)
-    hit_eof (dtp);
+    {
+      free_line (dtp);
+      hit_eof (dtp);
+    }
   return err;
 }
 
@@ -2018,7 +2036,10 @@ finish_list_read (st_parameter_dt *dtp)
 
   err = eat_line (dtp);
   if (err == LIBERROR_END)
-    hit_eof (dtp);
+    {
+      free_line (dtp);
+      hit_eof (dtp);
+    }
 }
 
 /*			NAMELIST INPUT

Re: [Patch, Fortran] PR55469 - fix I/O memory leaks in case of failure and iostat= being present

[Tobias Burnus]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

Tobias Burnus wrote:
> l_push_char allocates memory which is freed with free_line. However, 
> currently, the memory is not always freed when calling generate_error. 
> If one aborts, that's fine. However, generate_error can also set the 
> iostat variable.

Updated version: Corrected PR number - and ensured that if convert_real 
fails, free_saved is called (cf. additional test case in the PR).

Build and regtested on x86-64-gnu-linux.
OK for the trunk?

Tobias

[-- Attachment #2: io_free_line-v2.diff --]
[-- Type: text/x-patch, Size: 4195 bytes --]

2012-11-29  Tobias Burnus  <burnus@net-b.de>

	PR fortran/55469
	* io/list_read (parse_repeat, read_integer, read_character,
	parse_real, read_real, check_type, list_formatted_read_scalar,
	finish_list_read): Call list_free.

diff --git a/libgfortran/io/list_read.c b/libgfortran/io/list_read.c
index 403e719..5063c36 100644
--- a/libgfortran/io/list_read.c
+++ b/libgfortran/io/list_read.c
@@ -617,6 +617,7 @@ parse_repeat (st_parameter_dt *dtp)
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return 1;
     }
@@ -905,11 +906,14 @@ read_integer (st_parameter_dt *dtp, int length)
   free_saved (dtp);  
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return;
     }
   else if (c != '\n')
     eat_line (dtp);
+
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad integer for item %d in list input",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1078,7 +1082,6 @@ read_character (st_parameter_dt *dtp, int length __attribute__ ((unused)))
       unget_char (dtp, c);
       eat_separator (dtp);
       dtp->u.p.saved_type = BT_CHARACTER;
-      free_line (dtp);
     }
   else 
     {
@@ -1087,10 +1090,12 @@ read_character (st_parameter_dt *dtp, int length __attribute__ ((unused)))
 		  dtp->u.p.item_count);
       generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
     }
+  free_line (dtp);
   return;
 
  eof:
   free_saved (dtp);
+  free_line (dtp);
   hit_eof (dtp);
 }
 
@@ -1283,11 +1288,14 @@ parse_real (st_parameter_dt *dtp, void *buffer, int length)
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return 1;
     }
   else if (c != '\n')
     eat_line (dtp);
+
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad floating point number for item %d",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1387,11 +1395,14 @@ eol_4:
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return;
     }
   else if (c != '\n')   
     eat_line (dtp);
+
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad complex value in item %d of list input",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1624,7 +1635,10 @@ read_real (st_parameter_dt *dtp, void * dest, int length)
   eat_separator (dtp);
   push_char (dtp, '\0');
   if (convert_real (dtp, dest, dtp->u.p.saved_string, length))
-    return;
+    {
+      free_saved (dtp);
+      return;
+    }
 
   free_saved (dtp);
   dtp->u.p.saved_type = BT_REAL;
@@ -1762,12 +1776,14 @@ read_real (st_parameter_dt *dtp, void * dest, int length)
   free_saved (dtp);
   if (c == EOF)
     {
+      free_line (dtp);
       hit_eof (dtp);
       return;
     }
   else if (c != '\n')
     eat_line (dtp);
 
+  free_line (dtp);
   snprintf (message, MSGLEN, "Bad real number in item %d of list input",
 	      dtp->u.p.item_count);
   generate_error (&dtp->common, LIBERROR_READ_VALUE, message);
@@ -1784,6 +1800,7 @@ check_type (st_parameter_dt *dtp, bt type, int len)
 
   if (dtp->u.p.saved_type != BT_UNKNOWN && dtp->u.p.saved_type != type)
     {
+      free_line (dtp);
       snprintf (message, MSGLEN, "Read type %s where %s was expected for item %d",
 		  type_name (dtp->u.p.saved_type), type_name (type),
 		  dtp->u.p.item_count);
@@ -1797,6 +1814,7 @@ check_type (st_parameter_dt *dtp, bt type, int len)
 
   if (dtp->u.p.saved_length != len)
     {
+      free_line (dtp);
       snprintf (message, MSGLEN,
 		  "Read kind %d %s where kind %d is required for item %d",
 		  dtp->u.p.saved_length, type_name (dtp->u.p.saved_type), len,
@@ -1970,7 +1988,10 @@ list_formatted_read_scalar (st_parameter_dt *dtp, bt type, void *p,
 
 cleanup:
   if (err == LIBERROR_END)
-    hit_eof (dtp);
+    {
+      free_line (dtp);
+      hit_eof (dtp);
+    }
   return err;
 }
 
@@ -2018,7 +2039,10 @@ finish_list_read (st_parameter_dt *dtp)
 
   err = eat_line (dtp);
   if (err == LIBERROR_END)
-    hit_eof (dtp);
+    {
+      free_line (dtp);
+      hit_eof (dtp);
+    }
 }
 
 /*			NAMELIST INPUT

Re: [Patch, Fortran] PR55469 - fix I/O memory leaks in case of failure and iostat= being present

[Janne Blomqvist]

On Thu, Nov 29, 2012 at 1:38 PM, Tobias Burnus <burnus@net-b.de> wrote:
> Tobias Burnus wrote:
>>
>> l_push_char allocates memory which is freed with free_line. However,
>> currently, the memory is not always freed when calling generate_error. If
>> one aborts, that's fine. However, generate_error can also set the iostat
>> variable.
>
>
> Updated version: Corrected PR number - and ensured that if convert_real
> fails, free_saved is called (cf. additional test case in the PR).
>
> Build and regtested on x86-64-gnu-linux.
> OK for the trunk?

IIRC this is supposed to be a cache than can be subsequently reused
without freeing and allocating it again. So it might be better to free
it once when the unit is closed.

At some point this line buffer should be removed completely and
replaced with the fbuf_*() machinery. But so far nobody has found the
time to work on that.

-- 
Janne Blomqvist

Re: [Patch, Fortran] PR55469 - fix I/O memory leaks in case of failure and iostat= being present

[Tobias Burnus]

I have committed the patch after Janne's approval on IRC  and fresh 
building/regtesting as Rev. 203086. See 
http://gcc.gnu.org/ml/fortran/2012-11/msg00092.html

As suggested by Janne, I will backport it to 4.8 in in a bunch of days.

Tobias

On November 29, 2012 12:38, Tobias Burnus wrote:
> Tobias Burnus wrote:
>> l_push_char allocates memory which is freed with free_line. However, 
>> currently, the memory is not always freed when calling 
>> generate_error. If one aborts, that's fine. However, generate_error 
>> can also set the iostat variable.
>
> Updated version: Corrected PR number - and ensured that if 
> convert_real fails, free_saved is called (cf. additional test case in 
> the PR).
>
> Build and regtested on x86-64-gnu-linux.
> OK for the trunk?
>
> Tobias