svn commit: r250972 - head/usr.bin/patch

[Stefan Esser]

Am 25.05.2013 06:31, schrieb Bruce Evans:
> On Fri, 24 May 2013, Stefan Esser wrote:
> 
>> ...
>> Log:
>>  The error handling for writes to the target file could lead to the final
>>  fclose() being skipped. Fix this by using boolean "&" and "|" instead of
>>  short-cut operators "&&" and "||".
>>  While here, increment the last part of the version string. The reason is
>>  the fixed output file selection logic in pch.c, which was committed as
>>  r250943, yesterday.
>>
>>  Reviewed by:    pfg
>> ...
>> Modified: head/usr.bin/patch/patch.c
>> ==============================================================================
>>
>> --- head/usr.bin/patch/patch.c    Fri May 24 18:28:27 2013    (r250971)
>> +++ head/usr.bin/patch/patch.c    Fri May 24 18:54:52 2013    (r250972)
>> @@ -303,7 +303,7 @@ main(int argc, char *argv[])
>>                     ++fuzz <= mymaxfuzz);
>>
>>                 if (skip_rest_of_patch) {    /* just got decided */
>> -                    if (ferror(ofp) || fclose(ofp)) {
>> +                    if (ferror(ofp) | fclose(ofp)) {
>>                         say("Error writing %s\n",
>>                             TMPOUTNAME);
>>                         error = 1;
> 
> This is more broken than before.  Now the order of operations is
> indeterminate, so the behaviour is undefined if fclose(ofp) is evaluated
> before ferror(ofp) (then ferror(ofp) is passed an invalid pointer).
> Now even the usual case when there is no error initially is broken.

Oh, I see ... Sorry, I should have noticed this myself.

I had considered two more complex solutions, one with a temporary
variable "result" which was set by the call to ferror and where the
result of fclose was ORed in, the other with duplication of the
error message and setting of error. I guess that using a temporary
variable for the ferror result is better ...

> Error handling is very rarely correct in utilities that use stdio.
> patch at least checks for errors.  The easiest way to improve it (not
> just for patch) is to sprinkle fflush()es.  Blindly doing voided
> fflush()es before every ferror() check ensures that output gets done
> and checked even if you neglect to do fclose() or check for errors in
> it (provided you have enough ferror() checks).  This is especially

Yes, I had considered an fflush with error checking before the
fclose, but thought this was unnecessary (one extra call), and
that it were better to use a temporary result variable, instead.
And then I erroneously removed that variable and used the boolean
operators, instead ... (It's good to simplify an algorithm, but
not further than required to keep it functional ;-) ).

> important and especially mostly not done for stdout and stderr, since
> it is best to never close these (it may be impossible to report errors
> on stdout or stderr on themself, especially for stderr, but it doesn't
> hurt to try, except this may inhibit more correct error handling like
> logging the error to another file).
> 
> patch/pch.c has another ferror() || fclose() that wasn't touched by this
> commit.  It is almost OK, since the file was just opened successfully
> so ferror() on it "can't happen".  But this means that the normal
> broken idiom 'ferror() || fclose()' is not even wrong.

I noticed that case, but did not bother to change it, since
pfatal() is called immediately after a failed ferror(). This
will terminate the program and thus there is no resource leak.

> I think ferror() in 'ferror() || fclose()' should never be necessary
> unless you actually want to keep the file open if ferror(), and then
> you should normally use clearerr() in the error handling.  C99 is
> almost clear that fclose() must fail if ferror() was true when it was
> called.  It says that fclose() returns EOF iff any errors were detected,
> and it would take a weaselish reading of this to not allow detection
> of errors that are already present when it was called.  patch might
> be using 'ferror() || fclose() because it doesn't trust pre-C90 stdio.

Our version of patch must only work on supported releases of
FreeBSD. The ferror() before flcose() does not exist in the
versions it is derived from. It was added after the code was
imported from OpenBSD.

> It is also a style bug to use the bitwise boolean operations for logical
> boolean operations.  When we need sequential evaluation, this becomes a
> bug. We want sequential evaluation, and that should be written normally
> using
> sequence points (which are easiest to get using separate statements) if
> the logical boolean operations aren't suitable.  So to combine the errors
> from ferror() and fclose() so as to support buggy fclose()'s, and to do
> this without sprinkling ferror(), we must write something like:
> 
>                     locerror = 0;
>                     if (ferror(ofp)
>                         locerror = 1;
>                     if (fclose(ofp)
>                         locerror = 1;
>                     if (locerror) {
>                         say("Error writing %s\n",
>                             TMPOUTNAME);
>                         error = 1;

I had code like that, before I thought I could simplify it. It
read
	int ret;
	...
	ret = ferror(ofp);
	if (fclose(ofp) || ret) {
		say ...
		error = 1
	}

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

> @ #include <stdio.h>
> @ @ int
> @ main(void)
> @ {
> @     FILE *fp;
> @ @     remove("test.dat");
> @     fp = fopen("test.dat", "a");
> @     fclose(fp);
> @     fp = fopen("test.dat", "r");
> @     (void)fwrite("", 1, 1, fp);
> @     fprintf(stderr, "ferror %d\n", ferror(fp));
> @     fprintf(stderr, "fclose %d\n", fclose(fp));
> @     return (0);
> @ }
> 
> This finds ferror(), but then fclose() succeeds.  Setting ferror() without
> having any real i/o problems isn't easy, and this example is contrived
> since the file is only open for input, so fclose() cannot have any buffered
> output to flush.  However, there points to a general problem with ferror() --
> the error flag is shared between input and output, so if fclose() always
> checked the flag and failed if it is set, then its failure wouldn't be
> useful for detecting output errors.  Neither would ferror().  So if the
> stream is open for both reading and writing, the application should do
> something like clearerr() before all fflush() and fclose() operations so
> as to detect only write errors.  fclose() in FreeBSD doesn't seem to check
> the flag at all.  I can't see exactly what it does.  The low level function
> __sflush() mainly tries to write all buffered i/o.  It sets ferror() and
> returns an error if any of the writes fails, but it doesn't return an error
> if ferror() is already true.

Do we have compatibility and/or regression tests for stdio that
cover ferror/fflush/fclose?

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.

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.

One way to test write errors is via tmpfs:

# mount -t tmpfs -o maxfilesize=1 tmp /mnt

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);
}
-----------------------------------------------------------------
1) normal write - expect OK:
fwrite 3
ferror 0
fflush 0
fclose 0

2) write 1 byte - expect OK:
fwrite 1
ferror 0
fflush 0
ferror 0
fclose 0

3) write 1 byte - expect OK:
fwrite 1
ferror 0
fflush 0
ferror 0
fclose 0

4) write 3 bytes - expect FAIL:
fwrite 3
fclose -1

5) write 3 bytes - expect FAIL:
fwrite 3
fflush -1
fclose 0

6) write 3 bytes - expect FAIL:
fwrite 3
ferror 0
fclose -1

7) write 3 bytes - expect FAIL:
fwrite 3
ferror 0
fflush -1
ferror 1
fclose 0

8) write 1 bytes to R/O file - expect FAIL:
fwrite 0
ferror 1
fflush -1
ferror 1
fclose 0
-----------------------------------------------------------------

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

Regards, STefan