From owner-svn-src-all@FreeBSD.ORG Mon May 27 08:33:38 2013 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id B73E4283; Mon, 27 May 2013 08:33:38 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail35.syd.optusnet.com.au (mail35.syd.optusnet.com.au [211.29.133.51]) by mx1.freebsd.org (Postfix) with ESMTP id C1B9ED54; Mon, 27 May 2013 08:33:37 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail35.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r4R8XILn001101 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 27 May 2013 18:33:22 +1000 Date: Mon, 27 May 2013 18:33:18 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Stefan Esser Subject: Re: svn commit: r250972 - head/usr.bin/patch In-Reply-To: <51A26FED.4020801@freebsd.org> Message-ID: <20130527172803.P1491@besplex.bde.org> References: <201305241854.r4OIsqdU043683@svn.freebsd.org> <20130525122811.I837@besplex.bde.org> <51A26FED.4020801@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=e/de0tV/ c=1 sm=1 a=0a0yn1FGu_gA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=LFobCLJwE3wA:10 a=x0QUDIz939gMxI9YZRcA:9 a=CjuIK1q_8ugA:10 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Stefan Esser , Bruce Evans X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 May 2013 08:33:38 -0000 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 > > 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