Re: Worktree creation race

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 28, 2019 at 7:58 AM Marketa Calabkova <mcalabkova@suse.cz> wrote:
> On 15/01/2019 15:03, Marketa Calabkova wrote:
> > I am writing to report a bug. The original report is from my colleague, I am also providing his suggestions.
> >
> > There is insufficient locking for worktree addition. Adding worktree may fail.
> >
> > The problem is that git reads the directory entries in $GIT_DIR/worktrees,
> > finds a worktree name that does not exist, tries to create it, and if an
> > error is returned adding the worktree fails. When multiple git processes
> > do this in parallel only one adds a worktree and the others fail. Git should
> > reread the directory and find a new name that does not exist when creating
> > the worktree directory fails because another git process already created it.
> >
> > I suppose adding PID in the tree name would mitigate the issue to the point it will be very unlikely to encounter.
> >
> > I need more than the tree in the temporary directory so using the temporary directory directly as a tree is out of question.
> >
> > cd gitrepo
> > git commit --allow-empty -m Empty
> > for n in $(seq 10000) ; do ( tmp=$(mktemp -d /dev/shm/gittest/test.XXXXXXXXXXX) ; mkdir $tmp/test ; git worktree add --detach $tmp/test ; ) & done
> >
> > (you should see many messages like:
> > fatal: could not create directory of '.git/worktrees/test284': File exists)
> >
> Does anyone has a suggestion what to do with this bug? It looks like a
> one-line fix probably in builtin/worktree.c, but I have no idea how to
> do it. Sorry.

I doubt this is a one-line fix, and I don't think it has anything to
do with reading entries in $GIT_DIR/worktrees.

add_worktree() already attempts to give a unique identifier to each
worktree by adding a numeric suffix and incrementing that suffix if
the name already exists (such as the 284 in your example error
message) but there is definitely a race-condition between the time it
stat()s the name and the time it mkdir()s it.

One possible fix would be to unconditionally use the PID, as you
suggest, though, this is not necessarily foolproof against races
either (though it makes collisions very unlikely).

Another possibility would be to skip the stat() and instead do the
mkdir() in a loop, incrementing the sequence number each time through
the loop. That should eliminate the race entirely (I think).