Re: [PATCH] tempfile: avoid "ferror | fclose" trick

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 17, 2017, 08:00 UTC
Message-ID: <923e328c-7fea-a9e4-1059-3bd6b8e58164@alum.mit.edu>
In-Reply-To: <20170216213140.xqw7gzjimhvg7tcm@sigill.intra.peff.net>

On 02/16/2017 10:31 PM, Jeff King wrote:

Show 69 quoted lines

> On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:
> 
>> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
>>
>>>>> 	int xfclose(FILE *fp)
>>>>> 	{
>>>>> 		return ferror(fp) | fclose(fp);
>>>>> 	}
>>>>
>>>> Yes, that's exactly what I had in mind (might be worth calling out the
>>>> bitwise-OR, though, just to make it clear it's not a typo).
>>>
>>> Since the order of evaluation is unspecified, it would be better to
>>> force sequencing ferror before fclose.
>>
>> Good point. Arguably the call in tempfile.c is buggy.
> 
> Here's a fix.
> 
> I think close_tempfile() suffers from the same errno problem discussed
> earlier in this thread (i.e., that after calling it, you may get an
> error return with a random, unrelated errno value if ferror() failed but
> fclose() did not).
> 
> -- >8 --
> Subject: [PATCH] tempfile: avoid "ferror | fclose" trick
> 
> The current code wants to record an error condition from
> either ferror() or fclose(), but makes sure that we always
> call both functions. So it can't use logical-OR "||", which
> would short-circuit when ferror() is true. Instead, it uses
> bitwise-OR "|" to evaluate both functions and set one or
> more bits in the "err" flag if they reported a failure.
> 
> Unlike logical-OR, though, bitwise-OR does not introduce a
> sequence point, and the order of evaluation for its operands
> is unspecified. So a compiler would be free to generate code
> which calls fclose() first, and then ferror() on the
> now-freed filehandle.
> 
> There's no indication that this has happened in practice,
> but let's write it out in a way that follows the standard.
> 
> Noticed-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  tempfile.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tempfile.c b/tempfile.c
> index 2990c9242..ffcc27237 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)
>  	tempfile->fd = -1;
>  	if (fp) {
>  		tempfile->fp = NULL;
> -
> -		/*
> -		 * Note: no short-circuiting here; we want to fclose()
> -		 * in any case!
> -		 */
> -		err = ferror(fp) | fclose(fp);
> +		err = ferror(fp);
> +		err |= fclose(fp);
>  	} else {
>  		err = close(fd);
>  	}
>

Thanks for fixing this; the old code was definitely wrong.

As you pointed out, if ferror() fails, it doesn't set errno properly. At least one caller tries to strerror(errno), so it would probably be good to store *something* in there, probably EIO.

To be really pedantic, there's also the question of what errno the caller would want if *both* ferror() and fclose() fail. Normally I would say "the first error that occurred", but in this case we don't know the correct errno from the error reported by ferror(), so maybe the fclose() errno is more likely to hint at the underlying reason for the failure.

So I (reluctantly) propose

	if (ferror(fp)) {
		if (!fclose(fp)) {
			/*
			 * ferror() doesn't set errno, so we have to
			 * set one. (By contrast, when fclose() fails
			 * too, we leave *its* errno in place.)
			 */
			errno = EIO;
		}
		return -1;
	}
	return fclose();

Michael