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

On Wed, 20 Feb 2019 11:34:54 -0500
Eric Sunshine [off-list ref] wrote:

On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek [off-list ref] wrote:

quoted

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 <redacted>
---

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