Re: [PATCH][BZ 15362] Fix fwrite() reading beyond end of buffer in error path

[Eric Biggers <ebiggers3 at gmail dot com>]

> 
> Run make check with -k and record which tests fail due to this patch
> (if any).
> 

I must say that running the test suite was not particularly easy for a newcomer.
There were over a hundred test failures for no apparent reason regardless of
whether my patch was applied.  Finally, based on the wiki (`make check' really
should warn you about this itself...), I configured with '--prefix=/usr', which
fixed most of the failing tests.  However, some tests still failed even without
my patch applied (using the master branch at commit
e913141d5f4d4eed4b65f55b0077aeb1c8234e25):

  make[2]: *** [/home/e/src/glibc/build-master2/math/test-ldouble.out] Error 1
  make[2]: *** [/home/e/src/glibc/build-master2/math/test-ildoubl.out] Error 1
  make[1]: *** [math/tests] Error 2
  make[2]: [/home/e/src/glibc/build-master2/posix/annexc.out] Error 1 (ignored)
  make[2]: *** [/home/e/src/glibc/build-master2/rt/tst-cpuclock2.out] Error 1
  make[1]: *** [rt/tests] Error 2
  make[2]: [/home/e/src/glibc/build-master2/conform/run-conformtest.out] Error 1 (ignored)
  make: *** [check] Error 2

The failed tests with my patch applied were the same, except for
'rt/tst-cpuclock2' which did not fail with my patch applied.  The compiler was
gcc 4.8, if that makes a difference.  (Perhaps it did, since it's a new gcc
release and maybe people haven't been testing it much yet...)  The architecture
was x86_64 and the kernel was Linux 3.8.6.

If there are tests that are expected to fail in some situations, is there any
way that the test suite itself could warn of that?

I was also surprised that there seemed to be no place where a clear summary of
the test suite results gets logged, other than by redirecting the standard error
of 'make check -k' and grepping for the word "Error" to get past all the
compiler warnings that also get sent to stderr.  Is there a better way to run
the tests?

> 
> There should be two spaces after the end of the sentence.
> 

The fixed patch follows, although note there are still other comments in the
same source code file that do not follow this style guideline.

diff --git a/ChangeLog b/ChangeLog
index 90d6d47..e978ee2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2013-04-20  Eric Biggers  <ebiggers3@gmail.com>
+
+	[BZ #15362]
+	* libio/fileops.c:  Revert problematic fixes for [BZ #11741]
+	* libio/iofwrite.c:  Likewise.
+	* libio/iofwrite_u.c:  Likewise.
+	* libio/iopadn.c:  Likewise.
+	* libio/iowpadn.c:  Likewise.
+	* stdio-common/vfprintf.c:  Fix [BZ #11741] properly by checking whether
+	_IO_padn() returned the full count written.
+
 2013-04-16  Roland McGrath  <roland@hack.frob.com>
 
 	* rt/tst-aio7.c (do_test): Don't test O_RDONLY fd case, which is now
diff --git a/libio/fileops.c b/libio/fileops.c
index 61b61b3..5f3dae7 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -1245,13 +1245,12 @@ _IO_new_file_write (f, data, n)
      _IO_ssize_t n;
 {
   _IO_ssize_t to_do = n;
-  _IO_ssize_t count = 0;
   while (to_do > 0)
     {
-      count = (__builtin_expect (f->_flags2
-				 & _IO_FLAGS2_NOTCANCEL, 0)
-	       ? write_not_cancel (f->_fileno, data, to_do)
-	       : write (f->_fileno, data, to_do));
+      _IO_ssize_t count = (__builtin_expect (f->_flags2
+					     & _IO_FLAGS2_NOTCANCEL, 0)
+			   ? write_not_cancel (f->_fileno, data, to_do)
+			   : write (f->_fileno, data, to_do));
       if (count < 0)
 	{
 	  f->_flags |= _IO_ERR_SEEN;
@@ -1263,7 +1262,7 @@ _IO_new_file_write (f, data, n)
   n -= to_do;
   if (f->_offset >= 0)
     f->_offset += n;
-  return count < 0 ? count : n;
+  return n;
 }
 
 _IO_size_t
@@ -1323,13 +1322,11 @@ _IO_new_file_xsputn (f, data, n)
       _IO_size_t block_size, do_write;
       /* Next flush the (full) buffer. */
       if (_IO_OVERFLOW (f, EOF) == EOF)
-	/* If nothing else has to be written or nothing has been written, we
-	   must not signal the caller that the call was even partially
-	   successful.  */
-	return (to_do == 0 || to_do == n) ? EOF : n - to_do;
+	/* If nothing else has to be written we must not signal the
+	   caller that everything has been written.  */
+	return to_do == 0 ? EOF : n - to_do;
 
-      /* Try to maintain alignment: write a whole number of blocks.
-	 dont_write is what gets left over. */
+      /* Try to maintain alignment: write a whole number of blocks.  */
       block_size = f->_IO_buf_end - f->_IO_buf_base;
       do_write = to_do - (block_size >= 128 ? to_do % block_size : 0);
 
diff --git a/libio/iofwrite.c b/libio/iofwrite.c
index 81596a6..66542ea 100644
--- a/libio/iofwrite.c
+++ b/libio/iofwrite.c
@@ -42,12 +42,12 @@ _IO_fwrite (buf, size, count, fp)
   if (_IO_vtable_offset (fp) != 0 || _IO_fwide (fp, -1) == -1)
     written = _IO_sputn (fp, (const char *) buf, request);
   _IO_release_lock (fp);
-  /* We are guaranteed to have written all of the input, none of it, or
-     some of it.  */
-  if (written == request)
+  /* We have written all of the input in case the return value indicates
+     this or EOF is returned.  The latter is a special case where we
+     simply did not manage to flush the buffer.  But the data is in the
+     buffer and therefore written as far as fwrite is concerned.  */
+  if (written == request || written == EOF)
     return count;
-  else if (written == EOF)
-    return 0;
   else
     return written / size;
 }
diff --git a/libio/iofwrite_u.c b/libio/iofwrite_u.c
index 4a9d6ca..18dc6d0 100644
--- a/libio/iofwrite_u.c
+++ b/libio/iofwrite_u.c
@@ -44,12 +44,12 @@ fwrite_unlocked (buf, size, count, fp)
   if (_IO_fwide (fp, -1) == -1)
     {
       written = _IO_sputn (fp, (const char *) buf, request);
-      /* We are guaranteed to have written all of the input, none of it, or
-	 some of it.  */
-      if (written == request)
+      /* We have written all of the input in case the return value indicates
+	 this or EOF is returned.  The latter is a special case where we
+	 simply did not manage to flush the buffer.  But the data is in the
+	 buffer and therefore written as far as fwrite is concerned.  */
+      if (written == request || written == EOF)
 	return count;
-      else if (written == EOF)
-	return 0;
     }
 
   return written / size;
diff --git a/libio/iopadn.c b/libio/iopadn.c
index cc93c0f..5ebbcf4 100644
--- a/libio/iopadn.c
+++ b/libio/iopadn.c
@@ -59,7 +59,7 @@ _IO_padn (fp, pad, count)
       w = _IO_sputn (fp, padptr, PADSIZE);
       written += w;
       if (w != PADSIZE)
-	return w == EOF ? w : written;
+	return written;
     }
 
   if (i > 0)
diff --git a/libio/iowpadn.c b/libio/iowpadn.c
index d94db71..5600f37 100644
--- a/libio/iowpadn.c
+++ b/libio/iowpadn.c
@@ -65,7 +65,7 @@ _IO_wpadn (fp, pad, count)
       w = _IO_sputn (fp, (char *) padptr, PADSIZE);
       written += w;
       if (w != PADSIZE)
-	return w == EOF ? w : written;
+	return written;
     }
 
   if (i > 0)
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index c8bcf5a..61d9dc2 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
@@ -90,13 +90,13 @@
   do {									      \
     if (width > 0)							      \
       {									      \
-	unsigned int d = _IO_padn (s, (Padchar), width);		      \
-	if (__glibc_unlikely (d == EOF))				      \
+	_IO_ssize_t written = _IO_padn (s, (Padchar), width);		      \
+	if (__glibc_unlikely (written != width))			      \
 	  {								      \
 	    done = -1;							      \
 	    goto all_done;						      \
 	  }								      \
-	done_add (d);							      \
+	done_add (written);						      \
       }									      \
   } while (0)
 # define PUTC(C, F)	_IO_putc_unlocked (C, F)
@@ -119,13 +119,13 @@
   do {									      \
     if (width > 0)							      \
       {									      \
-	unsigned int d = _IO_wpadn (s, (Padchar), width);		      \
-	if (__glibc_unlikely (d == EOF))				      \
+	_IO_ssize_t written = _IO_wpadn (s, (Padchar), width);		      \
+	if (__glibc_unlikely (written != width))			      \
 	  {								      \
 	    done = -1;							      \
 	    goto all_done;						      \
 	  }								      \
-	done_add (d);							      \
+	done_add (written);						      \
       }									      \
   } while (0)
 # define PUTC(C, F)	_IO_putwc_unlocked (C, F)