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

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

According to C99 6.5:

        [#3]  The grouping of operators and operands is indicated by
        the syntax.61)  Except as specified later (for the function-
        call  (),  &&,  ||,  ?:,  and comma operators), the order of
        evaluation of subexpressions and the  order  in  which  side
        effects take place are both unspecified.

The && and || operators are often abused slightly, as here, to get
a particular order of evaluation in a largish subexpression.

> @@ -385,7 +385,7 @@ main(int argc, char *argv[])
> 				}
> 			}
> 		}
> -		if (ferror(rejfp) || fclose(rejfp)) {
> +		if (ferror(rejfp) | fclose(rejfp)) {
> 			say("Error writing %s\n", rejname);
> 			error = 1;
> 		}

Similarly.

> @@ -977,7 +977,7 @@ spew_output(void)
> #endif
> 	if (input_lines)
> 		copy_till(input_lines, true);	/* dump remainder of file */
> -	rv = ferror(ofp) == 0 && fclose(ofp) == 0;
> +	rv = ferror(ofp) == 0 & fclose(ofp) == 0;
> 	ofp = NULL;
> 	return rv;
> }

SImilarly.

> ...

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

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;

This is too ugly.  I prefer to depend on fclose() being non-broken.
Unfortunately, fclose() seems to be broken in FreeBSD:

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

Bruce