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

Junio C Hamano [off-list ref] wrote:

Junio C Hamano [off-list ref] writes:

quoted

Linus Torvalds [off-list ref] writes:

quoted

On Thu, Oct 27, 2016 at 4:36 PM, Junio C Hamano [off-list ref] wrote:

quoted

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.

quoted

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;