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

[Eric Biggers <ebiggers3 at gmail dot com>]

On Sun, Apr 14, 2013 at 12:54:58PM +0530, Siddhesh Poyarekar wrote:
> It is not necessary the the stream is line buffered.  It could also be
> that the buffer was already full (or the stream is unbuffered) and had
> to be flushed before writing anything at all into it.

Before the "fix" to 11741, the code was:

  if (to_do + must_flush > 0)
    {
      _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 we must not signal the
           caller that everything has been written.  */
        return to_do == 0 ? EOF : n - to_do;

to_do == 0 in that code block implies must_flush > 0.  And must_flush is only
set greater than zero in conditional code executed if the FILE is line-buffered,
the stream was already in put mode, and the data fully fits in the buffer.  So
the EOF return only happened in that case.  Certainly, the _IO_OVERFLOW() could
fail in cases where to_do > 0 and must_flush == 0, but then the return value
will be (n - to_do).  If (to_do == n), then nothing has been written, not even
to the buffer, and the return value (before your fix to 11741) was 0, which I
believe is correct, as 0 bytes were successfully written.  Your patch to fix
11741 had changed the code to return EOF in this case, but it wasn't clear to me
why this was done.

> It is a bug.  It should return the full count for fwrite in this case.

I'm not really convinced that returning a full count in this case is the correct
behavior for line-buffered FILEs.  If I use fwrite() to write data containing a
newline to a line-buffered FILE, and fwrite() returns a count that indicates it
has successfully written data up to and including a given newline character,
then I would expect that, because the FILE is line-buffered, the corresponding
data up to this newline character will have been flushed with sys_write().  Is
there any prior discussion on this that could be referred to?  The only thing I
can think of is that since fwrite() is often used for binary data and not text,
the C library need not guarantee that line-buffering semantics are respected,
although as a programmer I wouldn't really expect this behavior.  Also, the C99
standard implies fwrite() is supposed to behavior consistent with calling
fputc() for each byte.  And fputc('\n', fp) is supposed to flush a line-buffered
stream, is it not?

> This should be handled correctly with the fixed version of
> _IO_new_file_xsputn (i.e. the version due to 1174).  The first attempt
> at flushing occurs in _IO_OVERFLOW, where the current buffer contents
> - which in case of the above scenario will be contents that fit in the
> remaining buffer and not just up to newline, if any - are flushed.  In
> the above case, _IO_OVERFLOW will fail, resulting in an EOF return.
> If you're considering that this overflow code succeeds and the
> subsequent new_do_write fails and does not return a failure correctly,
> then  yes, that will happen.  That code is broken as a result of the
> 1174 change.  In fact, IIRC now, _IO_OVERFLOW was the reason why I had
> not considered even partial writes as success.

No, in the version following your patch to 11741, EOF is only returned if to_do
== 0 or to_do == n.  If some, but not all, of the input data fits into the FILE
buffer, then 'count' is set to the remaining space and 'to_do' will be
decremented by 'count', leaving 'to_do' at neither 0 nor n, so upon overflow
failure the return value will be the number of bytes buffered.  My
interpretation is that this violates line-buffering semantics if the buffered
data contains newlines, as the code returns that the data was successfully
written, but the line(s) were not actually flushed with sys_write().  Here is a
test program that returns 0 if and only if the C library handles this case the
way I think it should:

  #include <stdio.h>
  #include <unistd.h>
  #include <string.h>

  static char stdout_buf[1024];
  
  int main()
  {
  	size_t i;
  	size_t bytes_written;
  
  	setvbuf(stdout, stdout_buf, _IOLBF, sizeof(stdout_buf));
  	fputc('\n', stdout);
  	close(STDOUT_FILENO);
	/* Write a block of data with embedded newlines that is too large to fit
	 * in the FILE's internal buffer.  In this particular example we place a
	 * newline every 8 bytes.  Since the FILE is line-buffered and the
	 * underlying file descriptor has been closed, it should be impossible
	 * to successfully fwrite() any more than 7 bytes. */
  	char data[sizeof(stdout_buf) * 2];
  	for (i = 0; i < sizeof(data) / 8; i ++)
  		memcpy(&data[i * 8], "Hello!!\n", 8);
  	bytes_written = fwrite(data, 1, sizeof(data), stdout);
  	fprintf(stderr, "Wrote %zu bytes (expected less than 8)\n",
  		bytes_written);
  	return bytes_written < 8 ? 0 : -1;
  }

You may have brought up another relevant point, which is that _IO_OVERFLOW()
only returns 0 or EOF, never a partial write, even though it may have done a
partial write.  Let's consider what this means.  For unbuffered FILEs this is
irrelevant because there is never any buffered data to flush.  For fully
buffered FILEs this is also mostly irrelevant because we consider all buffered
data to have been "written"; we only need to know whether the flush succeeded or
not so we can correctly return a short count if there is more to write.  It may,
however, be desirable for the FILE to adjust its internal state so that exactly
the unwritten bytes are buffered, rather than the current behavior in
new_do_write() of resetting the buffer even if it has not all been successfully
flushed.  This would mean that the following program would print the buffer of
'A's when the final fputc() executes, rather than the current behavior of
discarding them on the fputc() that errors.  The current behavior in this case
may be very much intentional, but I'm just making sure.

  #include <unistd.h>
  #include <stdio.h>
  #include <string.h>
  #include <assert.h>
  
  static char stdout_buf[1024];
  
  int main()
  {
  	setvbuf(stdout, stdout_buf, _IOFBF, sizeof(stdout_buf));
  
  	char buf[1023];
  	memset(buf, 'A', sizeof(buf));
  
  	assert(fwrite(buf, 1, sizeof(buf), stdout) == 1023);
  
  	int stdout_save = dup(1);
  	close(1);
  
  	assert(fputc('a', stdout) == 'a');
  	assert(fputc('a', stdout) == EOF);
  
  	dup2(stdout_save, 1);
  	assert(fputc('a', stdout) == 'a');
  	eturn 0;
  }

Anyway, for line buffered FILEs, I believe we do need the partial write count
from _IO_OVERFLOW().  For example, someone could write multiple lines of data
in one call to fputs() or fwrite().  If flushing the stream results in a partial
write but the first line was fully written with sys_write(), then I believe the
expected return value would be the total number of bytes successfully written
with sys_write(), minus any preceding bytes in the first line that were already
buffered before the fputs() or fwrite() call, plus any additional bytes that are
still buffered and are part of the next line to be written with sys_write().  No
version of the code we are considering appears to have this behavior, as it is
not even supported by the semantics of _IO_OVERFLOW().

> 
> Here's what we could do (in different patches):
> 
> - Revert fixes for 1174, thus fixing 15362 and reopening 1174.
> - Make _overflow and PAD aware of short writes, while ensuring that
> fwrite does not suffer as a result
> 
> I'll do the revert.  Let me know if you want to work on second bit;
> I'd be happy to review.  I don't know if you have an FSF copyright
> assignment in place for glibc, but I think you'll need that for your
> patches to be accepted.  It might be worthwhile to go through the
> contribution checklist on the wiki:
> 
> http://sourceware.org/glibc/wiki/Contribution%20checklist

I do not have a FSF copyright assignment in place yet, but I will do that if
needed.  I think that after the revert, bug 11741 would be fixed just by
changing PAD() in the way I suggested.  The other issues I have brought up, such
as line buffering semantics not always being respected in error paths could be
considered separate bugs.  But before I can change any other code I think we
need to agree on what the behavior should be in the cases I've brought up, and
also whether the corresponding code should be changed in the equivalent
functions in wfileops.c and oldfileops.c.