Re: [RFC PATCH] vreportf: ensure sensible ordering of normal and error output

> 
> On Tue, Nov 30 2021, Jeff King wrote:
> 
> > On Tue, Nov 30, 2021 at 02:14:25AM -0500, Jeff King wrote:
> >
> >>   - flushing causes us to block. This implies our stdout is connected to
> >>     a pipe or socket, and the other side is not expecting to read. A
> >>     plausible case here is a client sending us a big input which we find
> >>     to be bogus (maybe index-pack checking an incoming pack). We call
> >>     die() to complain about the input, but the client is still writing.
> >>     In the current code, we'd write out our error and then exit; the
> >>     client would get SIGPIPE or a write() error and abort. But with a
> >>     flush here, we could block writing back to the client, and now we're
> >>     in a deadlock; they are trying to write to us but we are no longer
> >>     reading, and we are blocked trying to get out a little bit of
> >>     irrelevant stdout data.
> >> 
> >>     I _think_ we're probably OK here. The scenario above means that the
> >>     caller is already doing asynchronous I/O via stdio and is subject to
> >>     deadlock. Because the segment of buffer we try to flush here _could_
> >>     have been flushed already under the hood, which would have caused
> >>     the same blocking. A careful caller might be using select() or
> >>     similar to decide when it is OK to write, but I find it highly
> >>     unlikely they'd be using stdio in that case.
> >> 
> >> Of the two, the deadlock case worries me more, just because it would be
> >> quiet subtle and racy. As I said, I think we may be OK, but my reasoning
> >> there is pretty hand-wavy.
> >
> > Thinking on this a bit more: I guess as soon as we exit libc would call
> > the equivalent of fflush(NULL) anyway, and try that same flush. So in a
> > sense this is just ordering a bit differently, and not introducing any
> > new problems. (Unless libc is clever enough to avoid blocking, but that
> > doesn't seem like something we could or should rely on in general).
> 
> I think this change is probably OK too, but let's not forget about
> warning() and error(). I.e. we are not always on a path to a fatal error
> with vreportf(), that's just with die(), usage() and BUG().