diff mbox

[libgfortran,7/8,Regression] Crash of runtime gfortran library during integer transformation

Message ID eb5c03da-e195-78fd-6cac-01cb625f9092@charter.net
State New
Headers show

Commit Message

Jerry DeLisle May 15, 2017, 8:10 p.m. UTC 
Hi all,

Crash is a misnomer on this PR [aside: People see the backtrace and assume]

This patch fixes the problem by correctly detecting the EOR condition for 
internal units. The previous check in read_sf_internal was wrong, relying 
probably on uninitialized memory as can be seen by the still open PR78881. 
Removing the bad hunk fixes the regression here and the new code lets 
dtio_26.f90 pass as expected.

Regression tested on x86_64. New test case will be added.

OK for trunk? Will back port in a few days to 7.

I will also have Rainer verify it fixes the problem on sparc (78881)

Regards,

Jerry

2017-05-15  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libgfortran/80727
	* transfer.c (read_sf_internal): Remove bogus code to detect EOR.
	(read_block_form): For internal units, generate EOR if no more
	bytes left in unit and we are trying to read with ADVANCE='NO'.

Comments

Steve Kargl May 15, 2017, 8:33 p.m. UTC | #1 
On Mon, May 15, 2017 at 01:10:43PM -0700, Jerry DeLisle wrote:
> Hi all,
> 
> Crash is a misnomer on this PR [aside: People see the backtrace and assume]
> 
> This patch fixes the problem by correctly detecting the EOR condition for 
> internal units. The previous check in read_sf_internal was wrong, relying 
> probably on uninitialized memory as can be seen by the still open PR78881. 
> Removing the bad hunk fixes the regression here and the new code lets 
> dtio_26.f90 pass as expected.
> 
> Regression tested on x86_64. New test case will be added.
> 
> OK for trunk? Will back port in a few days to 7.
> 

No.  There are a number of other failures with your patch applied.

Running /home/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/dg.exp ...
FAIL: gfortran.dg/read_3.f90   -O0  (test for excess errors)
FAIL: gfortran.dg/read_3.f90   -O1  (test for excess errors)
FAIL: gfortran.dg/read_3.f90   -O2  (test for excess errors)
FAIL: gfortran.dg/read_3.f90   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
FAIL: gfortran.dg/read_3.f90   -O3 -g  (test for excess errors)
FAIL: gfortran.dg/read_3.f90   -Os  (test for excess errors)

                === gfortran Summary ===

# of expected passes            6
# of unexpected failures        6
/mnt/sgk/objx/gcc/gfortran  version 8.0.0 20170515 (experimental) (GCC)
Steve Kargl May 15, 2017, 8:54 p.m. UTC | #2 
On Mon, May 15, 2017 at 01:33:55PM -0700, Steve Kargl wrote:
> On Mon, May 15, 2017 at 01:10:43PM -0700, Jerry DeLisle wrote:
> > Hi all,
> > 
> > Crash is a misnomer on this PR [aside: People see the backtrace and assume]
> > 
> > This patch fixes the problem by correctly detecting the EOR condition for 
> > internal units. The previous check in read_sf_internal was wrong, relying 
> > probably on uninitialized memory as can be seen by the still open PR78881. 
> > Removing the bad hunk fixes the regression here and the new code lets 
> > dtio_26.f90 pass as expected.
> > 
> > Regression tested on x86_64. New test case will be added.
> > 
> > OK for trunk? Will back port in a few days to 7.
> > 
> 
> No.  There are a number of other failures with your patch applied.
> 
> Running /home/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/dg.exp ...
> FAIL: gfortran.dg/read_3.f90   -O0  (test for excess errors)
> FAIL: gfortran.dg/read_3.f90   -O1  (test for excess errors)
> FAIL: gfortran.dg/read_3.f90   -O2  (test for excess errors)
> FAIL: gfortran.dg/read_3.f90   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  (test for excess errors)
> FAIL: gfortran.dg/read_3.f90   -O3 -g  (test for excess errors)
> FAIL: gfortran.dg/read_3.f90   -Os  (test for excess errors)
> 

The failures are from an unexpected warning.

Executing on host: /mnt/sgk/objx/gcc/testsuite/gfortran2/../../gfortran -B/mnt/sgk/objx/gcc/testsuite/gfortran2/../../ -B/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/ /home/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/read_3.f90    -fno-diagnostics-show-caret -fdiagnostics-color=never    -O0   -pedantic-errors  -B/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libatomic/.libs -B/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libquadmath/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libquadmath/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libquadmath/.libs  -lm  -o ./read_3.exe    (timeout = 300)
spawn -ignore SIGHUP /mnt/sgk/objx/gcc/testsuite/gfortran2/../../gfortran -B/mnt/sgk/objx/gcc/testsuite/gfortran2/../../ -B/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/ /home/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/read_3.f90 -fno-diagnostics-show-caret -fdiagnostics-color=never -O0 -pedantic-errors -B/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libgfortran/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libatomic/.libs -B/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libquadmath/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libquadmath/.libs -L/mnt/sgk/objx/x86_64-unknown-freebsd12.0/./libquadmath/.libs -lm -o ./read_3.exe

/home/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/read_3.f90:6:11: Warning: GNU Extension: Nonstandard type declaration INTEGER*4 at (1)

FAIL: gfortran.dg/read_3.f90   -O0  (test for excess errors)
Excess errors:
/home/sgk/gcc/gccx/gcc/testsuite/gfortran.dg/read_3.f90:6:11: Warning: GNU Extension: Nonstandard type declaration INTEGER*4 at (1)
Steve Kargl May 15, 2017, 10:51 p.m. UTC | #3 
On Mon, May 15, 2017 at 01:10:43PM -0700, Jerry DeLisle wrote:
> 
> 2017-05-15  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
> 
> 	PR libgfortran/80727
> 	* transfer.c (read_sf_internal): Remove bogus code to detect EOR.
> 	(read_block_form): For internal units, generate EOR if no more
> 	bytes left in unit and we are trying to read with ADVANCE='NO'.

OK with change below.

> 
> ! { dg-do run }
> ! PR80727 Crash of runtime gfortran library during integer transformation
> ! Note: before the patch this was giving an incorrect EOR error on READ.
> program    gfortran_710_io_bug
>   character  str*4
>   integer*4  i4

    integer(4)

>   str =''
>   i = 256
>   write(str,fmt='(a)') i
>   i = 0
>   read ( unit=str(1:4), fmt='(a)' ) i4
>   if (i4.ne.256) call abort
> end  program  gfortran_710_io_bug
Jerry DeLisle May 15, 2017, 11:21 p.m. UTC | #4 
On 05/15/2017 03:51 PM, Steve Kargl wrote:
> On Mon, May 15, 2017 at 01:10:43PM -0700, Jerry DeLisle wrote:
>>
>> 2017-05-15  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
>>
>> 	PR libgfortran/80727
>> 	* transfer.c (read_sf_internal): Remove bogus code to detect EOR.
>> 	(read_block_form): For internal units, generate EOR if no more
>> 	bytes left in unit and we are trying to read with ADVANCE='NO'.
> 
> OK with change below.
> 
>>
>> ! { dg-do run }
>> ! PR80727 Crash of runtime gfortran library during integer transformation
>> ! Note: before the patch this was giving an incorrect EOR error on READ.
>> program    gfortran_710_io_bug
>>   character  str*4
>>   integer*4  i4
> 
>     integer(4)
> 
>>   str =''
>>   i = 256
>>   write(str,fmt='(a)') i
>>   i = 0
>>   read ( unit=str(1:4), fmt='(a)' ) i4
>>   if (i4.ne.256) call abort
>> end  program  gfortran_710_io_bug 
> 

Oh, thanks Steve. I forgot to run the new test with the machinery.

Will fix, check it one more time, then commit.

Jerry
diff mbox

Patch

diff --git a/libgfortran/io/transfer.c b/libgfortran/io/transfer.c
index f16d8c55..928a448f 100644
--- a/libgfortran/io/transfer.c
+++ b/libgfortran/io/transfer.c
@@ -272,12 +272,6 @@  read_sf_internal (st_parameter_dt *dtp, int *length)
       return NULL;
     }
 
-  if (base && *base == 0)
-    {
-      generate_error (&dtp->common, LIBERROR_EOR, NULL);
-      return NULL;
-    }
-
   dtp->u.p.current_unit->bytes_left -= *length;
 
   if (((dtp->common.flags & IOPARM_DT_HAS_SIZE) != 0) ||
@@ -470,11 +464,24 @@  read_block_form (st_parameter_dt *dtp, int *nbytes)
 		}
 	    }
 
-	  if (unlikely (dtp->u.p.current_unit->bytes_left == 0
-	      && !is_internal_unit(dtp)))
+	  if (is_internal_unit(dtp))
 	    {
-	      hit_eof (dtp);
-	      return NULL;
+	      if (*nbytes > 0 && dtp->u.p.current_unit->bytes_left == 0)
+	        {
+		  if (dtp->u.p.advance_status == ADVANCE_NO)
+		    {
+		      generate_error (&dtp->common, LIBERROR_EOR, NULL);
+		      return NULL;
+		    }
+		}
+	    }
+	  else
+	    {
+	      if (unlikely (dtp->u.p.current_unit->bytes_left == 0))
+		{
+		  hit_eof (dtp);
+		  return NULL;
+		}
 	    }
 
 	  *nbytes = dtp->u.p.current_unit->bytes_left;