Re: [PATCH v1.1][BZ #15362] Correctly return bytes written out in _IO_new_file_write

[Eric Biggers <ebiggers3 at gmail dot com>]

Hi,

I believe there are problems with the original code that this patch, and the
original commit you made (2b766585f9b4) to fix bug #11741 do not address very
well.  Before commit 2b766585f9b4, vfprintf() wrote padding as follows:

# define PAD(Padchar) \
  if (width > 0)							      \
    done_add (_IO_padn (s, (Padchar), width))

I see no check for whether the full 'width' of padding had actually been
written.  Instead, the byte counter was incremented by the number of bytes
written, which was not necessarily the number requested.  This was the cause of
bug #11741.  The fix for this that would have made sense to me would have been
to replace the PAD() macro with something like this:

# define PAD(Padchar) \
  do									      \
    {									      \
      _IO_size_t written = _IO_padn (s, (Padchar), width);		      \
      if (__glibc_unlikely (written != width))				      \
	{								      \
	  done = -1;							      \
	  goto all_done;						      \
	}								      \
      done_add (written);						      \
    }									      \
  while (0)

which just makes sure that all 'width' bytes were written.  Your solution is
similar but not quite the same:

# define PAD(Padchar) \
  do {									      \
    if (width > 0)							      \
      {									      \
	unsigned int d = _IO_padn (s, (Padchar), width);		      \
	if (__glibc_unlikely (d == EOF))				      \
	  {								      \
	    done = -1;							      \
	    goto all_done;						      \
	  }								      \
	done_add (d);							      \
      }									      \
  } while (0)

which only checks for EOF, not short count.  This is apparently because you
changed the behavior of _IO_padn() to return EOF in some cases, but as far as I
can see, a short count return is still possible and would still indicate a write
error.

But in fact, I think that the new behavior of returning EOF from _IO_padn() is
probably unnecessary, given that a short count already indicates an error.  I
believe that some of the confusion arises from the fact that, for quite some
time now (since commit 34c5e4a1 dating back to 2005), _IO_new_file_xsputn() has
returned EOF in a certain situation--- more specifically when all of the
following conditions occurred in a call to _IO_new_file_xsputn():

  1. The FILE was line-buffered.
  2. The data buffer passed to _IO_new_file_xsputn() contained one or more
     newline characters.
  3. The entire data buffer passed to _IO_new_file_xsputn() was successfully
     buffered in the FILE's internal buffer.
  4. Flushing the FILE's internal buffer up to and including the final newline
     character failed.
   
This return value was never documented in the comment above _IO_xsputn_t in
libio/libioP.h, which stated that the C++ semantics for streambuf::xsputn()
applied to implementations of _IO_xsputn_t, nor did the comment above the
relevant return statement clearly explain its purpose.  As far as I can tell,
this special return value was used to make this specific case cause a failure in
the *printf() family of functions and fputs(), but still succeed when called
from fwrite().  I question whether this really was the desired behavior, as my
understanding is that a program could use fwrite() on a line-buffered output
stream, in which case an error (short return) would be expected if the output
data contained newline(s) but was not successfully flushed with sys_write() up
until the final newline.

Incidentally, commit 2b766585f9b4 changed the old fwrite() behavior, as it now
contains:

  /* We are guaranteed to have written all of the input, none of it, or
     some of it.  */
  if (written == request)
    return count;
  else if (written == EOF)
    return 0;
  else
    return written / size;

meaning that it returns 0 rather than the full count in the special
line-buffered case described previously.  If this was intentional then I think
this should be clearly documented by the comment, which currently contains no
information whatsoever (obviously "all", "none", or "some" of the input was
written, what other options are there...).

Another problem: _IO_padn() in libio/iopadn.c contains the following code:

  if (i > 0)
    {
      w = _IO_sputn (fp, padptr, i);
      written += w;
    }
  return written;

That's been there for a very long time and was never updated to take into
account that _IO_sputn() may return EOF.  So it will blindly subtract 1 from
'written' rather than return the correct count.  Of course, if _IO_sputn() only
returned the number of bytes written, this would not be a problem.

Anyway, I think the simplest and most correct way to solve all these problems is
to revert to the pre-2b766585f9b4 code in the relevant locations, then fix both
occurrences of PAD() in the way I suggested above (checking for short count),
then change _IO_new_file_xsputn() to return 0 instead of EOF in the
line-buffered failure case detailed above (and clearly document this case to
avoid any confusion).

For reference, a *preliminary* (not tested!!!) patch to do this follows.  Please
note: the patch makes the corresponding change to _IO_old_file_xsputn(), which I
wasn't sure whether to do or not since I assume the "old" operations are not to
be changed for backwards compatibility reasons.  However, the same fwrite()
seems to be used with both the old and new operations, and it had its behavior
changed by commit de2fd463b1c031...  Another note: I changed the test condition
in _IO_new_file_xsputn() to 'must_flush' instead of 'to_do == 0', which
effectively makes the 0 return value apply without condition (3) in the 4
conditions I listed above (i.e. 0 will now be returned if line-buffered data was
partially buffered up to some newline character but could not be flushed).  I
think this is the correct behavior.  I would, however, have to ask (and this
applies to the code even without my patch) whether the buffered data that could
not be flushed should be removed from the buffer or not before returning that it
could not be written to the FILE.  To me, it makes sense that the data not be
saved in the FILE at all if we're returning that it cannot be written, but the
original code did not seem to have this behavior in the line-buffered failure
case described previously.

diff --git a/libio/fileops.c b/libio/fileops.c
index 61b61b3..a64c05e 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,16 @@ _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;
+	{
+	    /* Flushing the buffer failed.  If must_flush is set,
+	       then we failed to flush line-buffered output and
+	       should return 0 bytes written.  Otherwise, we should
+	       return the number of bytes that were buffered
+	       successfully. */
+	    return must_flush ? 0 : 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..ada9ece 100644
--- a/libio/iofwrite.c
+++ b/libio/iofwrite.c
@@ -42,12 +42,13 @@ _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.  */
+
+  /* In the common case, all the data elements have been written
+     and we can simply re-use 'count' instead of performing a
+     division to calculate the actual number of data elements
+     written. */
   if (written == request)
     return count;
-  else if (written == EOF)
-    return 0;
   else
     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/libio/libioP.h b/libio/libioP.h
index 911f649..e859a90 100644
--- a/libio/libioP.h
+++ b/libio/libioP.h
@@ -166,7 +166,7 @@ typedef int (*_IO_pbackfail_t) (_IO_FILE *, int);
 #define _IO_WPBACKFAIL(FP, CH) WJUMP1 (__pbackfail, FP, CH)
 
 /* The 'xsputn' hook writes upto N characters from buffer DATA.
-   Returns EOF or the number of character actually written.
+   Returns the number of characters actually written.
    It matches the streambuf::xsputn virtual function. */
 typedef _IO_size_t (*_IO_xsputn_t) (_IO_FILE *FP, const void *DATA,
 				    _IO_size_t N);
diff --git a/libio/oldfileops.c b/libio/oldfileops.c
index 80e4b57..8f8aea2 100644
--- a/libio/oldfileops.c
+++ b/libio/oldfileops.c
@@ -751,7 +751,7 @@ _IO_old_file_xsputn (f, data, n)
       _IO_size_t block_size, do_write;
       /* Next flush the (full) buffer. */
       if (__overflow (f, EOF) == EOF)
-	return to_do == 0 ? EOF : n - to_do;
+	return must_flush ? 0 : n - to_do;
 
       /* Try to maintain alignment: write a whole number of blocks.
 	 dont_write is what gets left over. */
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index c8bcf5a..0e3a89e 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_size_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_size_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)