This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


I'm not going to consider the effects of the current patch I proposed
since I agree that it is broken for now.

On 13 April 2013 22:22, Eric Biggers <ebiggers3@gmail.com> wrote:
> 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.

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.

> 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...).

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

> 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.

I had actually fixed this bit in the first iteration of the patch
IIRC, but it was dropped after review for some reason.

On 14 April 2013 04:23, Eric Biggers <ebiggers3@gmail.com> wrote:
> Upon further examination, testing 'must_flush' is exactly equivalent to testing
> 'to_do == 0', because the block of code is only entered if 'to_do + must_flush >
> 0'.  Furthermore, 'must_flush' is only set if all the data to be written fits
> into the internal buffer.  So I believe that neither the original
> _IO_new_file_xsputn() nor either version of the patched _IO_new_file_xsputn()
> correctly handles the following sequence of events:
>
>   1. Data is written to a line-buffered FILE using fputs(), puts(), fwrite(), or
>      possibly other functions that internally call _IO_new_file_xsputn().
>
>   2. The amount of data to be written is greater than the space remaining in the
>      FILE's internal buffer, but there are one or more newlines in the part of
>      the data that does fit in the internal buffer.
>
>   3. The internal buffer is filled, but flushing it fails.
>
> The current behavior in this situation is to return the number of bytes
> buffered, which I believe would violate the meaning of "line-buffered" because
> the code returns that it has successfully consumed data containing a newline
> without having successfully flushed it out with the underlying write() system
> call.  Or am I misunderstanding how line buffering is supposed to work...?  This
> perhaps should be a separate bug, but this has to do with the very same line of
> code.

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.

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

Siddhesh
--
http://siddhesh.in


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]