> On 06 Sep 2016, at 13:38, Johannes Schindelin <johannes.schindelin@xxxxxx> wrote:
>
> Hi Eric & Lars,
>
> On Mon, 5 Sep 2016, Eric Wong wrote:
>
>> larsxschneider@xxxxxxxxx wrote:
>>> All processes that the Git main process spawns inherit the open file
>>> descriptors of the main process. These leaked file descriptors can
>>> cause problems.
>>
>>
>>> -int git_open_noatime(const char *name)
>>> +int git_open_noatime_cloexec(const char *name)
>>> {
>>> - static int sha1_file_open_flag = O_NOATIME;
>>> + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
>>>
>>> for (;;) {
>>> int fd;
>
>> I question the need for the "_cloexec" suffixing in the
>> function name since the old function is going away entirely.
>
> Me, too. While it is correct, it makes things harder to read, so it may
> even cause more harm than it does good.
What name would you suggest? Leaving the name as-is seems misleading to me.
Maybe just "git_open()" ?
>> I prefer all FD-creating functions set cloexec by default
>> for FD > 2 to avoid inadvertantly leaking FDs. So we
>> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...
>> and fallback to the racy+slower F_SETFD when not available.
>
> In the original Pull Request where the change was contributed to Git for
> Windows, this was tested (actually, the code did not see whether fd > 2,
> but simply assumed that all newly opened file descriptors would be > 2
> anyway), and it failed:
>
> https://github.com/git-for-windows/git/pull/755#issuecomment-220247972
>
> So it appears that we would have to exclude at least the code path to `git
> upload-pack` from that magic.
I just realized that Dscho improved his original patch in GfW with a
fallback if CLOEXEC is not present.
I applied the same mechanism here. Would that be OK?
Thanks,
Lars
- static int sha1_file_open_flag = O_NOATIME;
+ static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
for (;;) {
int fd;
@@ -1471,12 +1471,17 @@ int git_open_noatime(const char *name)
if (fd >= 0)
return fd;
- /* Might the failure be due to O_NOATIME? */
- if (errno != ENOENT && sha1_file_open_flag) {
- sha1_file_open_flag = 0;
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ if (O_CLOEXEC && errno == EINVAL && (sha1_file_open_flag & O_CLOEXEC)) {
+ sha1_file_open_flag &= ~O_CLOEXEC;
continue;
}
+ /* Might the failure be due to O_NOATIME? */
+ if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
+ sha1_file_open_flag &= ~O_NOATIME;
+ continue;
+ }