Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC

From: Eric Wong <e@80x24.org>
Date: Oct 28, 2016, 05:51 UTC
Message-ID: <20161028055159.GA25950@starla>
In-Reply-To: <xmqq60od42s0.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Linus Torvalds <torvalds@linux-foundation.org> writes:
> >
> >> On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >>>
> >>> Would the best endgame shape for this function be to open with
> >>> O_NOATIME (and retry without), and then add CLOEXEC with fcntl(2)
> >>> but ignoring an error from it, I guess?  That would be the closest
> >>> to what we historically had, I would think.
> >>
> >> I think that's the best model.

Actually, I would flip the order of flags. O_CLOEXEC is more important from a correctness standpoint.

Show 7 quoted lines

> > OK, so perhaps like this.
> 
> Hmph.  This may not fly well in practice, though.  
> 
> To Unix folks, CLOEXEC is not a huge correctness issue.  A child
> process may hold onto an open file descriptor a bit longer than the
> lifetime of the parent but as long as the child eventually exits,

I'm not too familiar with C internals of git; but I know we use threads in some places, and fork+execve in others.

If our usage of threads and execve intersects, and we run untrusted code in an execve-ed child, then only having cloexec on open() will save us time when auditing for leaking FDs.

fcntl(fd, F_SETFD, O_CLOEXEC) is racy in if there are other threads doing execve; so I wouldn't rely on it as a first choice.

So I suppose something like this:

	static int noatime = 1;
	int fd = open(... | O_CLOEXEC);
	...error checking and retrying...

	if (fd >= 0 && noatime && fcntl(fd, F_SETFL, O_NOATIME) != 0)
		noatime = 0;

	return fd;