Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 6, 2016, 11:38 UTC
Message-ID: <alpine.DEB.2.20.1609061333030.129229@virtualbox>
In-Reply-To: <20160905222715.GA30031@starla>

Hi Eric & Lars,

On Mon, 5 Sep 2016, Eric Wong wrote:

Show 19 quoted lines

> larsxschneider@gmail.com 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;
> 
> If there's real problems being caused by lack of cloexec
> today, I think the F_SETFD fallback I proposed in
> https://public-inbox.org/git/20160818173555.GA29253@starla/
> will be necessary.

Yes, it is good to have that patch available to go if we need it. I do not think that we will need it, though, as the biggest problems that are solved through the CLOEXEC flag are ones caused on Windows, when files cannot be deleted or renamed because there are still (uselessly) open handles referencing them.

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

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

Ciao, Dscho