# [PATCH 1/2] worktree: fix worktree add race. 7 messages from 2019-02-15 to 2019-02-18. Participants: Michal Suchanek, Junio C Hamano, Michal Suchánek, Eric Sunshine. Thread: https://gitlist.dev/t/50519 ## Michal Suchanek, 2019-02-15 18:16 Subject: [PATCH 1/2] worktree: fix worktree add race. Message-ID: <429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek@suse.de> URL: https://gitlist.dev/e/429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek%40suse.de ``` Git runs a stat loop to find a worktree name that's available and then does mkdir on the found name. Turn it to mkdir loop to avoid another invocation of worktree add finding the same free name and creating the directory first. Signed-off-by: Michal Suchanek --- builtin/worktree.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index 3f9907fcc994..e1a2a56c03c5 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -268,10 +268,9 @@ static int add_worktree(const char *path, const char *refname, struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT; struct strbuf sb = STRBUF_INIT; const char *name; - struct stat st; struct child_process cp = CHILD_PROCESS_INIT; struct argv_array child_env = ARGV_ARRAY_INIT; - int counter = 0, len, ret; + int counter = 1, len, ret; struct strbuf symref = STRBUF_INIT; struct commit *commit = NULL; int is_branch = 0; @@ -295,19 +294,21 @@ static int add_worktree(const char *path, const char *refname, if (safe_create_leading_directories_const(sb_repo.buf)) die_errno(_("could not create leading directories of '%s'"), sb_repo.buf); - while (!stat(sb_repo.buf, &st)) { + + while (mkdir(sb_repo.buf, 0777)) { counter++; + if(!counter) break; /* don't loop forever */ strbuf_setlen(&sb_repo, len); strbuf_addf(&sb_repo, "%d", counter); } + if (!counter) + die_errno(_("could not create directory of '%s'"), sb_repo.buf); name = strrchr(sb_repo.buf, '/') + 1; junk_pid = getpid(); atexit(remove_junk); sigchain_push_common(remove_junk_on_signal); - if (mkdir(sb_repo.buf, 0777)) - die_errno(_("could not create directory of '%s'"), sb_repo.buf); junk_git_dir = xstrdup(sb_repo.buf); is_junk = 1; -- 2.20.1 ``` ## Michal Suchanek, 2019-02-15 18:16 Subject: [PATCH 2/2] setup: don't fail if commondir is deleted. Message-ID: URL: https://gitlist.dev/e/dd42ab7054077c7d29ad62e7481c00ab5c1bf864.1550254374.git.msuchanek%40suse.de In-Reply-To: <429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek@suse.de> ``` When adding wotktrees git can die in get_common_dir_noenv while examining existing worktrees because the commondir file does not exist. Handle ENOENT so adding a worktree does not fail because of incompletely set-up other worktree. Signed-off-by: Michal Suchanek --- setup.c | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/setup.c b/setup.c index ca9e8a949ed8..7dec2e5589d9 100644 --- a/setup.c +++ b/setup.c @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) strbuf_addf(&path, "%s/commondir", gitdir); if (file_exists(path.buf)) { - if (strbuf_read_file(&data, path.buf, 0) <= 0) - die_errno(_("failed to read %s"), path.buf); - while (data.len && (data.buf[data.len - 1] == '\n' || - data.buf[data.len - 1] == '\r')) - data.len--; - data.buf[data.len] = '\0'; - strbuf_reset(&path); - if (!is_absolute_path(data.buf)) - strbuf_addf(&path, "%s/", gitdir); - strbuf_addbuf(&path, &data); - strbuf_add_real_path(sb, path.buf); - ret = 1; - } else { - strbuf_addstr(sb, gitdir); + if (strbuf_read_file(&data, path.buf, 0) <= 0) { + if (errno != ENOENT) + die_errno(_("failed to read %s"), path.buf); + } else { + while (data.len && (data.buf[data.len - 1] == '\n' || + data.buf[data.len - 1] == '\r')) + data.len--; + data.buf[data.len] = '\0'; + strbuf_reset(&path); + if (!is_absolute_path(data.buf)) + strbuf_addf(&path, "%s/", gitdir); + strbuf_addbuf(&path, &data); + strbuf_add_real_path(sb, path.buf); + ret = 1; + goto out; + } } - + strbuf_addstr(sb, gitdir); +out: strbuf_release(&data); strbuf_release(&path); return ret; -- 2.20.1 ``` ## Junio C Hamano, 2019-02-15 18:59 Subject: Re: [PATCH 1/2] worktree: fix worktree add race. Message-ID: URL: https://gitlist.dev/e/xmqq8syg6foq.fsf%40gitster-ct.c.googlers.com In-Reply-To: <429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek@suse.de> ``` Michal Suchanek writes: > Git runs a stat loop to find a worktree name that's available and then does > mkdir on the found name. Turn it to mkdir loop to avoid another invocation of > worktree add finding the same free name and creating the directory first. Yeah, relying on the atomicity of mkdir(2) is much saner approach than "check -- ah we can use the name -- try to create" that is race prone. Thanks for working on this. > Signed-off-by: Michal Suchanek > --- > builtin/worktree.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 3f9907fcc994..e1a2a56c03c5 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -268,10 +268,9 @@ static int add_worktree(const char *path, const char *refname, > struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT; > struct strbuf sb = STRBUF_INIT; > const char *name; > - struct stat st; > struct child_process cp = CHILD_PROCESS_INIT; > struct argv_array child_env = ARGV_ARRAY_INIT; > - int counter = 0, len, ret; > + int counter = 1, len, ret; > struct strbuf symref = STRBUF_INIT; > struct commit *commit = NULL; > int is_branch = 0; > @@ -295,19 +294,21 @@ static int add_worktree(const char *path, const char *refname, > if (safe_create_leading_directories_const(sb_repo.buf)) > die_errno(_("could not create leading directories of '%s'"), > sb_repo.buf); > - while (!stat(sb_repo.buf, &st)) { > + > + while (mkdir(sb_repo.buf, 0777)) { > counter++; > + if(!counter) break; /* don't loop forever */ > strbuf_setlen(&sb_repo, len); > strbuf_addf(&sb_repo, "%d", counter); Style: if (!counter) break; /* don't loop forever */ More importantly, how long would it take to loop thru all possible integers (can be simulated by making the parent directory unwritable)? Don't we want to cut off with more conservative upper limit, say 1000 rounds or even 100 rounds or so? Also, is the behaviour for a signed integer wrapping around due to getting incremented too many times well defined? I'd feel safer, especially if you are willing to spin for 4 billion times like this patch does, if you changed the counter to "unsigned int". I see you changed "counter" to start from 1, but that would mean that these fallback names would start with suffix 2, not 1. Which would look funny. I would have expected ".1", ".2", etc. as suffix, but the original used "1", "2", etc. so I won't complain on the format, but I do find it questionable to start counting from 2. > } > + if (!counter) > + die_errno(_("could not create directory of '%s'"), sb_repo.buf); It would have saved reviewer's time if this die() were inside the loop where you punted with "break". > name = strrchr(sb_repo.buf, '/') + 1; > > junk_pid = getpid(); > atexit(remove_junk); > sigchain_push_common(remove_junk_on_signal); > > - if (mkdir(sb_repo.buf, 0777)) > - die_errno(_("could not create directory of '%s'"), sb_repo.buf); > junk_git_dir = xstrdup(sb_repo.buf); > is_junk = 1; ``` ## Michal Suchánek, 2019-02-16 00:18 Subject: Re: [PATCH 1/2] worktree: fix worktree add race. Message-ID: <20190216011843.24d15051@naga> URL: https://gitlist.dev/e/20190216011843.24d15051%40naga In-Reply-To: ``` On Fri, 15 Feb 2019 10:59:33 -0800 Junio C Hamano wrote: > Michal Suchanek writes: > > > Git runs a stat loop to find a worktree name that's available and then does > > mkdir on the found name. Turn it to mkdir loop to avoid another invocation of > > worktree add finding the same free name and creating the directory first. > > Yeah, relying on the atomicity of mkdir(2) is much saner approach > than "check -- ah we can use the name -- try to create" that is race > prone. > > Thanks for working on this. > > > Signed-off-by: Michal Suchanek > > --- > > builtin/worktree.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > index 3f9907fcc994..e1a2a56c03c5 100644 > > --- a/builtin/worktree.c > > +++ b/builtin/worktree.c > > @@ -268,10 +268,9 @@ static int add_worktree(const char *path, const char *refname, > > struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT; > > struct strbuf sb = STRBUF_INIT; > > const char *name; > > - struct stat st; > > struct child_process cp = CHILD_PROCESS_INIT; > > struct argv_array child_env = ARGV_ARRAY_INIT; > > - int counter = 0, len, ret; > > + int counter = 1, len, ret; > > struct strbuf symref = STRBUF_INIT; > > struct commit *commit = NULL; > > int is_branch = 0; > > @@ -295,19 +294,21 @@ static int add_worktree(const char *path, const char *refname, > > if (safe_create_leading_directories_const(sb_repo.buf)) > > die_errno(_("could not create leading directories of '%s'"), > > sb_repo.buf); > > - while (!stat(sb_repo.buf, &st)) { > > + > > + while (mkdir(sb_repo.buf, 0777)) { > > counter++; > > + if(!counter) break; /* don't loop forever */ > > strbuf_setlen(&sb_repo, len); > > strbuf_addf(&sb_repo, "%d", counter); > > Style: > > if (!counter) > break; /* don't loop forever */ > > More importantly, how long would it take to loop thru all possible > integers (can be simulated by making the parent directory > unwritable)? Don't we want to cut off with more conservative upper > limit, say 1000 rounds or even 100 rounds or so? > > Also, is the behaviour for a signed integer wrapping around due to > getting incremented too many times well defined? I'd feel safer, > especially if you are willing to spin for 4 billion times like this > patch does, if you changed the counter to "unsigned int". If there are 4 billion worktrees .. but there is no need to spin if the failure reason is not EEXIST. > > I see you changed "counter" to start from 1, but that would mean > that these fallback names would start with suffix 2, not 1. Which > would look funny. > > I would have expected ".1", ".2", etc. as suffix, but the original > used "1", "2", etc. so I won't complain on the format, but I do find > it questionable to start counting from 2. Yes, there is no need to change the starting counter. > > > } > > + if (!counter) > > + die_errno(_("could not create directory of '%s'"), sb_repo.buf); > > It would have saved reviewer's time if this die() were inside the > loop where you punted with "break". Yes, that's a leftover of the existing code structure. > > > name = strrchr(sb_repo.buf, '/') + 1; > > > > junk_pid = getpid(); > > atexit(remove_junk); > > sigchain_push_common(remove_junk_on_signal); > > > > - if (mkdir(sb_repo.buf, 0777)) > > - die_errno(_("could not create directory of '%s'"), sb_repo.buf); > > junk_git_dir = xstrdup(sb_repo.buf); > > is_junk = 1; Thanks Michal ``` ## Eric Sunshine, 2019-02-17 07:05 Subject: Re: [PATCH 1/2] worktree: fix worktree add race. Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQCw-7WALdzdFD--Jt5d%2BiZbY8DBBQdC_P8wZ9CVTYZKA%40mail.gmail.com In-Reply-To: ``` On Fri, Feb 15, 2019 at 1:59 PM Junio C Hamano wrote: > Michal Suchanek writes: > > Git runs a stat loop to find a worktree name that's available and then does > > mkdir on the found name. Turn it to mkdir loop to avoid another invocation of > > worktree add finding the same free name and creating the directory first. > > Yeah, relying on the atomicity of mkdir(2) is much saner approach > than "check -- ah we can use the name -- try to create" that is race > prone. Yep. When you re-roll, please mention [1] in the cover letter or in the commentary section of this patch (after the "---" line below your sign-off) to provide context to reviewers. Also, when re-rolling, please Cc: people involved in that earlier discussion (whom I've added to the Cc: of this reply). Thanks. [1]: http://public-inbox.org/git/CAPig+cSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24=ZwbuQtsaLNZw@mail.gmail.com/ ``` ## Eric Sunshine, 2019-02-17 07:14 Subject: Re: [PATCH 2/2] setup: don't fail if commondir is deleted. Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTS0MNvwx67ZVbYUvi9bA63KWnNnSmWdFjLuqc_SNAy8A%40mail.gmail.com In-Reply-To: ``` On Fri, Feb 15, 2019 at 1:16 PM Michal Suchanek wrote: > When adding wotktrees git can die in get_common_dir_noenv while > examining existing worktrees because the commondir file does not exist. > Handle ENOENT so adding a worktree does not fail because of incompletely > set-up other worktree. > > Signed-off-by: Michal Suchanek > --- > diff --git a/setup.c b/setup.c > @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) > + if (strbuf_read_file(&data, path.buf, 0) <= 0) { > + if (errno != ENOENT) > + die_errno(_("failed to read %s"), path.buf); Documentation for strbuf_read_file() in strbuf.h does not state that 'errno' has any meaningful value when this function fails, however, the actual implementation in strbuf.c is careful to preserve 'errno' when something goes wrong. Therefore, it is safe to consult 'errno' here. Fine. It might be a good idea to fix the documentation of strbuf_read_file() to mention 'errno' explicitly, as a preparatory patch of this series (though not at all a requirement when re-rolling; just a suggestion to assist reviewers). ``` ## Michal Suchánek, 2019-02-18 08:54 Subject: Re: [PATCH 2/2] setup: don't fail if commondir is deleted. Message-ID: <20190218095423.0705aa7a@naga> URL: https://gitlist.dev/e/20190218095423.0705aa7a%40naga In-Reply-To: ``` On Sun, 17 Feb 2019 02:14:14 -0500 Eric Sunshine wrote: > On Fri, Feb 15, 2019 at 1:16 PM Michal Suchanek wrote: > > When adding wotktrees git can die in get_common_dir_noenv while > > examining existing worktrees because the commondir file does not exist. > > Handle ENOENT so adding a worktree does not fail because of incompletely > > set-up other worktree. > > > > Signed-off-by: Michal Suchanek > > --- > > diff --git a/setup.c b/setup.c > > @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) > > + if (strbuf_read_file(&data, path.buf, 0) <= 0) { > > + if (errno != ENOENT) > > + die_errno(_("failed to read %s"), path.buf); > > Documentation for strbuf_read_file() in strbuf.h does not state that > 'errno' has any meaningful value when this function fails, however, It is stated in the documentation of strbuf_read(). Thanks Michal ```