> On 11 Apr 2017, at 18:16, Jeff King <[email protected]> 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
