# Worktree creation race 7 messages from 2019-01-15 to 2019-02-01. Participants: Marketa Calabkova, Eric Sunshine, Duy Nguyen, Junio C Hamano. Thread: https://gitlist.dev/t/50228 ## Marketa Calabkova, 2019-01-15 14:03 Subject: Worktree creation race Message-ID: <89985b60-4c28-ebf0-64b1-8da75263d745@suse.cz> URL: https://gitlist.dev/e/89985b60-4c28-ebf0-64b1-8da75263d745%40suse.cz ``` Hello, 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. to test: cd /dev/shm mkdir gittest cd gittest git init gitrepo 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) Greetings, Marketa ``` ## Marketa Calabkova, 2019-01-28 12:58 Subject: Re: Worktree creation race Message-ID: <2ee89ff3-e672-e940-f601-aa1d2647756a@suse.cz> URL: https://gitlist.dev/e/2ee89ff3-e672-e940-f601-aa1d2647756a%40suse.cz In-Reply-To: <89985b60-4c28-ebf0-64b1-8da75263d745@suse.cz> ``` Hi, you have probably overseen my email :) . Please, I would like to get it fixed. 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. Thank you, Marketa On 15/01/2019 15:03, Marketa Calabkova wrote: > Hello, > > 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. > > to test: > > cd /dev/shm > mkdir gittest > cd gittest > git init gitrepo > 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) > > Greetings, > Marketa > > ``` ## Eric Sunshine, 2019-02-01 06:27 Subject: Re: Worktree creation race Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24%3DZwbuQtsaLNZw%40mail.gmail.com In-Reply-To: <2ee89ff3-e672-e940-f601-aa1d2647756a@suse.cz> ``` On Mon, Jan 28, 2019 at 7:58 AM Marketa Calabkova 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). ``` ## Duy Nguyen, 2019-02-01 07:06 Subject: Re: Worktree creation race Message-ID: URL: https://gitlist.dev/e/CACsJy8D%2Bzvgtw8RuBefYQsYrvn_vQT1dUO3Veg5ZtfQa2u5QKA%40mail.gmail.com In-Reply-To: ``` Thanks for including me. Apparently I did miss some emails :) On Fri, Feb 1, 2019 at 1:27 PM Eric Sunshine wrote: > > On Mon, Jan 28, 2019 at 7:58 AM Marketa Calabkova 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. I never thought people would create worktrees "like crazy" to end up worrying about races like this. The mkdir loop would be one way to go. But I'm going to add a new option to let the user control this directory name. This is necessary since this name is now exposed via "worktrees/" reference space and should also be reported in "git worktree list". Avoiding the race is a nice bonus. > 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). -- Duy ``` ## Duy Nguyen, 2019-02-01 13:16 Subject: Re: Worktree creation race Message-ID: <20190201131659.GA30564@ash> URL: https://gitlist.dev/e/20190201131659.GA30564%40ash In-Reply-To: ``` On Fri, Feb 01, 2019 at 02:06:43PM +0700, Duy Nguyen wrote: > worrying about races like this. The mkdir loop would be one way to go. > But I'm going to add a new option to let the user control this > directory name. This is necessary since this name is now exposed via > "worktrees/" reference space and should also be reported in "git > worktree list". Avoiding the race is a nice bonus. I'm not going to bother you with code yet (although if you want, you can check out branch worktree-name on my gitlab repo), but this is what the user facing changes look like. Looking good? PS. I think this also calls for a command to rename working trees. Sigh.. this worktree thingy never ends. -- 8< -- diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index 18469e202e..d5db69dec7 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees SYNOPSIS -------- [verse] -'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] [] +'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] [--name ] [] 'git worktree list' [--porcelain] 'git worktree lock' [--reason ] 'git worktree move' @@ -195,6 +195,13 @@ This can also be set up as the default behaviour by using the --reason :: With `lock`, an explanation why the working tree is locked. +--name :: + Name of the working tree. Each working tree must have a unique + name. This is also the directory name containing all working + tree's specific information under `$GIT_COMMON_DIR/worktrees`. + If `--name` is not given, the name is based on basename(3) + optionally with a number suffix to make it unique. + :: Working trees can be identified by path, either relative or absolute. @@ -323,11 +330,15 @@ details on a single line with columns. For example: ------------ $ git worktree list -/path/to/bare-source (bare) -/path/to/linked-worktree abcd1234 [master] -/path/to/other-linked-worktree 1234abc (detached HEAD) +(main) /path/to/bare-source (bare) +linked /path/to/linked-worktree abcd1234 [master] +other /path/to/other-linked-worktree 1234abc (detached HEAD) ------------ +The first column is the name of the working tree. The second column is +the location of the working tree. The third column is the short commit +name of the current branch. The branch name is put in square brackets. + Porcelain Format ~~~~~~~~~~~~~~~~ The porcelain format has a line per attribute. Attributes are listed with a @@ -341,10 +352,12 @@ $ git worktree list --porcelain worktree /path/to/bare-source bare +name linked worktree /path/to/linked-worktree HEAD abcd1234abcd1234abcd1234abcd1234abcd1234 branch refs/heads/master +name other worktree /path/to/other-linked-worktree HEAD 1234abc1234abc1234abc1234abc1234abc1234a detached -- 8< -- -- Duy ``` ## Junio C Hamano, 2019-02-01 18:06 Subject: Re: Worktree creation race Message-ID: URL: https://gitlist.dev/e/xmqqh8dnz8mr.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Eric Sunshine writes: > 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). Yeah, I think that is the only sensible option to rely on the atomicity of mkdir() call. ``` ## Eric Sunshine, 2019-02-01 19:54 Subject: Re: Worktree creation race Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRg5E%3D_QuUz4j4sGNR%3DziHth0Lv7hncm_hvZboBvv%2BbnQ%40mail.gmail.com In-Reply-To: <20190201131659.GA30564@ash> ``` On Fri, Feb 1, 2019 at 8:17 AM Duy Nguyen wrote: > I'm not going to bother you with code yet (although if you want, you > can check out branch worktree-name on my gitlab repo), but this is > what the user facing changes look like. Looking good? > > -- 8< -- > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees > SYNOPSIS > +'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ] [--name ] [] > @@ -195,6 +195,13 @@ This can also be set up as the default behaviour by using the > +--name :: > + Name of the working tree. Each working tree must have a unique > + name. This is also the directory name containing all working > + tree's specific information under `$GIT_COMMON_DIR/worktrees`. > + If `--name` is not given, the name is based on basename(3) > + optionally with a number suffix to make it unique. I think the "DETAILS" section also needs to be updated. It presently says: The private sub-directory’s name is usually the base name of the linked working tree’s path, possibly appended with a number to make it unique. Perhaps amend it to: ...to make it unique, or the name provided by `--name`. > Porcelain Format > ~~~~~~~~~~~~~~~~ > The porcelain format has a line per attribute. Attributes are listed with a > @@ -341,10 +352,12 @@ $ git worktree list --porcelain > worktree /path/to/bare-source > bare > > +name linked > worktree /path/to/linked-worktree > HEAD abcd1234abcd1234abcd1234abcd1234abcd1234 > branch refs/heads/master > > +name other > worktree /path/to/other-linked-worktree > HEAD 1234abc1234abc1234abc1234abc1234abc1234a > detached Unfortunately, this will likely break existing tools. When I had outlined the proposed porcelain format, my suggestion was that a "worktree" line would indicate the start of a new stanza (with no blank line), however, as it eventually got implemented (not by me), that recommendation wasn't quite followed. Instead, what has been documented "officially" in the git-worktree man-page is that each stanza will start with a "worktree" line and that there will be a blank line between stanzas. So, the new "name" line will need to be placed somewhere after the "worktree" line in order to avoid tool breakage. ```