{"thread":{"id":"50519","subject":"[PATCH 1/2] worktree: fix worktree add race.","startedAt":"2019-02-15T18:16:25Z","lastAt":"2019-02-18T08:54:27Z","messageCount":7,"participants":["Michal Suchanek","Junio C Hamano","Michal Suchánek","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"369404","messageId":"429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek@suse.de","threadId":"50519","inReplyTo":null,"subject":"[PATCH 1/2] worktree: fix worktree add race.","fromName":"Michal Suchanek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-15T18:16:11Z","receivedAt":"2019-02-15T18:16:25Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"Git runs a stat loop to find a worktree name that's available and then does\nmkdir on the found name. Turn it to mkdir loop to avoid another invocation of\nworktree add finding the same free name and creating the directory first.\n\nSigned-off-by: Michal Suchanek \n---\n builtin/worktree.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 3f9907fcc994..e1a2a56c03c5 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -268,10 +268,9 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *name;\n-\tstruct stat st;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct argv_array child_env = ARGV_ARRAY_INIT;\n-\tint counter = 0, len, ret;\n+\tint counter = 1, len, ret;\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n@@ -295,19 +294,21 @@ static int add_worktree(const char *path, const char *refname,\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n \t\t\t sb_repo.buf);\n-\twhile (!stat(sb_repo.buf, &st)) {\n+\n+\twhile (mkdir(sb_repo.buf, 0777)) {\n \t\tcounter++;\n+\t\tif(!counter) break; /* don't loop forever */\n \t\tstrbuf_setlen(&sb_repo, len);\n \t\tstrbuf_addf(&sb_repo, \"%d\", counter);\n \t}\n+\tif (!counter)\n+\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n \tname = strrchr(sb_repo.buf, '/') + 1;\n \n \tjunk_pid = getpid();\n \tatexit(remove_junk);\n \tsigchain_push_common(remove_junk_on_signal);\n \n-\tif (mkdir(sb_repo.buf, 0777))\n-\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n \tjunk_git_dir = xstrdup(sb_repo.buf);\n \tis_junk = 1;\n \n-- \n2.20.1\n\n"},{"id":"369405","messageId":"dd42ab7054077c7d29ad62e7481c00ab5c1bf864.1550254374.git.msuchanek@suse.de","threadId":"50519","inReplyTo":"429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek@suse.de","subject":"[PATCH 2/2] setup: don't fail if commondir is deleted.","fromName":"Michal Suchanek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-15T18:16:12Z","receivedAt":"2019-02-15T18:16:26Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"When adding wotktrees git can die in get_common_dir_noenv while\nexamining existing worktrees because the commondir file does not exist.\nHandle ENOENT so adding a worktree does not fail because of incompletely\nset-up other worktree.\n\nSigned-off-by: Michal Suchanek \n---\n setup.c | 33 ++++++++++++++++++---------------\n 1 file changed, 18 insertions(+), 15 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex ca9e8a949ed8..7dec2e5589d9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n \n \tstrbuf_addf(&path, \"%s/commondir\", gitdir);\n \tif (file_exists(path.buf)) {\n-\t\tif (strbuf_read_file(&data, path.buf, 0) <= 0)\n-\t\t\tdie_errno(_(\"failed to read %s\"), path.buf);\n-\t\twhile (data.len && (data.buf[data.len - 1] == '\\n' ||\n-\t\t\t\t data.buf[data.len - 1] == '\\r'))\n-\t\t\tdata.len--;\n-\t\tdata.buf[data.len] = '\\0';\n-\t\tstrbuf_reset(&path);\n-\t\tif (!is_absolute_path(data.buf))\n-\t\t\tstrbuf_addf(&path, \"%s/\", gitdir);\n-\t\tstrbuf_addbuf(&path, &data);\n-\t\tstrbuf_add_real_path(sb, path.buf);\n-\t\tret = 1;\n-\t} else {\n-\t\tstrbuf_addstr(sb, gitdir);\n+\t\tif (strbuf_read_file(&data, path.buf, 0) <= 0) {\n+\t\t\tif (errno != ENOENT)\n+\t\t\t\tdie_errno(_(\"failed to read %s\"), path.buf);\n+\t\t} else {\n+\t\t\twhile (data.len && (data.buf[data.len - 1] == '\\n' ||\n+\t\t\t\t\t\tdata.buf[data.len - 1] == '\\r'))\n+\t\t\t\tdata.len--;\n+\t\t\tdata.buf[data.len] = '\\0';\n+\t\t\tstrbuf_reset(&path);\n+\t\t\tif (!is_absolute_path(data.buf))\n+\t\t\t\tstrbuf_addf(&path, \"%s/\", gitdir);\n+\t\t\tstrbuf_addbuf(&path, &data);\n+\t\t\tstrbuf_add_real_path(sb, path.buf);\n+\t\t\tret = 1;\n+\t\t\tgoto out;\n+\t\t}\n \t}\n-\n+\tstrbuf_addstr(sb, gitdir);\n+out:\n \tstrbuf_release(&data);\n \tstrbuf_release(&path);\n \treturn ret;\n-- \n2.20.1\n\n"},{"id":"369411","messageId":"xmqq8syg6foq.fsf@gitster-ct.c.googlers.com","threadId":"50519","inReplyTo":"429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek@suse.de","subject":"Re: [PATCH 1/2] worktree: fix worktree add race.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-15T18:59:33Z","receivedAt":"2019-02-15T18:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michal Suchanek writes:\n\n> Git runs a stat loop to find a worktree name that's available and then does\n> mkdir on the found name. Turn it to mkdir loop to avoid another invocation of\n> worktree add finding the same free name and creating the directory first.\n\nYeah, relying on the atomicity of mkdir(2) is much saner approach\nthan \"check -- ah we can use the name -- try to create\" that is race\nprone.\n\nThanks for working on this.\n\n> Signed-off-by: Michal Suchanek \n> ---\n> builtin/worktree.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 3f9907fcc994..e1a2a56c03c5 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -268,10 +268,9 @@ static int add_worktree(const char *path, const char *refname,\n> \tstruct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tconst char *name;\n> -\tstruct stat st;\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tstruct argv_array child_env = ARGV_ARRAY_INIT;\n> -\tint counter = 0, len, ret;\n> +\tint counter = 1, len, ret;\n> \tstruct strbuf symref = STRBUF_INIT;\n> \tstruct commit *commit = NULL;\n> \tint is_branch = 0;\n> @@ -295,19 +294,21 @@ static int add_worktree(const char *path, const char *refname,\n> \tif (safe_create_leading_directories_const(sb_repo.buf))\n> \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n> \t\t\t sb_repo.buf);\n> -\twhile (!stat(sb_repo.buf, &st)) {\n> +\n> +\twhile (mkdir(sb_repo.buf, 0777)) {\n> \t\tcounter++;\n> +\t\tif(!counter) break; /* don't loop forever */\n> \t\tstrbuf_setlen(&sb_repo, len);\n> \t\tstrbuf_addf(&sb_repo, \"%d\", counter);\n\nStyle:\n\n\t\tif (!counter)\n\t\t\tbreak; /* don't loop forever */\n\nMore importantly, how long would it take to loop thru all possible\nintegers (can be simulated by making the parent directory\nunwritable)? Don't we want to cut off with more conservative upper\nlimit, say 1000 rounds or even 100 rounds or so?\n\nAlso, is the behaviour for a signed integer wrapping around due to\ngetting incremented too many times well defined? I'd feel safer,\nespecially if you are willing to spin for 4 billion times like this\npatch does, if you changed the counter to \"unsigned int\".\n\nI see you changed \"counter\" to start from 1, but that would mean\nthat these fallback names would start with suffix 2, not 1. Which\nwould look funny.\n\nI would have expected \".1\", \".2\", etc. as suffix, but the original\nused \"1\", \"2\", etc. so I won't complain on the format, but I do find\nit questionable to start counting from 2.\n\n> \t}\n> +\tif (!counter)\n> +\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n\nIt would have saved reviewer's time if this die() were inside the\nloop where you punted with \"break\".\n\n> \tname = strrchr(sb_repo.buf, '/') + 1;\n> \n> \tjunk_pid = getpid();\n> \tatexit(remove_junk);\n> \tsigchain_push_common(remove_junk_on_signal);\n> \n> -\tif (mkdir(sb_repo.buf, 0777))\n> -\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n> \tjunk_git_dir = xstrdup(sb_repo.buf);\n> \tis_junk = 1;\n"},{"id":"369424","messageId":"20190216011843.24d15051@naga","threadId":"50519","inReplyTo":"xmqq8syg6foq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] worktree: fix worktree add race.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-16T00:18:43Z","receivedAt":"2019-02-16T00:18:49Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Fri, 15 Feb 2019 10:59:33 -0800\nJunio C Hamano wrote:\n\n> Michal Suchanek writes:\n> \n> > Git runs a stat loop to find a worktree name that's available and then does\n> > mkdir on the found name. Turn it to mkdir loop to avoid another invocation of\n> > worktree add finding the same free name and creating the directory first. \n> \n> Yeah, relying on the atomicity of mkdir(2) is much saner approach\n> than \"check -- ah we can use the name -- try to create\" that is race\n> prone.\n> \n> Thanks for working on this.\n> \n> > Signed-off-by: Michal Suchanek \n> > ---\n> > builtin/worktree.c | 11 ++++++-----\n> > 1 file changed, 6 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > index 3f9907fcc994..e1a2a56c03c5 100644\n> > --- a/builtin/worktree.c\n> > +++ b/builtin/worktree.c\n> > @@ -268,10 +268,9 @@ static int add_worktree(const char *path, const char *refname,\n> > \tstruct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n> > \tstruct strbuf sb = STRBUF_INIT;\n> > \tconst char *name;\n> > -\tstruct stat st;\n> > \tstruct child_process cp = CHILD_PROCESS_INIT;\n> > \tstruct argv_array child_env = ARGV_ARRAY_INIT;\n> > -\tint counter = 0, len, ret;\n> > +\tint counter = 1, len, ret;\n> > \tstruct strbuf symref = STRBUF_INIT;\n> > \tstruct commit *commit = NULL;\n> > \tint is_branch = 0;\n> > @@ -295,19 +294,21 @@ static int add_worktree(const char *path, const char *refname,\n> > \tif (safe_create_leading_directories_const(sb_repo.buf))\n> > \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n> > \t\t\t sb_repo.buf);\n> > -\twhile (!stat(sb_repo.buf, &st)) {\n> > +\n> > +\twhile (mkdir(sb_repo.buf, 0777)) {\n> > \t\tcounter++;\n> > +\t\tif(!counter) break; /* don't loop forever */\n> > \t\tstrbuf_setlen(&sb_repo, len);\n> > \t\tstrbuf_addf(&sb_repo, \"%d\", counter); \n> \n> Style:\n> \n> \t\tif (!counter)\n> \t\t\tbreak; /* don't loop forever */\n> \n> More importantly, how long would it take to loop thru all possible\n> integers (can be simulated by making the parent directory\n> unwritable)? Don't we want to cut off with more conservative upper\n> limit, say 1000 rounds or even 100 rounds or so?\n> \n> Also, is the behaviour for a signed integer wrapping around due to\n> getting incremented too many times well defined? I'd feel safer,\n> especially if you are willing to spin for 4 billion times like this\n> patch does, if you changed the counter to \"unsigned int\".\n\nIf there are 4 billion worktrees ..\nbut there is no need to spin if the failure reason is not EEXIST.\n\n> \n> I see you changed \"counter\" to start from 1, but that would mean\n> that these fallback names would start with suffix 2, not 1. Which\n> would look funny.\n> \n> I would have expected \".1\", \".2\", etc. as suffix, but the original\n> used \"1\", \"2\", etc. so I won't complain on the format, but I do find\n> it questionable to start counting from 2.\n\nYes, there is no need to change the starting counter.\n\n> \n> > \t}\n> > +\tif (!counter)\n> > +\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf); \n> \n> It would have saved reviewer's time if this die() were inside the\n> loop where you punted with \"break\".\n\nYes, that's a leftover of the existing code structure.\n\n> \n> > \tname = strrchr(sb_repo.buf, '/') + 1;\n> > \n> > \tjunk_pid = getpid();\n> > \tatexit(remove_junk);\n> > \tsigchain_push_common(remove_junk_on_signal);\n> > \n> > -\tif (mkdir(sb_repo.buf, 0777))\n> > -\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n> > \tjunk_git_dir = xstrdup(sb_repo.buf);\n> > \tis_junk = 1; \n\nThanks\n\nMichal\n"},{"id":"369476","messageId":"CAPig+cQCw-7WALdzdFD--Jt5d+iZbY8DBBQdC_P8wZ9CVTYZKA@mail.gmail.com","threadId":"50519","inReplyTo":"xmqq8syg6foq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] worktree: fix worktree add race.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-17T07:05:11Z","receivedAt":"2019-02-17T07:05:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 15, 2019 at 1:59 PM Junio C Hamano wrote:\n> Michal Suchanek writes:\n> > Git runs a stat loop to find a worktree name that's available and then does\n> > mkdir on the found name. Turn it to mkdir loop to avoid another invocation of\n> > worktree add finding the same free name and creating the directory first.\n>\n> Yeah, relying on the atomicity of mkdir(2) is much saner approach\n> than \"check -- ah we can use the name -- try to create\" that is race\n> prone.\n\nYep. When you re-roll, please mention [1] in the cover letter or in\nthe commentary section of this patch (after the \"---\" line below your\nsign-off) to provide context to reviewers. Also, when re-rolling,\nplease Cc: people involved in that earlier discussion (whom I've added\nto the Cc: of this reply). Thanks.\n\n[1]: http://public-inbox.org/git/CAPig+cSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24=ZwbuQtsaLNZw@mail.gmail.com/\n"},{"id":"369477","messageId":"CAPig+cTS0MNvwx67ZVbYUvi9bA63KWnNnSmWdFjLuqc_SNAy8A@mail.gmail.com","threadId":"50519","inReplyTo":"dd42ab7054077c7d29ad62e7481c00ab5c1bf864.1550254374.git.msuchanek@suse.de","subject":"Re: [PATCH 2/2] setup: don't fail if commondir is deleted.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-17T07:14:14Z","receivedAt":"2019-02-17T07:14:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 15, 2019 at 1:16 PM Michal Suchanek wrote:\n> When adding wotktrees git can die in get_common_dir_noenv while\n> examining existing worktrees because the commondir file does not exist.\n> Handle ENOENT so adding a worktree does not fail because of incompletely\n> set-up other worktree.\n>\n> Signed-off-by: Michal Suchanek \n> ---\n> diff --git a/setup.c b/setup.c\n> @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> + if (strbuf_read_file(&data, path.buf, 0) <= 0) {\n> + if (errno != ENOENT)\n> + die_errno(_(\"failed to read %s\"), path.buf);\n\nDocumentation for strbuf_read_file() in strbuf.h does not state that\n'errno' has any meaningful value when this function fails, however,\nthe actual implementation in strbuf.c is careful to preserve 'errno'\nwhen something goes wrong. Therefore, it is safe to consult 'errno'\nhere. Fine.\n\nIt might be a good idea to fix the documentation of strbuf_read_file()\nto mention 'errno' explicitly, as a preparatory patch of this series\n(though not at all a requirement when re-rolling; just a suggestion to\nassist reviewers).\n"},{"id":"369521","messageId":"20190218095423.0705aa7a@naga","threadId":"50519","inReplyTo":"CAPig+cTS0MNvwx67ZVbYUvi9bA63KWnNnSmWdFjLuqc_SNAy8A@mail.gmail.com","subject":"Re: [PATCH 2/2] setup: don't fail if commondir is deleted.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-18T08:54:23Z","receivedAt":"2019-02-18T08:54:27Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Sun, 17 Feb 2019 02:14:14 -0500\nEric Sunshine wrote:\n\n> On Fri, Feb 15, 2019 at 1:16 PM Michal Suchanek wrote:\n> > When adding wotktrees git can die in get_common_dir_noenv while\n> > examining existing worktrees because the commondir file does not exist.\n> > Handle ENOENT so adding a worktree does not fail because of incompletely\n> > set-up other worktree.\n> >\n> > Signed-off-by: Michal Suchanek \n> > ---\n> > diff --git a/setup.c b/setup.c\n> > @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> > + if (strbuf_read_file(&data, path.buf, 0) <= 0) {\n> > + if (errno != ENOENT)\n> > + die_errno(_(\"failed to read %s\"), path.buf); \n> \n> Documentation for strbuf_read_file() in strbuf.h does not state that\n> 'errno' has any meaningful value when this function fails, however,\n\nIt is stated in the documentation of strbuf_read().\n\nThanks\n\nMichal\n"}]}