| Message ID | 429046b2c9f02c5e4f0af88db51f6c0c099f08a9.1550254374.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] worktree: fix worktree add race. | expand |
Michal Suchanek <msuchanek@suse.de> 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 <msuchanek@suse.de> > --- > 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;
On Fri, 15 Feb 2019 10:59:33 -0800 Junio C Hamano <gitster@pobox.com> wrote: > Michal Suchanek <msuchanek@suse.de> 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 <msuchanek@suse.de> > > --- > > 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
On Fri, Feb 15, 2019 at 1:59 PM Junio C Hamano <gitster@pobox.com> wrote: > Michal Suchanek <msuchanek@suse.de> 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/
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;
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 <msuchanek@suse.de> --- builtin/worktree.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)