Re: [PATCH v3 1/2] worktree: fix worktree add race.

["Michal Suchánek" <msuchanek@suse.de>]

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