patch, 2 partsUse CLOEXEC to avoid fd leaks

From: Lars Schneider <larsxschneider@gmail.com>

Use the CLOEXEC flag similar to 05d1ed61 to fix leaked file descriptors.

Patch 1/2 was contemplated by Junio here: http://public-inbox.org/git/xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com/

I followed Torsten's advice and renamed `git_open_noatime` to `git_open_noatime_cloexec`: http://public-inbox.org/git/20160830145429.GA11221@tb-raspi/

Patch 2/2 was part of my "Git filter protocol" series: http://public-inbox.org/git/20160825110752.31581-14-larsxschneider@gmail.com/ (I kept the patch as-is but changed the commit messages slightly)

Thanks, Lars

Lars Schneider (2):
  sha1_file: open window into packfiles with CLOEXEC
  read-cache: make sure file handles are not inherited by child
    processes

 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 pack-bitmap.c          |  2 +-
 read-cache.c           |  2 +-
 sha1_file.c            | 14 +++++++-------
 5 files changed, 11 insertions(+), 11 deletions(-)

-- 2.10.0

From: Lars Schneider <larsxschneider@gmail.com>

All processes that the Git main process spawns inherit the open file descriptors of the main process. These leaked file descriptors can cause problems.

Use the CLOEXEC flag similar to 05d1ed61 to fix the leaked file descriptors.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 builtin/pack-objects.c |  2 +-
 cache.h                |  2 +-
 pack-bitmap.c          |  2 +-
 sha1_file.c            | 14 +++++++-------
 4 files changed, 10 insertions(+), 10 deletions(-)

larsxschneider@gmail.com wrote:

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.

I question the need for the "_cloexec" suffixing in the function name since the old function is going away entirely.

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.

Fwiw, Perl has been setting cloexec on FDs above $^F (2, $SYSTEM_FD_MAX) for decades, and Ruby started doing it a few years ago, too.

W dniu 06.09.2016 o 00:27, Eric Wong pisze:

[...]

On the other hand the new name is descriptive...

Hi Eric & Lars,

On Mon, 5 Sep 2016, Eric Wong wrote:

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.

Me, too. While it is correct, it makes things harder to read, so it may even cause more harm than it does good.

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

What name would you suggest? Leaving the name as-is seems misleading to me. Maybe just "git_open()" ?

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;

Lars Schneider <larsxschneider@gmail.com> wrote:

Maybe "_noatime" is useful in some cases, but maybe not *shrug*

My original point for removing the "_cloexec" suffix was that (at least for Perl and Ruby), cloexec-by-default was so prevalent in FD-creating syscalls that having the suffix wasn't needed.

80 columns overflow

But otherwise much better since it doesn't blindly zero sha1_file_open_flag :>

From: Lars Schneider <larsxschneider@gmail.com>

This fix prepares a series with the goal to avoid launching a new clean/smudge filter process for each file that is filtered. A new long running filter process is introduced that is used to filter all files in a single Git invocation.

Consider the case of a file that requires filtering and is present in branch A but not in branch B. If A is the current HEAD and we checkout B then the following happens:

1. ce_compare_data() opens the file
2.   index_fd() detects that the file requires to run a clean filter and
     calls index_stream_convert_blob()
4.     index_stream_convert_blob() calls convert_to_git_filter_fd()
5.       convert_to_git_filter_fd() calls apply_filter() which creates a
         new long running filter process (in case it is the first file
         of this kind to be filtered)
6.       The new filter process inherits all file handles. This is the
         default on Linux/OSX and is explicitly defined in the
         `CreateProcessW` call in `mingw.c` on Windows.
7. ce_compare_data() closes the file
8. Git unlinks the file as it is not present in B

The unlink operation does not work on Windows because the filter process has still an open handle to the file. On Linux/OSX the unlink operation succeeds but the file descriptors still leak into the child process.

Fix this problem by opening files in read-cache with the CLOEXEC flag to ensure that the file descriptor does not remain open in a newly spawned process similar to 05d1ed61.

Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
 read-cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Hi Lars,

On Mon, 5 Sep 2016, larsxschneider@gmail.com wrote:

Makes sense.

Eric's comment on 1/2 applies here, too, of course: should this cause any problems on non-Windows platforms, we always have that FD_CLOEXEC thing that we could probably use to fix it.

But let's cross that bridge when (or better: if) we get there.

Ciao, Dscho

larsxschneider@gmail.com wrote:

Also, this needs to check EINVAL when O_CLOEXEC != 0 the same way create_tempfile currently does. Somebody could be building with modern headers but running an old kernel that doesn't understand O_CLOEXEC.

There should probably be a open() wrapper for handling this case since we're now up to 3 places where open(... O_CLOEXEC) is used.

Right! Actually "sha1_file.c:git_open_noatime()" is already a wrapper, no? Can't we use this here? The O_NOATIME flag shouldn't hurt, right?

Thanks, Lars

Lars Schneider <larsxschneider@gmail.com> wrote:

For ce_compare_data (and other O_RDONLY users), I guess git_open_noatime is fine; and probably preferable because of O_NOATIME.

We probably should be using O_NOATIME for all O_RDONLY cases to get the last bit of performance out (especially since non-modern-Linux systems probably still lack relatime).

However, create_tempfile needs O_RDWR|O_CREAT|O_EXCL but I guess we can clean that up in another series.

Eric Wong <e@80x24.org> writes:

No, please do not go there.

The user can read from a file in a working tree using "less", "grep", etc., and they all update the atime, so should "git grep". We do not use atime ourselves on these files but we should let outside tools rely on the validity of atime (e.g. "what are the files that were looked at yesterday?").

If you grep for noatime in our current codebase, you'd notice that we use it only for files in objects/ hierarchy, and that makes very good sense. These files are what we create for our _sole_ use and no other tools can peek at them and expect to get any useful information out of them (we hear from time to time that virus scanners leaving open file descriptors on them causing trouble, but that is an example of a useless access), and that makes a file in objects/ hierarchy a fair game for noatime optimization.

How do we deal with read-cache:ce_compare_data, though?

By your definition above we shouldn't use NOATIME since the read file is not in objects/. However, the file read is not something the user explicitly triggers. The read is part of the Git internal "clean" machinery.

What would you suggest? Should I open the file with or without NOATIME?

Thanks, Lars

Lars Schneider <larsxschneider@gmail.com> writes:

We are using open() in our current code in that codepath, without NOATIME. We shouldn't start using git_open_noatime() merely for convenience.

We would probably want to do a preliminary refactoring so that we can say "we want only CLOEXEC and not NOATIME". Perhaps we would want to make the existing one in sha1_file.c to something like:

	int git_open_noatime(const char *name)
        {
		return git_open(name, GIT_OPEN_NOATIME);
	}

and then add

	#define GIT_OPEN_NOATIME 01

to cache.h and add

	int git_open(const char *name, unsigned int flag)
        {
        	static int open_noatime = O_NOATIME;

		for (;;) {
                	int fd, open_flag;

                        open_flag = 0;
                        if (flag & GIT_OPEN_NOATIME)
                                open_flag |= open_noatime;

			errno = 0;
                        fd = open(name, O_RDONLY | open_flag);
                        if (fd >= 0)
                        	return fd;

			if (errno == ENOENT || !open_flag)
				return -1;

			/* The failure may be due to additional	flags */
                       	if ((flag & GIT_OPEN_NOATIME) &&
			    (open_flag & O_NOATIME)) {
				flag &= ~GIT_OPEN_NOATIME;
				open_noatime = 0;
			}
		}
	}

to wrapper.c in the first step, which is a "no-op refactoring" step.

Then add

	#define GIT_OPEN_CLOEXEC 02

and update git_open(), perhaps like so:

	int git_open(const char *name, unsigned int flag)
        {
        	static int open_noatime = O_NOATIME;
        	static int open_cloexec = O_CLOEXEC;

		for (;;) {
                	int fd, open_flag;

                        open_flag = 0;
                        if (flag & GIT_OPEN_NOATIME)
                                open_flag |= open_noatime;
                        if (flag & GIT_OPEN_CLOEXEC)
                                open_flag |= open_cloexec;

			errno = 0;
                        fd = open(name, O_RDONLY | open_flag);
                        if (fd >= 0)
                        	return fd;

			if (errno == ENOENT || !open_flag)
				return -1;

			/* The failure may be due to additional	flags */
                       	if ((flag & GIT_OPEN_NOATIME) &&
			    (open_flag & O_NOATIME)) {
				flag &= ~GIT_OPEN_NOATIME;
                                open_noatime = 0;
			}
                       	if ((flag & GIT_OPEN_CLOEXEC) &&
			    (open_flag & O_CLOEXEC)) {
				flag &= ~GIT_OPEN_CLOEXEC;
                                open_cloexec = 0;
			}
		}
	}

The retry logic is "if we were asked to do this flag, and if we did pass that flag, then we know open() with that flag fails here, so we won't waste time trying with it again", which came from the NOATIME codepath we already have, but it may not match what we use CLOEXEC for and may need to be adjusted. I didn't think that part of the code through.

Then the ce codepath that reads from the working tree would use

	git_open(ce->name, GIT_OPEN_CLOEXEC);

to obtain the file descriptor for reading, perhaps?