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

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

[Michal Suchanek]

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>
---
 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);
 	}
+	if (!counter)
+		die_errno(_("could not create directory of '%s'"), sb_repo.buf);
 	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;
 
-- 
2.20.1

[PATCH 2/2] setup: don't fail if commondir is deleted.

[Michal Suchanek]

When adding wotktrees git can die in get_common_dir_noenv while
examining existing worktrees because the commondir file does not exist.
Handle ENOENT so adding a worktree does not fail because of incompletely
set-up other worktree.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 setup.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/setup.c b/setup.c
index ca9e8a949ed8..7dec2e5589d9 100644
--- a/setup.c
+++ b/setup.c
@@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
 
 	strbuf_addf(&path, "%s/commondir", gitdir);
 	if (file_exists(path.buf)) {
-		if (strbuf_read_file(&data, path.buf, 0) <= 0)
-			die_errno(_("failed to read %s"), path.buf);
-		while (data.len && (data.buf[data.len - 1] == '\n' ||
-				    data.buf[data.len - 1] == '\r'))
-			data.len--;
-		data.buf[data.len] = '\0';
-		strbuf_reset(&path);
-		if (!is_absolute_path(data.buf))
-			strbuf_addf(&path, "%s/", gitdir);
-		strbuf_addbuf(&path, &data);
-		strbuf_add_real_path(sb, path.buf);
-		ret = 1;
-	} else {
-		strbuf_addstr(sb, gitdir);
+		if (strbuf_read_file(&data, path.buf, 0) <= 0) {
+			if (errno != ENOENT)
+				die_errno(_("failed to read %s"), path.buf);
+		} else {
+			while (data.len && (data.buf[data.len - 1] == '\n' ||
+						data.buf[data.len - 1] == '\r'))
+				data.len--;
+			data.buf[data.len] = '\0';
+			strbuf_reset(&path);
+			if (!is_absolute_path(data.buf))
+				strbuf_addf(&path, "%s/", gitdir);
+			strbuf_addbuf(&path, &data);
+			strbuf_add_real_path(sb, path.buf);
+			ret = 1;
+			goto out;
+		}
 	}
-
+	strbuf_addstr(sb, gitdir);
+out:
 	strbuf_release(&data);
 	strbuf_release(&path);
 	return ret;
-- 
2.20.1

Re: [PATCH 2/2] setup: don't fail if commondir is deleted.

[Eric Sunshine]

On Fri, Feb 15, 2019 at 1:16 PM Michal Suchanek <msuchanek@suse.de> wrote:
> When adding wotktrees git can die in get_common_dir_noenv while
> examining existing worktrees because the commondir file does not exist.
> Handle ENOENT so adding a worktree does not fail because of incompletely
> set-up other worktree.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> diff --git a/setup.c b/setup.c
> @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> +               if (strbuf_read_file(&data, path.buf, 0) <= 0) {
> +                       if (errno != ENOENT)
> +                               die_errno(_("failed to read %s"), path.buf);

Documentation for strbuf_read_file() in strbuf.h does not state that
'errno' has any meaningful value when this function fails, however,
the actual implementation in strbuf.c is careful to preserve 'errno'
when something goes wrong. Therefore, it is safe to consult 'errno'
here. Fine.

It might be a good idea to fix the documentation of strbuf_read_file()
to mention 'errno' explicitly, as a preparatory patch of this series
(though not at all a requirement when re-rolling; just a suggestion to
assist reviewers).

Re: [PATCH 2/2] setup: don't fail if commondir is deleted.

[Michal Suchánek]

On Sun, 17 Feb 2019 02:14:14 -0500
Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Fri, Feb 15, 2019 at 1:16 PM Michal Suchanek <msuchanek@suse.de> wrote:
> > When adding wotktrees git can die in get_common_dir_noenv while
> > examining existing worktrees because the commondir file does not exist.
> > Handle ENOENT so adding a worktree does not fail because of incompletely
> > set-up other worktree.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> > diff --git a/setup.c b/setup.c
> > @@ -274,22 +274,25 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> > +               if (strbuf_read_file(&data, path.buf, 0) <= 0) {
> > +                       if (errno != ENOENT)
> > +                               die_errno(_("failed to read %s"), path.buf);  
> 
> Documentation for strbuf_read_file() in strbuf.h does not state that
> 'errno' has any meaningful value when this function fails, however,

It is stated in the documentation of strbuf_read().

Thanks

Michal

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

[Junio C Hamano]

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".

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.

>  	}
> +	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".

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

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

[Michal Suchánek]

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

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

[Eric Sunshine]

On Fri, Feb 15, 2019 at 1:59 PM 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.

Yep. When you re-roll, please mention [1] in the cover letter or in
the commentary section of this patch (after the "---" line below your
sign-off) to provide context to reviewers. Also, when re-rolling,
please Cc: people involved in that earlier discussion (whom I've added
to the Cc: of this reply). Thanks.

[1]: http://public-inbox.org/git/CAPig+cSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24=ZwbuQtsaLNZw@mail.gmail.com/