| Message ID | e134801d570d0a0c85424eb80b41893f4d8383ca.1550679076.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/2] worktree: fix worktree add race. | expand |
On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek <msuchanek@suse.de> wrote: > 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> > --- > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -295,8 +295,12 @@ 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 ((errno != EEXIST) || !counter /* overflow */) > + die_errno(_("could not create directory of '%s'"), > + sb_repo.buf); > strbuf_setlen(&sb_repo, len); > strbuf_addf(&sb_repo, "%d", counter); > } > @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname, > 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; Did you audit this "junk" handling to verify that stuff which ought to be cleaned up still is cleaned up now that the mkdir() and die() have been moved above the atexit(remove_junk) invocation? I did just audit it, and I _think_ that it still works as expected, but it would be good to hear that someone else has come to the same conclusion.
On Wed, 20 Feb 2019 11:34:54 -0500 Eric Sunshine <sunshine@sunshineco.com> wrote: > On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek <msuchanek@suse.de> wrote: > > 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> > > --- > > diff --git a/builtin/worktree.c b/builtin/worktree.c > > @@ -295,8 +295,12 @@ 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 ((errno != EEXIST) || !counter /* overflow */) > > + die_errno(_("could not create directory of '%s'"), > > + sb_repo.buf); > > strbuf_setlen(&sb_repo, len); > > strbuf_addf(&sb_repo, "%d", counter); > > } > > @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname, > > 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; > > Did you audit this "junk" handling to verify that stuff which ought to > be cleaned up still is cleaned up now that the mkdir() and die() have > been moved above the atexit(remove_junk) invocation? > > I did just audit it, and I _think_ that it still works as expected, > but it would be good to hear that someone else has come to the same > conclusion. The die() is executed only when mkdir() fails so there is no junk to clean up in that case. Thanks Michal
Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good regardless, maybe pick it up now and let 2/2 come later whenever it's ready? On Wed, Feb 20, 2019 at 11:16 PM Michal Suchanek <msuchanek@suse.de> wrote: > > 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> > --- > v2: > - simplify loop exit condition > - exit early if the mkdir fails for reason other than already present > worktree > - make counter unsigned > --- > builtin/worktree.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index 3f9907fcc994..85a604cfe98c 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -268,10 +268,10 @@ 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; > + unsigned int counter = 0; > + int len, ret; > struct strbuf symref = STRBUF_INIT; > struct commit *commit = NULL; > int is_branch = 0; > @@ -295,8 +295,12 @@ 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 ((errno != EEXIST) || !counter /* overflow */) > + die_errno(_("could not create directory of '%s'"), > + sb_repo.buf); > strbuf_setlen(&sb_repo, len); > strbuf_addf(&sb_repo, "%d", counter); > } > @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname, > 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 >
On Fri, Mar 8, 2019 at 4:20 AM Duy Nguyen <pclouds@gmail.com> wrote: > Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good > regardless, maybe pick it up now and let 2/2 come later whenever it's > ready? Yep, 1/2 seems a good idea and has not been controversial. It may not solve all the race conditions, but it is a good step forward.
Duy Nguyen <pclouds@gmail.com> writes: > Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good > regardless, maybe pick it up now and let 2/2 come later whenever it's > ready? Thanks for poking, and I think it is a good idea.
diff --git a/builtin/worktree.c b/builtin/worktree.c index 3f9907fcc994..85a604cfe98c 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -268,10 +268,10 @@ 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; + unsigned int counter = 0; + int len, ret; struct strbuf symref = STRBUF_INIT; struct commit *commit = NULL; int is_branch = 0; @@ -295,8 +295,12 @@ 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 ((errno != EEXIST) || !counter /* overflow */) + die_errno(_("could not create directory of '%s'"), + sb_repo.buf); strbuf_setlen(&sb_repo, len); strbuf_addf(&sb_repo, "%d", counter); } @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname, 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> --- v2: - simplify loop exit condition - exit early if the mkdir fails for reason other than already present worktree - make counter unsigned --- builtin/worktree.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)