Fix fwrite() reading beyond end of buffer in error path

Partially revert commits 2b766585f9b4ffabeef2f36200c275976b93f2c7 and
de2fd463b1c0310d75084b6d774fb974075a4ad9, which were intended to fix BZ#11741
but caused another, likely worse bug, namely that fwrite() and fputs() could,
in an error path, read data beyond the end of the specified buffer, and
potentially even write this data to the file.

Fix BZ#11741 properly by checking the return value from _IO_padn() in
stdio-common/vfprintf.c.

diff --git a/ChangeLog b/ChangeLog
index 5d99bd810b477a7a94da32059ac7c1fc41ba39d4..c4a5d8490a04586ecad08e4e995da8c684db79b6 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2013-10-11  Eric Biggers  <ebiggers3@gmail.com>
+
+       [BZ #15362]
+       * libio/fileops.c (_IO_new_file_write): Return count of bytes
+       written.
+       (_IO_new_file_xsputn): Don't return EOF if nothing has been
+       written.
+       * libio/iofwrite.c (_IO_fwrite): Return count if bytes were
+       written to buffer but not flushed.
+       * libio/iofwrite_u.c:  Likewise.
+       * libio/iopadn.c:  Return bytes returned even if EOF was
+       encountered.
+       * libio/iowpadn.c:  Likewise.
+       * stdio-common/vfprintf.c [COMPILE_WPRINTF] (PAD): Return error
+       if _IO_padn does not write the whole buffer.
+       [!COMPILE_WPRINTF] (PAD): Likewise.
+
 2013-10-10  David S. Miller  <davem@davemloft.net>
 
        * sysdeps/posix/dirstream.h (struct __dirstream): Fix alignment of

diff --git a/NEWS b/NEWS
index 5f0a710503b06c893275301b85d3c82dab2565ff..48a92e83650bff375c15e85887b8b7dcf01db704 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -9,12 +9,12 @@ Version 2.19
 
 * The following bugs are resolved with this release:
 
-  156, 431, 13982, 13985, 14155, 14547, 14699, 15048, 15400, 15427, 15522,
-  15531, 15532, 15608, 15609, 15610, 15632, 15640, 15680, 15681, 15723,
-  15734, 15735, 15736, 15748, 15749, 15754, 15760, 15797, 15844, 15849,
-  15855, 15856, 15857, 15859, 15867, 15886, 15887, 15890, 15892, 15893,
-  15895, 15897, 15905, 15909, 15919, 15921, 15923, 15939, 15963, 15966,
-  15988, 16034.
+  156, 431, 13982, 13985, 14155, 14547, 14699, 15048, 15362, 15400, 15427,
+  15522, 15531, 15532, 15608, 15609, 15610, 15632, 15640, 15680, 15681,
+  15723, 15734, 15735, 15736, 15748, 15749, 15754, 15760, 15797, 15844,
+  15849, 15855, 15856, 15857, 15859, 15867, 15886, 15887, 15890, 15892,
+  15893, 15895, 15897, 15905, 15909, 15919, 15921, 15923, 15939, 15963,
+  15966, 15988, 16034.
 
 * CVE-2012-4412 The strcoll implementation caches indices and rules for
   large collation sequences to optimize multiple passes.  This cache

diff --git a/libio/fileops.c b/libio/fileops.c
index e92f85b243496c07d3677b97c785da7f42fb6c38..c58e860c049e72a192d0f8710ede3043b27fbd2f 100644 (file)
--- 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 81596a64c44ae7caaf0e6051f74557a42f63c13c..66542eaea502b96e4bb5a66625a36c9b3eab904b 100644 (file)
--- 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 4a9d6caa0491b6ca9c3f3823bce6223f0cf9de72..18dc6d032d39e8af59d7563533bb5f0da8b9ff1f 100644 (file)
--- 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 cc93c0f7ac2aa6511261fc7c50a495cf1f2dad00..5ebbcf45511e6af0f8a1827196e017db267152e9 100644 (file)
--- 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 d94db71f2bc7ed123a69595e97c1a973e0ba9a78..5600f3711c84037a9694433870950b175ac1d5b9 100644 (file)
--- 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 fb22f6969e0ea9156c21d648e512309831916ca6..8cd7a85b216ab941b1fd459d88af36832562841f 100644 (file)
--- 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)