User account creation filtered due to spam.

Bug 31880 - [4.2 only] silent data corruption in gfortran read statement
Summary: [4.2 only] silent data corruption in gfortran read statement
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: libfortran (show other bugs)
Version: 4.1.2
: P3 critical
Target Milestone: ---
Assignee: Jerry DeLisle
URL:
Keywords: patch, wrong-code
Depends on:
Blocks: 19292
  Show dependency treegraph
 
Reported: 2007-05-09 16:56 UTC by Russell O'Connor
Modified: 2007-05-11 23:00 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 4.1.3 4.2.0 4.3.0
Last reconfirmed: 2007-05-10 01:07:10


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Russell O'Connor 2007-05-09 16:56:36 UTC
This program should print out "1".
Instead, it prints out "1024":

      program r3

      integer*4 a(1025),b(1025),c(1025),d(2048),e(1022)

      do i=1,2048
         d(i)=i
      end do

      open  (3,file='a',form='unformatted')
      write (3) a,b,c,d,e
      rewind 3
      read  (3) a,b,c,d
      close (3)

      print *,d(1)

      end

output of /opt/bin/gfortran -v:

Using built-in specs.
Target: x86_64-unknown-linux-gnu
Configured with: ../gcc-4.1.2/configure --prefix=/opt --disable-multilib
Thread model: posix
gcc version 4.1.2

It was compiled with:
/opt/bin/gfortran -Wall -o r3 r3.f

and run with:
LD_LIBRARY_PATH=/opt/lib64 ./r3

This patch fixes it:

--- gcc-4.1-4.1.2/gcc-4.1.2/libgfortran/io/unix.c	2006-05-29 22:51:26.000000000 -0400
+++ gcc-4.1-4.1.2_patched/gcc-4.1.2/libgfortran/io/unix.c	2007-05-08 19:22:09.000000000 -0400
@@ -465,7 +465,7 @@
       if (n < 0)
 	return NULL;
 
-      s->physical_offset = where + n;
+      s->physical_offset = m + n;
       s->active += n;
     }
   else
@@ -476,7 +476,7 @@
       if (do_read (s, s->buffer + s->active, &n) != 0)
 	return NULL;
 
-      s->physical_offset = where + n;
+      s->physical_offset = m + n;
       s->active += n;
     }


The problem is that s->physical_offset is being miscalculated as
where+n, which is the requested read start address plus the bytes read
from the disk to fill the buffer.  It should be where+s->active+n
(=m+n), the requested read start address plus the as-yet-unread bytes
still in the buffer plus the bytes read from the disk to fill the
buffer.  The s->logical_offset is correctly set to where+*len.

What happens is that if an unbuffered (<8192 bytes) read of more than
half the buffer (array b in the example) exceeds the data remaining in
the buffer, the remaining data will get moved to the beginning of the
buffer, and some amount of data will be read from disk to fill the
buffer.  If the next read (array c) is of the same length, it will
also exceed the data remaining in the buffer, the remaining data will
get moved to the beginning of the buffer, and this time the length of
the data read from the disk (n) will be the same as the requested read
length (*len).  Since the physical_offset is being set to where+n and
s->logical_offset is set to where+*len, they are equal.  If the next
read is >=8192 bytes (array d), it will be unbuffered, which would
normally cause the data remaining in the buffer to be ignored and
cause a seek to re-read that data along with the rest of the request,
from the disk.  But the code will decide that it doesn't have to seek
because physical_offset=logical_offset.  So the remaining data in the
buffer will be lost.
Comment 1 Francois-Xavier Coudert 2007-05-09 19:18:26 UTC
Confirmed on i686-linux, for all active branches.
Comment 2 kargl 2007-05-10 00:47:40 UTC
(In reply to comment #1)
> Confirmed on i686-linux, for all active branches.
> 

I'm not sure how you can confirm this.  The program is invalid.
a,b,c, and e are undefined in the write statement, so gfortran 
can do whateve it wants.
Comment 3 roconnor 2007-05-10 01:02:03 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > Confirmed on i686-linux, for all active branches.
> > 
> 
> I'm not sure how you can confirm this.  The program is invalid.
> a,b,c, and e are undefined in the write statement, so gfortran 
> can do whateve it wants.
> 

This also exhibits the bug:

      program r3

      integer*4 a(1025),b(1025),c(1025),d(2048),e(1022)

      a = 0
      b = 0
      c = 0
      e = 0

      do i=1,2048
         d(i)=i
      end do

      open  (3,file='a',form='unformatted')
      write (3) a,b,c,d,e
      rewind 3
      read  (3) a,b,c,d
      close (3)

      print *,d(1)

      end
Comment 4 Jerry DeLisle 2007-05-10 01:07:10 UTC
I am regression testing the patch.
Comment 5 Jerry DeLisle 2007-05-10 02:01:55 UTC
Subject: Bug 31880

Author: jvdelisle
Date: Thu May 10 01:01:27 2007
New Revision: 124588

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=124588
Log:
2007-05-09  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/31880
	* io/unix.c (fd_alloc_r_at): Fix calculation of physical offset.

Modified:
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/io/unix.c

Comment 6 Jerry DeLisle 2007-05-10 02:10:15 UTC
Subject: Bug 31880

Author: jvdelisle
Date: Thu May 10 01:09:57 2007
New Revision: 124590

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=124590
Log:
2007-05-09  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/31880
	* gfortran.dg/unf_read_corrupted_2.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/unf_read_corrupted_2.f90
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 7 Jerry DeLisle 2007-05-10 02:14:33 UTC
Russel, thanks for finding this and reporting it.  I assume you do not have copyright assignment.  I committed the patch because it is very small and simple.

Thanks much!
Comment 8 Jerry DeLisle 2007-05-10 04:22:55 UTC
Subject: Bug 31880

Author: jvdelisle
Date: Thu May 10 03:22:40 2007
New Revision: 124591

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=124591
Log:
2007-05-09  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/31880
	* gfortran.dg/unf_read_corrupted_2.f90: Fix test.

Modified:
    trunk/gcc/testsuite/gfortran.dg/unf_read_corrupted_2.f90

Comment 9 Russell O'Connor 2007-05-10 12:50:08 UTC
(In reply to comment #7)
> Russel, thanks for finding this and reporting it.  I assume you do not have
> copyright assignment.  I committed the patch because it is very small and
> simple.
> 
You're welcome.  Thank you for the prompt response.
You're right about the copyright assignment.  But even with the test case, it comes in under the FSF's 15-line threshold for legally significant.

Comment 10 patchapp@dberlin.org 2007-05-10 17:25:30 UTC
Subject: Bug number PR31880

A patch for this bug has been added to the patch tracker.
The mailing list url for the patch is http://gcc.gnu.org/ml/gcc-patches/2007-05/msg00728.html
Comment 11 Jerry DeLisle 2007-05-11 01:34:54 UTC
Subject: Bug 31880

Author: jvdelisle
Date: Fri May 11 00:34:41 2007
New Revision: 124609

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=124609
Log:
2007-05-10  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/31880
	* io/unix.c (fd_alloc_r_at): Fix calculation of physical offset.

2007-05-10  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/31880
	* gfortran.dg/unf_read_corrupted_2.f90: New test.

Added:
    branches/gcc-4_1-branch/gcc/testsuite/gfortran.dg/unf_read_corrupted_2.f90
Modified:
    branches/gcc-4_1-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_1-branch/libgfortran/ChangeLog
    branches/gcc-4_1-branch/libgfortran/io/unix.c

Comment 12 Thomas Koenig 2007-05-11 19:16:33 UTC
This is a regression WRT g77.
Comment 13 Thomas Koenig 2007-05-11 19:19:48 UTC
Mark, this bug is critical - silent data corruption upon read
is really, really bad.

I second Jerry's DeLisle's request for inclusion in 4.2.  This is a
regression versus g77, the fix is simple and safe.
Comment 14 Mark Mitchell 2007-05-11 19:45:34 UTC
Subject: Re:  [4.2 only] silent data corruption in gfortran
 read statement

tkoenig at gcc dot gnu dot org wrote:

> I second Jerry's DeLisle's request for inclusion in 4.2.  This is a
> regression versus g77, the fix is simple and safe.

OK, please apply the patch to 4.2.

However, please note for future that a patch should never go on 4.1
before it has gone on 4.2: we want to be sure that new regressions are
not being introduced!

Comment 15 Jerry DeLisle 2007-05-11 22:54:10 UTC
Subject: Bug 31880

Author: jvdelisle
Date: Fri May 11 21:53:52 2007
New Revision: 124626

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=124626
Log:
2007-05-11  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/31880
	* io/unix.c (fd_alloc_r_at): Fix calculation of physical offset.

Modified:
    branches/gcc-4_2-branch/libgfortran/ChangeLog
    branches/gcc-4_2-branch/libgfortran/io/unix.c

Comment 16 Jerry DeLisle 2007-05-11 22:57:18 UTC
Subject: Bug 31880

Author: jvdelisle
Date: Fri May 11 21:57:05 2007
New Revision: 124627

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=124627
Log:
2007-05-11  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/31880
	* gfortran.dg/unf_read_corrupted_2.f90: New test.

Added:
    branches/gcc-4_2-branch/gcc/testsuite/gfortran.dg/unf_read_corrupted_2.f90
Modified:
    branches/gcc-4_2-branch/gcc/testsuite/ChangeLog

Comment 17 Jerry DeLisle 2007-05-11 23:00:39 UTC
Fixed on 4.2, Closing.  Sorry for the premature commit on 4.1.