{"thread":{"id":"44014","subject":"[PATCH v1 0/2] Use CLOEXEC to avoid fd leaks","startedAt":"2016-09-05T21:11:19Z","lastAt":"2016-09-08T17:38:04Z","messageCount":15,"participants":["larsxschneider@gmail.com","Eric Wong","Jakub Narębski","Johannes Schindelin","Lars Schneider","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"301138","messageId":"20160905211111.72956-1-larsxschneider@gmail.com","threadId":"44014","inReplyTo":null,"subject":"[PATCH v1 0/2] Use CLOEXEC to avoid fd leaks","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-05T21:11:09Z","receivedAt":"2016-09-05T21:11:19Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nUse the CLOEXEC flag similar to 05d1ed61 to fix leaked file descriptors.\n\nPatch 1/2 was contemplated by Junio here:\nhttp://public-inbox.org/git/xmqq37lnbbpk.fsf@gitster.mtv.corp.google.com/\n\nI followed Torsten's advice and renamed `git_open_noatime` to\n`git_open_noatime_cloexec`:\nhttp://public-inbox.org/git/20160830145429.GA11221@tb-raspi/\n\nPatch 2/2 was part of my \"Git filter protocol\" series:\nhttp://public-inbox.org/git/20160825110752.31581-14-larsxschneider@gmail.com/\n(I kept the patch as-is but changed the commit messages slightly)\n\nThanks,\nLars\n\nLars Schneider (2):\n sha1_file: open window into packfiles with CLOEXEC\n read-cache: make sure file handles are not inherited by child\n processes\n\n builtin/pack-objects.c | 2 +-\n cache.h | 2 +-\n pack-bitmap.c | 2 +-\n read-cache.c | 2 +-\n sha1_file.c | 14 +++++++-------\n 5 files changed, 11 insertions(+), 11 deletions(-)\n\n--\n2.10.0\n\n"},{"id":"301139","messageId":"20160905211111.72956-2-larsxschneider@gmail.com","threadId":"44014","inReplyTo":"20160905211111.72956-1-larsxschneider@gmail.com","subject":"[PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-05T21:11:10Z","receivedAt":"2016-09-05T21:11:24Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nAll processes that the Git main process spawns inherit the open file\ndescriptors of the main process. These leaked file descriptors can\ncause problems.\n\nUse the CLOEXEC flag similar to 05d1ed61 to fix the leaked file\ndescriptors.\n\nSigned-off-by: Lars Schneider \n---\n builtin/pack-objects.c | 2 +-\n cache.h | 2 +-\n pack-bitmap.c | 2 +-\n sha1_file.c | 14 +++++++-------\n 4 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 4a63398..a2b1fb6 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -718,7 +718,7 @@ static off_t write_reused_pack(struct sha1file *f)\n \tif (!is_pack_valid(reuse_packfile))\n \t\tdie(\"packfile is invalid: %s\", reuse_packfile->pack_name);\n \n-\tfd = git_open_noatime(reuse_packfile->pack_name);\n+\tfd = git_open_noatime_cloexec(reuse_packfile->pack_name);\n \tif (fd < 0)\n \t\tdie_errno(\"unable to open packfile for reuse: %s\",\n \t\t\t reuse_packfile->pack_name);\ndiff --git a/cache.h b/cache.h\nindex b780a91..ae79747 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1089,7 +1089,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,\n extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n-extern int git_open_noatime(const char *name);\n+extern int git_open_noatime_cloexec(const char *name);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex b949e51..1b39e5d 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -266,7 +266,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)\n \t\treturn -1;\n \n \tidx_name = pack_bitmap_filename(packfile);\n-\tfd = git_open_noatime(idx_name);\n+\tfd = git_open_noatime_cloexec(idx_name);\n \tfree(idx_name);\n \n \tif (fd < 0)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3045aea..c1701dc 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -356,7 +356,7 @@ void read_info_alternates(const char * relative_base, int depth)\n \tint fd;\n \n \tpath = xstrfmt(\"%s/info/alternates\", relative_base);\n-\tfd = git_open_noatime(path);\n+\tfd = git_open_noatime_cloexec(path);\n \tfree(path);\n \tif (fd < 0)\n \t\treturn;\n@@ -550,7 +550,7 @@ static int check_packed_git_idx(const char *path, struct packed_git *p)\n \tstruct pack_idx_header *hdr;\n \tsize_t idx_size;\n \tuint32_t version, nr, i, *index;\n-\tint fd = git_open_noatime(path);\n+\tint fd = git_open_noatime_cloexec(path);\n \tstruct stat st;\n \n \tif (fd < 0)\n@@ -956,7 +956,7 @@ static int open_packed_git_1(struct packed_git *p)\n \twhile (pack_max_fds <= pack_open_fds && close_one_pack())\n \t\t; /* nothing */\n \n-\tp->pack_fd = git_open_noatime(p->pack_name);\n+\tp->pack_fd = git_open_noatime_cloexec(p->pack_name);\n \tif (p->pack_fd < 0 || fstat(p->pack_fd, &st))\n \t\treturn -1;\n \tpack_open_fds++;\n@@ -1459,9 +1459,9 @@ int check_sha1_signature(const unsigned char *sha1, void *map,\n \treturn hashcmp(sha1, real_sha1) ? -1 : 0;\n }\n \n-int git_open_noatime(const char *name)\n+int git_open_noatime_cloexec(const char *name)\n {\n-\tstatic int sha1_file_open_flag = O_NOATIME;\n+\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n \n \tfor (;;) {\n \t\tint fd;\n@@ -1505,7 +1505,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tstruct alternate_object_database *alt;\n \tint most_interesting_errno;\n \n-\tfd = git_open_noatime(sha1_file_name(sha1));\n+\tfd = git_open_noatime_cloexec(sha1_file_name(sha1));\n \tif (fd >= 0)\n \t\treturn fd;\n \tmost_interesting_errno = errno;\n@@ -1513,7 +1513,7 @@ static int open_sha1_file(const unsigned char *sha1)\n \tprepare_alt_odb();\n \tfor (alt = alt_odb_list; alt; alt = alt->next) {\n \t\tfill_sha1_path(alt->name, sha1);\n-\t\tfd = git_open_noatime(alt->base);\n+\t\tfd = git_open_noatime_cloexec(alt->base);\n \t\tif (fd >= 0)\n \t\t\treturn fd;\n \t\tif (most_interesting_errno == ENOENT)\n-- \n2.10.0\n\n"},{"id":"301140","messageId":"20160905211111.72956-3-larsxschneider@gmail.com","threadId":"44014","inReplyTo":"20160905211111.72956-1-larsxschneider@gmail.com","subject":"[PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-05T21:11:11Z","receivedAt":"2016-09-05T21:11:27Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider \n\nThis fix prepares a series with the goal to avoid launching a new\nclean/smudge filter process for each file that is filtered. A new\nlong running filter process is introduced that is used to filter all\nfiles in a single Git invocation.\n\nConsider the case of a file that requires filtering and is present in\nbranch A but not in branch B. If A is the current HEAD and we checkout B\nthen the following happens:\n\n1. ce_compare_data() opens the file\n2. index_fd() detects that the file requires to run a clean filter and\n calls index_stream_convert_blob()\n4. index_stream_convert_blob() calls convert_to_git_filter_fd()\n5. convert_to_git_filter_fd() calls apply_filter() which creates a\n new long running filter process (in case it is the first file\n of this kind to be filtered)\n6. The new filter process inherits all file handles. This is the\n default on Linux/OSX and is explicitly defined in the\n `CreateProcessW` call in `mingw.c` on Windows.\n7. ce_compare_data() closes the file\n8. Git unlinks the file as it is not present in B\n\nThe unlink operation does not work on Windows because the filter process\nhas still an open handle to the file. On Linux/OSX the unlink operation\nsucceeds but the file descriptors still leak into the child process.\n\nFix this problem by opening files in read-cache with the CLOEXEC flag to\nensure that the file descriptor does not remain open in a newly spawned\nprocess similar to 05d1ed61.\n\nSigned-off-by: Lars Schneider \n---\n read-cache.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 491e52d..02f74d3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -156,7 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n {\n \tint match = -1;\n-\tint fd = open(ce->name, O_RDONLY);\n+\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n-- \n2.10.0\n\n"},{"id":"301152","messageId":"20160905222715.GA30031@starla","threadId":"44014","inReplyTo":"20160905211111.72956-2-larsxschneider@gmail.com","subject":"Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-09-05T22:27:15Z","receivedAt":"2016-09-05T22:27:35Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"larsxschneider@gmail.com wrote:\n> All processes that the Git main process spawns inherit the open file\n> descriptors of the main process. These leaked file descriptors can\n> cause problems.\n\n\n> -int git_open_noatime(const char *name)\n> +int git_open_noatime_cloexec(const char *name)\n> {\n> -\tstatic int sha1_file_open_flag = O_NOATIME;\n> +\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n> \n> \tfor (;;) {\n> \t\tint fd;\n\nIf there's real problems being caused by lack of cloexec\ntoday, I think the F_SETFD fallback I proposed in\nhttps://public-inbox.org/git/20160818173555.GA29253@starla/\nwill be necessary.\n\nI question the need for the \"_cloexec\" suffixing in the\nfunction name since the old function is going away entirely.\n\nI prefer all FD-creating functions set cloexec by default\nfor FD > 2 to avoid inadvertantly leaking FDs. So we\nought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...\nand fallback to the racy+slower F_SETFD when not available.\n\n\nFwiw, Perl has been setting cloexec on FDs above $^F\n(2, $SYSTEM_FD_MAX) for decades, and Ruby started\ndoing it a few years ago, too.\n"},{"id":"301180","messageId":"bd9fc74b-b9b0-7c09-9c22-02b6f0f3cec9@gmail.com","threadId":"44014","inReplyTo":"20160905222715.GA30031@starla","subject":"Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-06T09:36:33Z","receivedAt":"2016-09-06T09:36:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 06.09.2016 o 00:27, Eric Wong pisze:\n> larsxschneider@gmail.com wrote:\n\n>> -int git_open_noatime(const char *name)\n>> +int git_open_noatime_cloexec(const char *name)\n[...]\n> \n> I question the need for the \"_cloexec\" suffixing in the\n> function name since the old function is going away entirely.\n\nOn the other hand the new name is descriptive...\n\n> \n> I prefer all FD-creating functions set cloexec by default\n> for FD > 2 to avoid inadvertantly leaking FDs. So we\n> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...\n> and fallback to the racy+slower F_SETFD when not available.\n> \n> \n> Fwiw, Perl has been setting cloexec on FDs above $^F\n> (2, $SYSTEM_FD_MAX) for decades, and Ruby started\n> doing it a few years ago, too.\n \n\n"},{"id":"301181","messageId":"alpine.DEB.2.20.1609061333030.129229@virtualbox","threadId":"44014","inReplyTo":"20160905222715.GA30031@starla","subject":"Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T11:38:23Z","receivedAt":"2016-09-06T11:38:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric & Lars,\n\nOn Mon, 5 Sep 2016, Eric Wong wrote:\n\n> larsxschneider@gmail.com wrote:\n> > All processes that the Git main process spawns inherit the open file\n> > descriptors of the main process. These leaked file descriptors can\n> > cause problems.\n> \n> \n> > -int git_open_noatime(const char *name)\n> > +int git_open_noatime_cloexec(const char *name)\n> > {\n> > -\tstatic int sha1_file_open_flag = O_NOATIME;\n> > +\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n> > \n> > \tfor (;;) {\n> > \t\tint fd;\n> \n> If there's real problems being caused by lack of cloexec\n> today, I think the F_SETFD fallback I proposed in\n> https://public-inbox.org/git/20160818173555.GA29253@starla/\n> will be necessary.\n\nYes, it is good to have that patch available to go if we need it. I do not\nthink that we will need it, though, as the biggest problems that are\nsolved through the CLOEXEC flag are ones caused on Windows, when files\ncannot be deleted or renamed because there are still (uselessly) open\nhandles referencing them.\n\n> I question the need for the \"_cloexec\" suffixing in the\n> function name since the old function is going away entirely.\n\nMe, too. While it is correct, it makes things harder to read, so it may\neven cause more harm than it does good.\n\n> I prefer all FD-creating functions set cloexec by default\n> for FD > 2 to avoid inadvertantly leaking FDs. So we\n> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...\n> and fallback to the racy+slower F_SETFD when not available.\n\nIn the original Pull Request where the change was contributed to Git for\nWindows, this was tested (actually, the code did not see whether fd > 2,\nbut simply assumed that all newly opened file descriptors would be > 2\nanyway), and it failed:\n\nhttps://github.com/git-for-windows/git/pull/755#issuecomment-220247972\n\nSo it appears that we would have to exclude at least the code path to `git\nupload-pack` from that magic.\n\nCiao,\nDscho\n"},{"id":"301182","messageId":"alpine.DEB.2.20.1609061339230.129229@virtualbox","threadId":"44014","inReplyTo":"20160905211111.72956-3-larsxschneider@gmail.com","subject":"Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-09-06T11:41:00Z","receivedAt":"2016-09-06T11:41:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Lars,\n\nOn Mon, 5 Sep 2016, larsxschneider@gmail.com wrote:\n\n> [... commit message ...]\n\nMakes sense.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index 491e52d..02f74d3 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -156,7 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n> {\n> \tint match = -1;\n> -\tint fd = open(ce->name, O_RDONLY);\n> +\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n\nEric's comment on 1/2 applies here, too, of course: should this cause any\nproblems on non-Windows platforms, we always have that FD_CLOEXEC thing\nthat we could probably use to fix it.\n\nBut let's cross that bridge when (or better: if) we get there.\n\nCiao,\nDscho\n"},{"id":"301234","messageId":"20160906210632.GA28263@starla","threadId":"44014","inReplyTo":"20160905211111.72956-3-larsxschneider@gmail.com","subject":"Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-09-06T21:06:32Z","receivedAt":"2016-09-06T21:06:53Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"larsxschneider@gmail.com wrote:\n> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n> {\n> \tint match = -1;\n> -\tint fd = open(ce->name, O_RDONLY);\n> +\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n> \n> \tif (fd >= 0) {\n> \t\tunsigned char sha1[20];\n\nAlso, this needs to check EINVAL when O_CLOEXEC != 0 the same\nway create_tempfile currently does. Somebody could be building\nwith modern headers but running an old kernel that doesn't\nunderstand O_CLOEXEC.\n\nThere should probably be a open() wrapper for handling this case\nsince we're now up to 3 places where open(... O_CLOEXEC) is\nused.\n"},{"id":"301262","messageId":"F8E7B7CE-1177-4CBD-999E-21C593A8ACD2@gmail.com","threadId":"44014","inReplyTo":"alpine.DEB.2.20.1609061333030.129229@virtualbox","subject":"Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-07T13:20:28Z","receivedAt":"2016-09-07T13:20:37Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 06 Sep 2016, at 13:38, Johannes Schindelin wrote:\n> \n> Hi Eric & Lars,\n> \n> On Mon, 5 Sep 2016, Eric Wong wrote:\n> \n>> larsxschneider@gmail.com wrote:\n>>> All processes that the Git main process spawns inherit the open file\n>>> descriptors of the main process. These leaked file descriptors can\n>>> cause problems.\n>> \n>> \n>>> -int git_open_noatime(const char *name)\n>>> +int git_open_noatime_cloexec(const char *name)\n>>> {\n>>> -\tstatic int sha1_file_open_flag = O_NOATIME;\n>>> +\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n>>> \n>>> \tfor (;;) {\n>>> \t\tint fd;\n> \n>> I question the need for the \"_cloexec\" suffixing in the\n>> function name since the old function is going away entirely.\n> \n> Me, too. While it is correct, it makes things harder to read, so it may\n> even cause more harm than it does good.\n\nWhat name would you suggest? Leaving the name as-is seems misleading to me.\nMaybe just \"git_open()\" ?\n\n\n>> I prefer all FD-creating functions set cloexec by default\n>> for FD > 2 to avoid inadvertantly leaking FDs. So we\n>> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...\n>> and fallback to the racy+slower F_SETFD when not available.\n> \n> In the original Pull Request where the change was contributed to Git for\n> Windows, this was tested (actually, the code did not see whether fd > 2,\n> but simply assumed that all newly opened file descriptors would be > 2\n> anyway), and it failed:\n> \n> https://github.com/git-for-windows/git/pull/755#issuecomment-220247972\n> \n> So it appears that we would have to exclude at least the code path to `git\n> upload-pack` from that magic.\n\n\nI just realized that Dscho improved his original patch in GfW with a\nfallback if CLOEXEC is not present.\n\nI applied the same mechanism here. Would that be OK?\n\nThanks,\nLars\n\n- static int sha1_file_open_flag = O_NOATIME;\n+ static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n\n for (;;) {\n int fd;\n@@ -1471,12 +1471,17 @@ int git_open_noatime(const char *name)\n if (fd >= 0)\n return fd;\n\n- /* Might the failure be due to O_NOATIME? */\n- if (errno != ENOENT && sha1_file_open_flag) {\n- sha1_file_open_flag = 0;\n+ /* Try again w/o O_CLOEXEC: the kernel might not support it */\n+ if (O_CLOEXEC && errno == EINVAL && (sha1_file_open_flag & O_CLOEXEC)) {\n+ sha1_file_open_flag &= ~O_CLOEXEC;\n continue;\n }\n\n+ /* Might the failure be due to O_NOATIME? */\n+ if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n+ sha1_file_open_flag &= ~O_NOATIME;\n+ continue;\n+ }\n\n"},{"id":"301263","messageId":"7B903664-0324-4375-A81C-1317020CBE9B@gmail.com","threadId":"44014","inReplyTo":"20160906210632.GA28263@starla","subject":"Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-07T13:39:49Z","receivedAt":"2016-09-07T13:39:57Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 06 Sep 2016, at 23:06, Eric Wong wrote:\n> \n> larsxschneider@gmail.com wrote:\n>> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n>> {\n>> \tint match = -1;\n>> -\tint fd = open(ce->name, O_RDONLY);\n>> +\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n>> \n>> \tif (fd >= 0) {\n>> \t\tunsigned char sha1[20];\n> \n> Also, this needs to check EINVAL when O_CLOEXEC != 0 the same\n> way create_tempfile currently does. Somebody could be building\n> with modern headers but running an old kernel that doesn't\n> understand O_CLOEXEC.\n> \n> There should probably be a open() wrapper for handling this case\n> since we're now up to 3 places where open(... O_CLOEXEC) is\n> used.\n\nRight! Actually \"sha1_file.c:git_open_noatime()\" is already a wrapper, no?\nCan't we use this here? The O_NOATIME flag shouldn't hurt, right?\n\nThanks,\nLars\n\n"},{"id":"301303","messageId":"20160907181036.GA14931@starla","threadId":"44014","inReplyTo":"7B903664-0324-4375-A81C-1317020CBE9B@gmail.com","subject":"Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-09-07T18:10:36Z","receivedAt":"2016-09-07T18:10:41Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Lars Schneider wrote:\n> > On 06 Sep 2016, at 23:06, Eric Wong wrote:\n> > larsxschneider@gmail.com wrote:\n> >> static int ce_compare_data(const struct cache_entry *ce, struct stat *st)\n> >> {\n> >> \tint match = -1;\n> >> -\tint fd = open(ce->name, O_RDONLY);\n> >> +\tint fd = open(ce->name, O_RDONLY | O_CLOEXEC);\n> >> \n> >> \tif (fd >= 0) {\n> >> \t\tunsigned char sha1[20];\n> > \n> > Also, this needs to check EINVAL when O_CLOEXEC != 0 the same\n> > way create_tempfile currently does. Somebody could be building\n> > with modern headers but running an old kernel that doesn't\n> > understand O_CLOEXEC.\n> > \n> > There should probably be a open() wrapper for handling this case\n> > since we're now up to 3 places where open(... O_CLOEXEC) is\n> > used.\n> \n> Right! Actually \"sha1_file.c:git_open_noatime()\" is already a wrapper, no?\n> Can't we use this here? The O_NOATIME flag shouldn't hurt, right?\n\nFor ce_compare_data (and other O_RDONLY users), I guess\ngit_open_noatime is fine; and probably preferable because of\nO_NOATIME.\n\nWe probably should be using O_NOATIME for all O_RDONLY cases\nto get the last bit of performance out (especially since\nnon-modern-Linux systems probably still lack relatime).\n\nHowever, create_tempfile needs O_RDWR|O_CREAT|O_EXCL\nbut I guess we can clean that up in another series.\n"},{"id":"301305","messageId":"20160907181747.GB14931@starla","threadId":"44014","inReplyTo":"F8E7B7CE-1177-4CBD-999E-21C593A8ACD2@gmail.com","subject":"Re: [PATCH v1 1/2] sha1_file: open window into packfiles with CLOEXEC","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-09-07T18:17:47Z","receivedAt":"2016-09-07T18:17:56Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Lars Schneider wrote:\n> > On 06 Sep 2016, at 13:38, Johannes Schindelin wrote:\n> > On Mon, 5 Sep 2016, Eric Wong wrote:\n> >> larsxschneider@gmail.com wrote:\n> >>> -int git_open_noatime(const char *name)\n> >>> +int git_open_noatime_cloexec(const char *name)\n> >>> {\n> >>> -\tstatic int sha1_file_open_flag = O_NOATIME;\n> >>> +\tstatic int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n> >>> \n> >>> \tfor (;;) {\n> >>> \t\tint fd;\n> > \n> >> I question the need for the \"_cloexec\" suffixing in the\n> >> function name since the old function is going away entirely.\n> > \n> > Me, too. While it is correct, it makes things harder to read, so it may\n> > even cause more harm than it does good.\n> \n> What name would you suggest? Leaving the name as-is seems misleading to me.\n> Maybe just \"git_open()\" ?\n\nMaybe \"_noatime\" is useful in some cases, but maybe not *shrug*\n\nMy original point for removing the \"_cloexec\" suffix was that\n(at least for Perl and Ruby), cloexec-by-default was so prevalent\nin FD-creating syscalls that having the suffix wasn't needed.\n\n> >> I prefer all FD-creating functions set cloexec by default\n> >> for FD > 2 to avoid inadvertantly leaking FDs. So we\n> >> ought to use pipe2, accept4, socket(..., SOCK_CLOEXEC), etc...\n> >> and fallback to the racy+slower F_SETFD when not available.\n\n\n> I applied the same mechanism here. Would that be OK?\n> \n> Thanks,\n> Lars\n> \n> - static int sha1_file_open_flag = O_NOATIME;\n> + static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;\n> \n> for (;;) {\n> int fd;\n> @@ -1471,12 +1471,17 @@ int git_open_noatime(const char *name)\n> if (fd >= 0)\n> return fd;\n> \n> - /* Might the failure be due to O_NOATIME? */\n> - if (errno != ENOENT && sha1_file_open_flag) {\n> - sha1_file_open_flag = 0;\n> + /* Try again w/o O_CLOEXEC: the kernel might not support it */\n> + if (O_CLOEXEC && errno == EINVAL && (sha1_file_open_flag & O_CLOEXEC)) {\n\n80 columns overflow\n\n> + sha1_file_open_flag &= ~O_CLOEXEC;\n> continue;\n> }\n> \n> + /* Might the failure be due to O_NOATIME? */\n> + if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {\n> + sha1_file_open_flag &= ~O_NOATIME;\n> + continue;\n> + }\n\nBut otherwise much better since it doesn't blindly zero\nsha1_file_open_flag :>\n"},{"id":"301306","messageId":"xmqqtwdrmuvo.fsf@gitster.mtv.corp.google.com","threadId":"44014","inReplyTo":"20160907181036.GA14931@starla","subject":"Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T18:23:39Z","receivedAt":"2016-09-07T18:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> We probably should be using O_NOATIME for all O_RDONLY cases\n> to get the last bit of performance out (especially since\n> non-modern-Linux systems probably still lack relatime).\n\nNo, please do not go there.\n\nThe user can read from a file in a working tree using \"less\",\n\"grep\", etc., and they all update the atime, so should \"git grep\".\nWe do not use atime ourselves on these files but we should let\noutside tools rely on the validity of atime (e.g. \"what are the\nfiles that were looked at yesterday?\").\n\nIf you grep for noatime in our current codebase, you'd notice that\nwe use it only for files in objects/ hierarchy, and that makes very\ngood sense. These files are what we create for our _sole_ use and\nno other tools can peek at them and expect to get any useful\ninformation out of them (we hear from time to time that virus\nscanners leaving open file descriptors on them causing trouble, but\nthat is an example of a useless access), and that makes a file in\nobjects/ hierarchy a fair game for noatime optimization.\n"},{"id":"301363","messageId":"F33245FC-C53A-4977-8E72-68AF3D2BB8BB@gmail.com","threadId":"44014","inReplyTo":"xmqqtwdrmuvo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-09-08T05:57:39Z","receivedAt":"2016-09-08T06:17:12Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 07 Sep 2016, at 20:23, Junio C Hamano wrote:\n> \n> Eric Wong writes:\n> \n>> We probably should be using O_NOATIME for all O_RDONLY cases\n>> to get the last bit of performance out (especially since\n>> non-modern-Linux systems probably still lack relatime).\n> \n> No, please do not go there.\n> \n> The user can read from a file in a working tree using \"less\",\n> \"grep\", etc., and they all update the atime, so should \"git grep\".\n> We do not use atime ourselves on these files but we should let\n> outside tools rely on the validity of atime (e.g. \"what are the\n> files that were looked at yesterday?\").\n> \n> If you grep for noatime in our current codebase, you'd notice that\n> we use it only for files in objects/ hierarchy, and that makes very\n> good sense. These files are what we create for our _sole_ use and\n> no other tools can peek at them and expect to get any useful\n> information out of them (we hear from time to time that virus\n> scanners leaving open file descriptors on them causing trouble, but\n> that is an example of a useless access), and that makes a file in\n> objects/ hierarchy a fair game for noatime optimization.\n\nHow do we deal with read-cache:ce_compare_data, though?\n\nBy your definition above we shouldn't use NOATIME since the read file\nis not in objects/. However, the file read is not something the user\nexplicitly triggers. The read is part of the Git internal \"clean\"\nmachinery.\n\nWhat would you suggest? Should I open the file with or without NOATIME?\n\nThanks,\nLars\n"},{"id":"301412","messageId":"xmqqbmzyi972.fsf@gitster.mtv.corp.google.com","threadId":"44014","inReplyTo":"F33245FC-C53A-4977-8E72-68AF3D2BB8BB@gmail.com","subject":"Re: [PATCH v1 2/2] read-cache: make sure file handles are not inherited by child processes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-08T17:37:53Z","receivedAt":"2016-09-08T17:38:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>>> We probably should be using O_NOATIME for all O_RDONLY cases\n>>> to get the last bit of performance out (especially since\n>>> non-modern-Linux systems probably still lack relatime).\n>> \n>> No, please do not go there.\n>> \n>> The user can read from a file in a working tree using \"less\",\n>> \"grep\", etc., and they all update the atime, so should \"git grep\".\n>> We do not use atime ourselves on these files but we should let\n>> outside tools rely on the validity of atime (e.g. \"what are the\n>> files that were looked at yesterday?\").\n>> \n>> If you grep for noatime in our current codebase, you'd notice that\n>> we use it only for files in objects/ hierarchy, and that makes very\n>> good sense. These files are what we create for our _sole_ use and\n>> no other tools can peek at them and expect to get any useful\n>> information out of them (we hear from time to time that virus\n>> scanners leaving open file descriptors on them causing trouble, but\n>> that is an example of a useless access), and that makes a file in\n>> objects/ hierarchy a fair game for noatime optimization.\n>\n> How do we deal with read-cache:ce_compare_data, though?\n\nWe are using open() in our current code in that codepath, without\nNOATIME. We shouldn't start using git_open_noatime() merely for\nconvenience.\n\nWe would probably want to do a preliminary refactoring so that we\ncan say \"we want only CLOEXEC and not NOATIME\". Perhaps we would\nwant to make the existing one in sha1_file.c to something like:\n\n\tint git_open_noatime(const char *name)\n {\n\t\treturn git_open(name, GIT_OPEN_NOATIME);\n\t}\n\nand then add\n\n\t#define GIT_OPEN_NOATIME 01\n\nto cache.h and add\n\n\tint git_open(const char *name, unsigned int flag)\n {\n \tstatic int open_noatime = O_NOATIME;\n\n\t\tfor (;;) {\n \tint fd, open_flag;\n\n open_flag = 0;\n if (flag & GIT_OPEN_NOATIME)\n open_flag |= open_noatime;\n\n\t\t\terrno = 0;\n fd = open(name, O_RDONLY | open_flag);\n if (fd >= 0)\n \treturn fd;\n\n\t\t\tif (errno == ENOENT || !open_flag)\n\t\t\t\treturn -1;\n\n\t\t\t/* The failure may be due to additional\tflags */\n \tif ((flag & GIT_OPEN_NOATIME) &&\n\t\t\t (open_flag & O_NOATIME)) {\n\t\t\t\tflag &= ~GIT_OPEN_NOATIME;\n\t\t\t\topen_noatime = 0;\n\t\t\t}\n\t\t}\n\t}\n\nto wrapper.c in the first step, which is a \"no-op refactoring\" step.\n\nThen add\n\n\t#define GIT_OPEN_CLOEXEC 02\n\nand update git_open(), perhaps like so:\n\n\tint git_open(const char *name, unsigned int flag)\n {\n \tstatic int open_noatime = O_NOATIME;\n \tstatic int open_cloexec = O_CLOEXEC;\n\n\t\tfor (;;) {\n \tint fd, open_flag;\n\n open_flag = 0;\n if (flag & GIT_OPEN_NOATIME)\n open_flag |= open_noatime;\n if (flag & GIT_OPEN_CLOEXEC)\n open_flag |= open_cloexec;\n\n\t\t\terrno = 0;\n fd = open(name, O_RDONLY | open_flag);\n if (fd >= 0)\n \treturn fd;\n\n\t\t\tif (errno == ENOENT || !open_flag)\n\t\t\t\treturn -1;\n\n\t\t\t/* The failure may be due to additional\tflags */\n \tif ((flag & GIT_OPEN_NOATIME) &&\n\t\t\t (open_flag & O_NOATIME)) {\n\t\t\t\tflag &= ~GIT_OPEN_NOATIME;\n open_noatime = 0;\n\t\t\t}\n \tif ((flag & GIT_OPEN_CLOEXEC) &&\n\t\t\t (open_flag & O_CLOEXEC)) {\n\t\t\t\tflag &= ~GIT_OPEN_CLOEXEC;\n open_cloexec = 0;\n\t\t\t}\n\t\t}\n\t}\n\nThe retry logic is \"if we were asked to do this flag, and if we did\npass that flag, then we know open() with that flag fails here, so we\nwon't waste time trying with it again\", which came from the NOATIME\ncodepath we already have, but it may not match what we use CLOEXEC\nfor and may need to be adjusted. I didn't think that part of the\ncode through.\n\nThen the ce codepath that reads from the working tree would use\n\n\tgit_open(ce->name, GIT_OPEN_CLOEXEC);\n\nto obtain the file descriptor for reading, perhaps?\n"}]}