Re: [PATCH v5 3/8] convert: Split start_multi_file_filter into two separate functions
flat view
From: Jeff King <hidden>
Date: 2017-04-11 19:37:17
On Tue, Apr 11, 2017 at 09:29:36PM +0200, Lars Schneider wrote:
quoted
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).
quoted
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