Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions

[Jeff King <peff@peff.net>]

On Tue, Apr 11, 2017 at 09:29:36PM +0200, Lars Schneider wrote:

> >  1. Do we need to save errno before calling sigchain_pop()? It's making
> >     syscalls (though admittedly they are unlikely to fail).
> 
> What if we add the following right before sigchain_pop() ?
> 
> 	if (errno == EPIPE)
> 		err = -1;

Yes, that would be fine (though again, this runs against point 2 below).

> >  2. If err is 0, then nothing failed. Who would have set errno? Aren't
> >     we reading whatever cruft happened to be in errno before the
> >     function started?
> 
> Yeah, looks like you're right:
> https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=6619179
> 
> According to this article we shouldn't even check *only* for errno. 
> At least we should add
> 	errno = 0;
> at the beginning of the function, no?

If you initialize errno to 0 right before a syscall, then yes, you can
trust it without checking the return value of the syscall. I wouldn't
trust it before calling more complicated functions, though. Not even
xwrite(), which may see EINTR and keep going (which is OK for checking
for EPIPE, but not checking generally for errno values).

> This means we have many areas in Git where we don't handle errno
> correctly. E.g. right in convert.c where I stole code from:
> https://github.com/git/git/commit/0c4dd67a048b39470b9b95912e4912fecc405a85#diff-7949b716ab0a83e8c422a0d6336f19d6R361
> 
> Should that be addressed?

That one is questionable code, but I don't think it behaves incorrectly.
After the write_in_full() call finishes, then either:

  1. write_err is 0, and conditional is a noop

  2. write_err is non-zero, and errno is relevant

I do think it would be more clear as:

  if (write_err && errno == EPIPE)
	write_err = 0;

similar to the code right below it.

-Peff