Date: Mon, 27 May 2013 18:33:18 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Stefan Esser <se@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Stefan Esser <se@localhost.FreeBSD.org>, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r250972 - head/usr.bin/patch Message-ID: <20130527172803.P1491@besplex.bde.org> In-Reply-To: <51A26FED.4020801@freebsd.org> References: <201305241854.r4OIsqdU043683@svn.freebsd.org> <20130525122811.I837@besplex.bde.org> <51A26FED.4020801@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
n Sun, 26 May 2013, Stefan Esser wrote:
> Am 25.05.2013 06:31, schrieb Bruce Evans:
>> On Fri, 24 May 2013, Stefan Esser wrote:
> ...
> Another possibility (too obfuscated for my taste) is:
>
> if (ferror(ofp) {
> fclose(ofp);
> ofp = NULL;
> }
> if (ofp == NULL || fclose(ofp))
> say ...
> error = 1
> }
>
> This version does not need the temporary result variable,
> but uses two calls to fclose() and thus produces more code.
>
>> This is too ugly. I prefer to depend on fclose() being non-broken.
>> Unfortunately, fclose() seems to be broken in FreeBSD:
>
> Ughh. That might be the reason the ferror() tests were added
> in the FreeBSD version ...
The addition is fairly recent. I don't see any reason for it.
> ...
> Do we have compatibility and/or regression tests for stdio that
> cover ferror/fflush/fclose?
Not that I know of.
> I'm wondering, whether we could just implicitly call fflush before
> fclose and return an error condition, if the fflush failed. That
> way, fclose would report write errors only reported by fflush, now.
fflush() seems to be the most broken (see below), so using it increases
problems.
> The fclose(3) man-page states:
>
> ERRORS
> The fclose() function may also fail and set errno for any of the
> errors specified for the routines close(2) or fflush(3).
>
> So, returning fflush errors from fclose is documented behavior, but
> apparently not implemented behavior.
This is fuzzy wording. C99 says even less (nothing explicit about errno),
but POSIX is much more specific and says "shall fail" for most of the
errors specified for close() and fflush(). But at least in the 2001 draft
7, it also has bugs like specifying that st_ntime and st_ctime "[shall be
marked for update], if the stream was writable, and if buffered data
remains". This not only has bad grammar; it also specifies the update
if the write fails. This is inconsistent with the specification of
write(), that it only updates the times if it succeeds. fclose() has this
bug in a worse form. Similarly for fflush(), except the wording is better
and more clearly mis-specifies that the update always occurs iff any
buffered data remains.
So the above should say that fclose() does fail and set ferror() if
any of underlying functions (which are not necessarily write() and
close() fails. fflush() is only directly underlying, and of course
it must fail and set error if its underying functions fail.
I don't know of any implementations that get this wrong.
> One way to test write errors is via tmpfs:
>
> # mount -t tmpfs -o maxfilesize=1 tmp /mnt
A bit hard for me, since I don't believe in tmpfs and don't have it
on any of my systems.
>
> A few more tests with this special file system:
>
> -----------------------------------------------------------------
> #include <stdio.h>
>
> int
> main(void)
> {
> FILE *fp;
>
> remove("test.dat");
> fprintf(stderr, "1) normal write - expect OK:\n");
> fp = fopen("test.dat", "w");
> fprintf(stderr, "fwrite %zu\n", fwrite("a\n", 1, 3, fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fflush %d\n", fflush(fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
> //
> fprintf(stderr, "\n2) write 1 byte - expect OK:\n");
> remove("/mnt/test.dat");
> fp = fopen("/mnt/test.dat", "w");
> fprintf(stderr, "fwrite %zu\n", fwrite("b\n", 1, 1, fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fflush %d\n", fflush(fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
> //
> fprintf(stderr, "\n3) write 1 byte - expect OK:\n");
> remove("/mnt/test.dat");
> fp = fopen("/mnt/test.dat", "w+");
> fprintf(stderr, "fwrite %zu\n", fwrite("b\n", 1, 1, fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fflush %d\n", fflush(fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
> //
> fprintf(stderr, "\n4) write 3 bytes - expect FAIL:\n");
> remove("/mnt/test.dat");
> fp = fopen("/mnt/test.dat", "w");
> fprintf(stderr, "fwrite %zu\n", fwrite("c\n", 1, 3, fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
> //
> fprintf(stderr, "\n5) write 3 bytes - expect FAIL:\n");
> remove("/mnt/test.dat");
> fp = fopen("/mnt/test.dat", "w");
> fprintf(stderr, "fwrite %zu\n", fwrite("c\n", 1, 3, fp));
> fprintf(stderr, "fflush %d\n", fflush(fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
> //
> fprintf(stderr, "\n6) write 3 bytes - expect FAIL:\n");
> remove("/mnt/test.dat");
> fp = fopen("/mnt/test.dat", "w");
> fprintf(stderr, "fwrite %zu\n", fwrite("c\n", 1, 3, fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
> //
> fprintf(stderr, "\n7) write 3 bytes - expect FAIL:\n");
> remove("/mnt/test.dat");
> fp = fopen("/mnt/test.dat", "w");
> fprintf(stderr, "fwrite %zu\n", fwrite("c\n", 1, 3, fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fflush %d\n", fflush(fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
> //
> fprintf(stderr, "\n8) write 1 bytes to R/O file - expect FAIL:\n");
> //remove("/mnt/test.dat");
> fp = fopen("/mnt/test.dat", "r");
> fprintf(stderr, "fwrite %zu\n", fwrite("b\n", 1, 1, fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fflush %d\n", fflush(fp));
> fprintf(stderr, "ferror %d\n", ferror(fp));
> fprintf(stderr, "fclose %d\n", fclose(fp));
>
> return (0);
> }
> -----------------------------------------------------------------
> 5) write 3 bytes - expect FAIL:
> fwrite 3
> fflush -1
> fclose 0
fflush() seems to be very broken. It really does flush (discard) all
buffered data if there is a write error during the write(s) (and of
course it won't tell where the error occurred. Just another reason
why stdio should never be used. This misbehaviour seems to be
intentional: from libc/stdio/fflush.c:
% int
% __sflush(FILE *fp)
% {
% unsigned char *p;
% int n, t;
%
% t = fp->_flags;
% if ((t & __SWR) == 0)
% return (0);
%
% if ((p = fp->_bf._base) == NULL)
% return (0);
%
% n = fp->_p - p; /* write this much */
%
% /*
% * Set these immediately to avoid problems with longjmp and to allow
% * exchange buffering (via setvbuf) in user write function.
% */
% fp->_p = p;
It remembers how much to write, then advances the pointer and forgets where
it was. longjump will cause much larger problems now than when the above
was written, and a don't understand the point about exchange buffering.
This is normally (always from at least fflush()) called with FLOCKFILE(),
so if the lock is not null then longjmp would be especially fatal, but
maybe locking prevents problems with exchange buffering.
% fp->_w = t & (__SLBF|__SNBF) ? 0 : fp->_bf._size;
%
% for (; n > 0; n -= t, p += t) {
% t = _swrite(fp, (char *)p, n);
% if (t <= 0) {
n gives the amount not written, so we could easily reset the pointer, but
we don't. Locking should prevent any external use of the FILE object
while it is in an intermediate state.
% fp->_flags |= __SERR;
% return (EOF);
% }
% }
% return (0);
% }
Standards don't say anything to explicitly allow fflush() to flush (discard)
unwritable data. C99 says very little about anything in fflush(). POSIX
adds mainly
- a description of errors that shall be detected
- an example of fflush()ing stdout before using stdin (but nothing about
automatic fflush()ing of stdout that most unix stdios actually do)
- a rationale about the file position. The file position causes further
complications for failing fflush()es. Currently I think neither the
stdio position nor the kernel position is advanced for the failed part
of the write. This is as consistent as possible, but means that you
can't track the position by counting the bytes claimed to have been
written by fwrite() and putc(), etc.
> 6) write 3 bytes - expect FAIL:
> fwrite 3
> ferror 0
> fclose -1
Now when we don't try fflush() first, fclose() can actually see the
buffered data and fail trying to write it.
> So it seems that fclose will return an error if data could not be
> written (see case 4), but not if an fflush already returned the
> error condition. It is not sticky.
>
> So I guess it is sufficient to check the result returned by fclose,
> without the prior check of ferror. I do not see what cases ferror
> can catch, that are no caught by fclose ...
Now I see a problem that is large enough to require the the ferror()s:
__sflush() is called not only from fflush(), but for general operation.
Even putc() calls if fairly directly (via __swbuf() and __fflush()).
So when __sflush() fails in certain contexts, the only evidence of it
trashing the buffer is the sticky ferror() flag and the not-sticky
error return to functions like putc() and printf(). Most callers don't
check the results of their putc()'s and printf()'s. Hopefully printf()
itself checks, but patch(1) seems to use only fputs() and putc() to
write to ofp, and it _never_ checks their results. So in most cases
there is something in the buffer at fclose() time and fclose() detects
most errors in these cases (there might be previous undetected errors
but most error conditions are sticky at the fs level). But sometimes
the auto-flush might occur on the last byte of a file. Then trashing
the buffer would leave no problem for fclose() to detect.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130527172803.P1491>