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

[Junio C Hamano]

Jeff King  writes:

> I'm also not sure that it's all that useful to distinguish errors from
> fwrite() versus fclose(). In practice, errors will come from write() in
> either case, and the caller does not have much control over when the
> flushing happens. So any error saying "error closing file" is probably
> assuming too much anyway. It should be "error writing file".

Yes.

>> We _could_ clear errno to allow caller to tell them apart, though,
>> if we wanted to ;-)
>
> Hmm. So basically "errno = 0" instead of EIO? That at least has the
> advantage that you can tell it apart from a real EIO, and a caller
> _could_ if they chose do:
>
>   if (commit_lock_file()) {
>   if (!errno)
>   error("error writing to file");
>   else
>   error_errno("error closing file");
>   }

Exactly.

> But I am not sure I would want to annotate all such callers that way. It
> would probably be less bad to just pass down a "quiet" flag or a strbuf
> and have the low-level code fill it in. And that solves this problem
> _and_ the rename() thing above.
>
> But TBH, I am not sure if it is worth it. Nobody is complaining. This is
> just a thing we noticed. I think setting errno to EIO or to "0" is a
> strict improvement over what is there (where the errno is essentially
> random) and the complexity doesn't escape the function.
>
> So I think that "easy" thing falls far short of a solution, but it's at
> least easy. I could take it or leave it at this point.

Well, I already said that earlier in this thread, and ended up
queuing your patch on 'pu' ;-).

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

[Jeff King]

On Fri, Feb 17, 2017 at 03:52:36PM -0800, Junio C Hamano wrote:

> > So I think that "easy" thing falls far short of a solution, but it's at
> > least easy. I could take it or leave it at this point.
> 
> Well, I already said that earlier in this thread, and ended up
> queuing your patch on 'pu' ;-).

We're slowly converging on consensus in our apathy. ;)

-Peff

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

[Jeff King]

On Fri, Feb 17, 2017 at 02:40:19PM -0800, Junio C Hamano wrote:

> > Right. EIO is almost certainly _not_ the error we saw. But I would
> > rather consistently say "I/O error" and have the user scratch their
> > head, look up this thread, and say "ah, it was probably a deferred
> > error", as opposed to the alternative: the user sees something
> > nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> > may change from run to run based on what other code runs or fails in
> > between.
> 
> My point was actually not what errno we feed to strerror().  In that
> example, what is more misleading is the fixed part of the error
> message the caller of close_tempfile() used after seeing the funcion
> fail, i.e. "failed to close".  strerror() part is used to explain
> why we "failed to close", and of course any nonsensical errno that
> we did not get from the failed stdio call would not explain it, but
> a more misleading part is that we did not even "failed to close" it.
> 
> We just noticed an earlier error while attempting to close it.
> strerror() in the message does not even have to be related to the
> closing of the file handle.

Ah, I see.  I think the errno thing is a strict improvement over what is
there now. Actually having a separate error message is even better, but
it does end up rather verbose in the callers.

I'm also not sure that it's all that useful to distinguish errors from
fwrite() versus fclose(). In practice, errors will come from write() in
either case, and the caller does not have much control over when the
flushing happens. So any error saying "error closing file" is probably
assuming too much anyway. It should be "error writing file".

And I think in practice the messages end up quite generic anyway, as
they are really calling commit_lock_file(), which may also fail due to a
rename. So you get something like "unable to write 'foo': ", with errno
appended. That's _also_ potentially confusing when rename() fails.

Solving that would probably require passing down an "err" strbuf (or
other data structure) for the low-level code to fill in.

> > The only reason I do not think we should do so for close_tempfile() is
> > that the fclose is typically far away from the code that actually calls
> > error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> > state up through the stack (most of the calls indirectly come from
> > commit_lock_file(), I would think).
> 
> We _could_ clear errno to allow caller to tell them apart, though,
> if we wanted to ;-)

Hmm. So basically "errno = 0" instead of EIO? That at least has the
advantage that you can tell it apart from a real EIO, and a caller
_could_ if they chose do:

  if (commit_lock_file()) {
if (!errno)
error("error writing to file");
else
error_errno("error closing file");
  }

But I am not sure I would want to annotate all such callers that way. It
would probably be less bad to just pass down a "quiet" flag or a strbuf
and have the low-level code fill it in. And that solves this problem
_and_ the rename() thing above.

But TBH, I am not sure if it is worth it. Nobody is complaining. This is
just a thing we noticed. I think setting errno to EIO or to "0" is a
strict improvement over what is there (where the errno is essentially
random) and the complexity doesn't escape the function.

So I think that "easy" thing falls far short of a solution, but it's at
least easy. I could take it or leave it at this point.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

>> If we are trying to make sure that the caller would not say "failed
>> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
>> stdio opration, I am not sure if the patch improves things.  The
>> caller did not fail to close (most likely we successfully closed
>> it), and no matter what futzing we do to errno, the message supplied
>> by such a caller will not be improved.
>
> Right. EIO is almost certainly _not_ the error we saw. But I would
> rather consistently say "I/O error" and have the user scratch their
> head, look up this thread, and say "ah, it was probably a deferred
> error", as opposed to the alternative: the user sees something
> nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> may change from run to run based on what other code runs or fails in
> between.

My point was actually not what errno we feed to strerror().  In that
example, what is more misleading is the fixed part of the error
message the caller of close_tempfile() used after seeing the funcion
fail, i.e. "failed to close".  strerror() part is used to explain
why we "failed to close", and of course any nonsensical errno that
we did not get from the failed stdio call would not explain it, but
a more misleading part is that we did not even "failed to close" it.

We just noticed an earlier error while attempting to close it.
strerror() in the message does not even have to be related to the
closing of the file handle.

>> If the caller used "noticed an earlier error while closing tempfile:
>> ERRNO", such a message would describe the situation more correctly,
>> but then ERRNO that is not about stdio is probably acceptable in the
>> context of that message (the original ERRNO might be ENOSPC that is
>> even more specific than EIO, FWIW).  So I am not sure if the things
>> will improve from the status quo.
>
> Yes, that's I suggested that xfclose() is probably not a good direction.
> The _best_ thing we can do is have the caller not report errno at all
> (or even say "there was an earlier error, I have no idea what errno
> was"). And xfclose() works in the opposite direction.

I think we are in agreement on this point ;-)

> The only reason I do not think we should do so for close_tempfile() is
> that the fclose is typically far away from the code that actually calls
> error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> state up through the stack (most of the calls indirectly come from
> commit_lock_file(), I would think).

We _could_ clear errno to allow caller to tell them apart, though,
if we wanted to ;-)

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

[Jeff King]

On Fri, Feb 17, 2017 at 01:42:21PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
> >
> >> Stepping back a bit, would this be really needed?  Even if the ferror()
> >> does not update errno, the original stdio operation that failed
> >> would have, no?
> >
> > Sure, but we have no clue what happened in between.
> 
> Hmm, so we are protecting against somebody who does "errno = 0"
> explicitly, because she knows that she's dealt with the error from
> stdio earlier?  Such a careful person would have called clearerr()
> as well, I would guess.

I'm not sure I understand what you are saying here. If somebody calls
clearerr(), our ferror() handling does not trigger at all, and do not
care what is in errno either way. They can reset errno or not when they
clearerr(), but it is not relevant.

If you are asking about somebody who sets errno to "0" and _doesn't_
call clearerr(), then I don't know what that person is trying to
accomplish. Setting errno to "0" is not the right way to clear an error.
And they certainly should not be relying on it not to get overwritten
before we make it to the final ferror()/fclose().

> So let's assume we only care about the case where some other error
> was detected and errno was updated by a system library call.

Right.

> > I think our emails crossed, but our patches are obviously quite similar.
> > My commit message maybe explains a bit more of my thinking.
> 
> Yes, but ;-)
> 
> If we are trying to make sure that the caller would not say "failed
> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
> stdio opration, I am not sure if the patch improves things.  The
> caller did not fail to close (most likely we successfully closed
> it), and no matter what futzing we do to errno, the message supplied
> by such a caller will not be improved.

Right. EIO is almost certainly _not_ the error we saw. But I would
rather consistently say "I/O error" and have the user scratch their
head, look up this thread, and say "ah, it was probably a deferred
error", as opposed to the alternative: the user sees something
nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
may change from run to run based on what other code runs or fails in
between.

> If the caller used "noticed an earlier error while closing tempfile:
> ERRNO", such a message would describe the situation more correctly,
> but then ERRNO that is not about stdio is probably acceptable in the
> context of that message (the original ERRNO might be ENOSPC that is
> even more specific than EIO, FWIW).  So I am not sure if the things
> will improve from the status quo.

Yes, that's I suggested that xfclose() is probably not a good direction.
The _best_ thing we can do is have the caller not report errno at all
(or even say "there was an earlier error, I have no idea what errno
was"). And xfclose() works in the opposite direction.

The only reason I do not think we should do so for close_tempfile() is
that the fclose is typically far away from the code that actually calls
error(). We'd have to pass the tristate (success, fail, fail-with-errno)
state up through the stack (most of the calls indirectly come from
commit_lock_file(), I would think).

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
>
>> Stepping back a bit, would this be really needed?  Even if the ferror()
>> does not update errno, the original stdio operation that failed
>> would have, no?
>
> Sure, but we have no clue what happened in between.

Hmm, so we are protecting against somebody who does "errno = 0"
explicitly, because she knows that she's dealt with the error from
stdio earlier?  Such a careful person would have called clearerr()
as well, I would guess.

So let's assume we only care about the case where some other error
was detected and errno was updated by a system library call.

> I think our emails crossed, but our patches are obviously quite similar.
> My commit message maybe explains a bit more of my thinking.

Yes, but ;-)

If we are trying to make sure that the caller would not say "failed
to close tempfile: ERRNO" with an ERRNO that is unrelated to any
stdio opration, I am not sure if the patch improves things.  The
caller did not fail to close (most likely we successfully closed
it), and no matter what futzing we do to errno, the message supplied
by such a caller will not be improved.

If the caller used "noticed an earlier error while closing tempfile:
ERRNO", such a message would describe the situation more correctly,
but then ERRNO that is not about stdio is probably acceptable in the
context of that message (the original ERRNO might be ENOSPC that is
even more specific than EIO, FWIW).  So I am not sure if the things
will improve from the status quo.

It's easy for us to either apply the patch and be done with it (or
drop and do the same), and in the bigger picture it wouldn't make
that much of a difference, I would think, so I can go either way.

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

[Jeff King]

On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:

> > I guess we are simultaneously assuming that it is OK to munge errno on
> > success in our function, but that fclose() will not do so. Which seems a
> > bit hypocritical. Maybe the "if" dance is better.
> 
> Yes.  When both ferror() and fclose() are successful, we would
> prefer to see the original errno unmolested, so the "if" dance,
> even though it looks uglier, is better.  The ugliness is limited
> to the implementation anyway ;-)
> 
> But it does look ugly, especially when nested inside the existing
> code like so.

I think our emails crossed, but our patches are obviously quite similar.
My commit message maybe explains a bit more of my thinking.

> Stepping back a bit, would this be really needed?  Even if the ferror()
> does not update errno, the original stdio operation that failed
> would have, no?

Sure, but we have no clue what happened in between.

-Peff

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

[Junio C Hamano]

Jeff King  writes:

> On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:
>
>> On 02/17/2017 09:07 AM, Jeff King wrote:
>> > [...]
>> > That's similar to what I wrote earlier, but if we don't mind overwriting
>> > errno unconditionally, I think just:
>> > 
>> >   errno = EIO; /* covers ferror(), overwritten by failing fclose() */
>> >   err |= ferror(fp);
>> >   err |= fclose(fp);
>> > 
>> > does the same thing.
>> 
>> True; I'd forgotten the convention that non-failing functions are
>> allowed to change errno. Your solution is obviously simpler and faster.
>
> I guess we are simultaneously assuming that it is OK to munge errno on
> success in our function, but that fclose() will not do so. Which seems a
> bit hypocritical. Maybe the "if" dance is better.

Yes.  When both ferror() and fclose() are successful, we would
prefer to see the original errno unmolested, so the "if" dance,
even though it looks uglier, is better.  The ugliness is limited
to the implementation anyway ;-)

But it does look ugly, especially when nested inside the existing
code like so.

Stepping back a bit, would this be really needed?  Even if the ferror()
does not update errno, the original stdio operation that failed
would have, no?

-- >8 --
Subject: close_tempfile(): set errno when ferror() notices a previous error

In close_tempfile(), we may notice that previous stdio operations
failed when we inspect ferror(tempfile->fp).  As ferror() does not
set errno, and the caller of close_tempfile(), since it encountered
and ignored the original error, is likely to have called other
system library functions to cause errno to be modified, the caller
cannot really tell anything meaningful by looking at errno after we
return an error from here.  

Set errno to an arbitrary value EIO when ferror() sees an error but
fclose() succeeds.  If fclose() fails, we just let the caller see
errno from that failure.

---
 tempfile.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index ffcc272375..d2c6de83a9 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,8 +247,20 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
-   err = ferror(fp);
-   err |= fclose(fp);
+   if (ferror(fp)) {
+   err = -1;
+   if (!fclose(fp))
+   /*
+* There was some error detected by ferror()
+* but it is likely that the true errno has
+* long gone.  Leave something generic to make
+* it clear that the caller cannot rely on errno
+* at this point.
+*/
+   errno = EIO;
+   } else {
+   err = fclose(fp);
+   }
} else {
err = close(fd);
}

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

[Jeff King]

On Fri, Feb 17, 2017 at 03:54:42PM -0500, Jeff King wrote:

> I guess we are simultaneously assuming that it is OK to munge errno on
> success in our function, but that fclose() will not do so. Which seems a
> bit hypocritical. Maybe the "if" dance is better.

So here's that patch with a justification.

At this point, this snippet of code would be appropriate to pull into
xfclose() if we wanted. But possibly that is the wrong direction, as it
encourages callers to do:

  if (xfclose(fp))
err = error_errno("failure writing to ...");

when they could do:

  if (ferror(fp))
err = error("failure writing to ...");
  if (fclose(fp))
err = error_errno("failure writing to ...");

While longer, it's arguably better for them to distinguish the two
cases. It's only worth doing the errno magic when the close is deep
inside a callstack, and passing out the two cases is awkward.

-- >8 --
Subject: tempfile: set errno to a known value before calling ferror()

In close_tempfile(), we return an error if ferror()
indicated a previous failure, or if fclose() failed. In the
latter case, errno is set and it is useful for callers to
report it.

However, if _only_ ferror() triggers, then the value of
errno is based on whatever syscall happened to last fail,
which may not be related to our filehandle at all. A caller
cannot tell the difference between the two cases, and may
use "die_errno()" or similar to report a nonsense errno value.

One solution would be to actually pass back separate return
values for the two cases, so a caller can write a more
appropriate message for each case. But that makes the
interface clunky.

Instead, let's just set errno to the generic EIO in this case.
That's not as descriptive as we'd like, but at least it's
predictable. So it's better than the status quo in all cases
but one: when the last syscall really did involve a failure
on our filehandle, we'll be wiping that out. But that's a
fragile thing for us to rely on.

In any case, we'll let the errno result from fclose() take
precedence over our value, as we know that's recent and
accurate (and many I/O errors will persist through the
fclose anyway).

Signed-off-by: Jeff King 
---
 tempfile.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index ffcc27237..684371067 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -247,8 +247,13 @@ int close_tempfile(struct tempfile *tempfile)
tempfile->fd = -1;
if (fp) {
tempfile->fp = NULL;
-   err = ferror(fp);
-   err |= fclose(fp);
+   if (ferror(fp)) {
+   err = -1;
+   if (!fclose(fp))
+   errno = EIO;
+   } else {
+   err = fclose(fp);
+   }
} else {
err = close(fd);
}
-- 
2.12.0.rc1.612.ga5f664feb

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

[Jeff King]

On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:

> On 02/17/2017 09:07 AM, Jeff King wrote:
> > [...]
> > That's similar to what I wrote earlier, but if we don't mind overwriting
> > errno unconditionally, I think just:
> > 
> >   errno = EIO; /* covers ferror(), overwritten by failing fclose() */
> >   err |= ferror(fp);
> >   err |= fclose(fp);
> > 
> > does the same thing.
> 
> True; I'd forgotten the convention that non-failing functions are
> allowed to change errno. Your solution is obviously simpler and faster.

I guess we are simultaneously assuming that it is OK to munge errno on
success in our function, but that fclose() will not do so. Which seems a
bit hypocritical. Maybe the "if" dance is better.

-Peff

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

[Michael Haggerty]

On 02/17/2017 09:07 AM, Jeff King wrote:
> [...]
> That's similar to what I wrote earlier, but if we don't mind overwriting
> errno unconditionally, I think just:
> 
>   errno = EIO; /* covers ferror(), overwritten by failing fclose() */
>   err |= ferror(fp);
>   err |= fclose(fp);
> 
> does the same thing.

True; I'd forgotten the convention that non-failing functions are
allowed to change errno. Your solution is obviously simpler and faster.

Michael

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

[Jeff King]

On Fri, Feb 17, 2017 at 09:00:09AM +0100, Michael Haggerty wrote:

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

Yeah, we discussed this up-thread a bit, and my "solution" was similar
to yours. I don't like it, because EIO is a real thing that can happen,
too, and it would certainly be surprising to a user to see. But it's
probably better than the alternative, which is getting whatever random
value happened to be in errno.

The only downside is that if the value of errno _was_ valid (because the
last thing you did really was writing to the filehandle, then we'd
overwrite it).

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

Yes, I think we're better to take what fclose gives us, if we can.

> 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();

That's similar to what I wrote earlier, but if we don't mind overwriting
errno unconditionally, I think just:

  errno = EIO; /* covers ferror(), overwritten by failing fclose() */
  err |= ferror(fp);
  err |= fclose(fp);

does the same thing.

-Peff

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

[Michael Haggerty]

On 02/16/2017 10:31 PM, Jeff King wrote:
> 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 <sch...@linux-m68k.org>
> Signed-off-by: Jeff King <p...@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

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

[Jeff King]

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 <sch...@linux-m68k.org>
Signed-off-by: Jeff King <p...@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);
}
-- 
2.12.0.rc1.559.gd292418bf