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

On 11 Apr 2017, at 18:16, Jeff King [off-list ref] wrote:

On Fri, Apr 07, 2017 at 08:03:49AM -0400, Ben Peart wrote:

quoted

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