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

From: Lars Schneider <larsxschneider@gmail.com>
Date: Apr 11, 2017, 19:29 UTC
Message-ID: <FD58071A-4DC7-4AB7-8B6B-56B71E715144@gmail.com>
In-Reply-To: <20170411161617.fyu5pmzgplscoozz@sigill.intra.peff.net>

Show 17 quoted lines

> On 11 Apr 2017, at 18:16, Jeff King <peff@peff.net> wrote:
> 
> On Fri, Apr 07, 2017 at 08:03:49AM -0400, Ben Peart wrote:
> 
>> @@ -642,7 +621,41 @@ static struct cmd2process *start_multi_file_filter(struct hashmap *hashmap, cons
>> done:
>> 	sigchain_pop(SIGPIPE);
>> 
>> -	if (err || errno == EPIPE) {
>> +	if (err || errno == EPIPE)
>> +		err = err ? err : errno;
>> +
>> +	return err;
>> +}
> 
> This isn't a new problem introduced by your patch, but this use of errno
> seems funny to me. Specifically:

I introduced these lines, therefore I try to answer :-)

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

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

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?

- Lars