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

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

On Fri, 15 Feb 2019 10:59:33 -0800
Junio C Hamano <gitster@pobox.com> wrote:

> Michal Suchanek <msuchanek@suse.de> writes:
> 
> > 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.  
> 
> Yeah, relying on the atomicity of mkdir(2) is much saner approach
> than "check -- ah we can use the name -- try to create" that is race
> prone.
> 
> Thanks for working on this.
> 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  builtin/worktree.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/builtin/worktree.c b/builtin/worktree.c
> > index 3f9907fcc994..e1a2a56c03c5 100644
> > --- a/builtin/worktree.c
> > +++ b/builtin/worktree.c
> > @@ -268,10 +268,9 @@ 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;
> > +	int counter = 1, len, ret;
> >  	struct strbuf symref = STRBUF_INIT;
> >  	struct commit *commit = NULL;
> >  	int is_branch = 0;
> > @@ -295,19 +294,21 @@ 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(!counter) break; /* don't loop forever */
> >  		strbuf_setlen(&sb_repo, len);
> >  		strbuf_addf(&sb_repo, "%d", counter);  
> 
> Style:
> 
> 		if (!counter)
> 			break; /* don't loop forever */
> 
> More importantly, how long would it take to loop thru all possible
> integers (can be simulated by making the parent directory
> unwritable)?  Don't we want to cut off with more conservative upper
> limit, say 1000 rounds or even 100 rounds or so?
> 
> Also, is the behaviour for a signed integer wrapping around due to
> getting incremented too many times well defined?  I'd feel safer,
> especially if you are willing to spin for 4 billion times like this
> patch does, if you changed the counter to "unsigned int".

If there are 4 billion worktrees ..
but there is no need to spin if the failure reason is not EEXIST.

> 
> I see you changed "counter" to start from 1, but that would mean
> that these fallback names would start with suffix 2, not 1.  Which
> would look funny.
> 
> I would have expected ".1", ".2", etc.  as suffix, but the original
> used "1", "2", etc. so I won't complain on the format, but I do find
> it questionable to start counting from 2.

Yes, there is no need to change the starting counter.

> 
> >  	}
> > +	if (!counter)
> > +		die_errno(_("could not create directory of '%s'"), sb_repo.buf);  
> 
> It would have saved reviewer's time if this die() were inside the
> loop where you punted with "break".

Yes, that's a leftover of the existing code structure.

> 
> >  	name = strrchr(sb_repo.buf, '/') + 1;
> >  
> >  	junk_pid = getpid();
> >  	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;  

Thanks

Michal