[PATCH 0/4] worktree add: worktree_basename() fixes

[PATCH 0/4] worktree add: worktree_basename() fixes

[René Scharfe]

Here's my take on solving the issues addressed by [1].  I'd prefer a
reroll of that series, but in case it won't come we can use this one.

The first fix is the most important one, avoiding potential data loss.
The third one starts to accept paths with trailing path separators, the
fourth one simplifies the code.

  worktree add: don't read out of bounds in worktree_basename()
  worktree add: reject separator-only path
  worktree add: trim slashes when deriving branch name from path
  worktree add: let worktree_basename() return string copy

 builtin/worktree.c      | 33 ++++++++++++++-------------------
 t/t2400-worktree-add.sh | 17 +++++++++++++++++
 2 files changed, 31 insertions(+), 19 deletions(-)


[1] https://lore.kernel.org/git/pull.2187.git.1784978348.gitgitgadget@gmail.com/

-- 
2.55.0

[PATCH 1/4] worktree add: don't read out of bounds in worktree_basename()

[René Scharfe]

When we search for the start of the basename and `len` is zero, `name`
ends up being `path` - 1, out of bounds.  Avoid that by checking before
decrementing.

Fixes https://github.com/git-for-windows/git/issues/6346.

Original-patch-by: Matthias Aßhauer <mha1993@live.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/worktree.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 654d27c3e1..a770dd5ead 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -303,11 +303,9 @@ static const char *worktree_basename(const char *path, int *olen)
 	while (len && is_dir_sep(path[len - 1]))
 		len--;
 
-	for (name = path + len - 1; name > path; name--)
-		if (is_dir_sep(*name)) {
-			name++;
-			break;
-		}
+	name = path + len;
+	while (name > path && !is_dir_sep(name[-1]))
+		name--;
 
 	*olen = len;
 	return name;
-- 
2.55.0

[PATCH 2/4] worktree add: reject separator-only path

[René Scharfe]

worktree_basename() extracts an empty basename from a path consisting
only of zero or more path separators.  We can't use that as a worktree
name.  Properly report such a path as invalid instead of triggering a
BUG that asks the user what just happened.

Original-patch-by: Matthias Aßhauer <mha1993@live.de>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/worktree.c      | 2 ++
 t/t2400-worktree-add.sh | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index a770dd5ead..a53e815cc9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -491,6 +491,8 @@ static int add_worktree(const char *path, const char *refname,
 
 	name = worktree_basename(path, &len);
 	strbuf_add(&sb, name, path + len - name);
+	if (!sb.len)
+		die(_("invalid path '%s'"), path);
 	sanitize_refname_component(sb.buf, &sb_name);
 	if (!sb_name.len)
 		BUG("How come '%s' becomes empty after sanitization?", sb.buf);
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 87b926728a..280d2e2c07 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -46,6 +46,10 @@ test_expect_success '"add" refuses to checkout locked branch' '
 	test_path_is_missing .git/worktrees/zere
 '
 
+test_expect_success '"add" rejects an empty path' '
+	test_must_fail git worktree add "" HEAD
+'
+
 test_expect_success 'checking out paths not complaining about linked checkouts' '
 	(
 	cd existing_empty &&
-- 
2.55.0

[PATCH 3/4] worktree add: trim slashes when deriving branch name from path

[René Scharfe]

worktree_basename() sets `n` to the length of `path` without trailing
path separators, not to the length of the basename.  This matters when
deriving a branch name from a path with more than one component.  E.g.:

   path: /new/worktree/
   s:         ^
   n:    |-----------|

So here xstrndup(s, n) copies up to 13 characters from "worktree/",
effectively to the end of the string, including the trailing dash.

Path separators are not allowed at the end of branch names, so strip
them off by calculating the basename length and extracting just that
part.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/worktree.c      |  4 ++--
 t/t2400-worktree-add.sh | 13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index a53e815cc9..01c245778e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -769,7 +769,7 @@ static char *dwim_branch(const char *path, char **new_branch)
 	int n;
 	int branch_exists;
 	const char *s = worktree_basename(path, &n);
-	char *branchname = xstrndup(s, n);
+	char *branchname = xmemdupz(s, path + n - s);
 	struct strbuf ref = STRBUF_INIT;
 
 	branch_exists = !check_branch_ref(the_repository, &ref, branchname) &&
@@ -878,7 +878,7 @@ static int add(int ac, const char **av, const char *prefix,
 	if (opts.orphan && !new_branch) {
 		int n;
 		const char *s = worktree_basename(path, &n);
-		new_branch = new_branch_to_free = xstrndup(s, n);
+		new_branch = new_branch_to_free = xmemdupz(s, path + n - s);
 	} else if (opts.orphan) {
 		; /* no-op */
 	} else if (opts.detach) {
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 280d2e2c07..7e2811fa77 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -298,6 +298,11 @@ test_expect_success '"add" with <branch> omitted' '
 	test_cmp_rev HEAD bat
 '
 
+test_expect_success '"add" with trailing slash and <branch> omitted' '
+	git worktree add waffle/bit/ &&
+	test_cmp_rev HEAD bit
+'
+
 test_expect_success '"add" checks out existing branch of dwimd name' '
 	git branch dwim HEAD~1 &&
 	git worktree add dwim &&
@@ -388,6 +393,14 @@ test_expect_success '"add --orphan (no -b)"' '
 	test_cmp expected actual
 '
 
+test_expect_success '"add --orphan with trailing slash (no -b)"' '
+	test_when_finished "git worktree remove -f -f neworphan" &&
+	git worktree add --orphan ./neworphan/ &&
+	echo refs/heads/neworphan >expected &&
+	git -C neworphan symbolic-ref HEAD >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success '"add --orphan --quiet"' '
 	test_when_finished "git worktree remove -f -f orphandir" &&
 	git worktree add --quiet --orphan -b neworphan orphandir 2>log.actual &&
-- 
2.55.0

[PATCH 4/4] worktree add: let worktree_basename() return string copy

[René Scharfe]

worktree_basename() requires callers to do pointer arithmetic to get the
actual basename.  Simplify them by doing the calculations in the
function and returning a copy of the basename directly.

Remind programmers to free the result by renaming the function to
worktree_basename_dup().  Two already do; convert the remaining one from
resetting a shared strbuf to freeing the allocated string, which
requires the same number of lines, but no arithmetic.  The added
allocation is negligible because it's small and there's only one per run
of "git worktree add".

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/worktree.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 01c245778e..d95824b2fd 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -294,7 +294,7 @@ static void remove_junk_on_signal(int signo)
 	raise(signo);
 }
 
-static const char *worktree_basename(const char *path, int *olen)
+static char *worktree_basename_dup(const char *path)
 {
 	const char *name;
 	int len;
@@ -307,8 +307,7 @@ static const char *worktree_basename(const char *path, int *olen)
 	while (name > path && !is_dir_sep(name[-1]))
 		name--;
 
-	*olen = len;
-	return name;
+	return xmemdupz(name, path + len - name);
 }
 
 /* check that path is viable location for worktree */
@@ -462,6 +461,7 @@ 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;
+	char *name_to_free = NULL;
 	struct strvec child_env = STRVEC_INIT;
 	unsigned int counter = 0;
 	int len, ret;
@@ -489,14 +489,12 @@ static int add_worktree(const char *path, const char *refname,
 	if (!commit && !opts->orphan)
 		die(_("invalid reference: %s"), refname);
 
-	name = worktree_basename(path, &len);
-	strbuf_add(&sb, name, path + len - name);
-	if (!sb.len)
+	name = name_to_free = worktree_basename_dup(path);
+	if (!*name)
 		die(_("invalid path '%s'"), path);
-	sanitize_refname_component(sb.buf, &sb_name);
+	sanitize_refname_component(name, &sb_name);
 	if (!sb_name.len)
-		BUG("How come '%s' becomes empty after sanitization?", sb.buf);
-	strbuf_reset(&sb);
+		BUG("How come '%s' becomes empty after sanitization?", name);
 	name = sb_name.buf;
 	repo_git_path_replace(the_repository, &sb_repo, "worktrees/%s", name);
 	len = sb_repo.len;
@@ -630,6 +628,7 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_release(&sb_git);
 	strbuf_release(&sb_name);
 	free_worktree(wt);
+	free(name_to_free);
 	return ret;
 }
 
@@ -766,10 +765,8 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
 
 static char *dwim_branch(const char *path, char **new_branch)
 {
-	int n;
 	int branch_exists;
-	const char *s = worktree_basename(path, &n);
-	char *branchname = xmemdupz(s, path + n - s);
+	char *branchname = worktree_basename_dup(path);
 	struct strbuf ref = STRBUF_INIT;
 
 	branch_exists = !check_branch_ref(the_repository, &ref, branchname) &&
@@ -876,9 +873,7 @@ static int add(int ac, const char **av, const char *prefix,
 	}
 
 	if (opts.orphan && !new_branch) {
-		int n;
-		const char *s = worktree_basename(path, &n);
-		new_branch = new_branch_to_free = xmemdupz(s, path + n - s);
+		new_branch = new_branch_to_free = worktree_basename_dup(path);
 	} else if (opts.orphan) {
 		; /* no-op */
 	} else if (opts.detach) {
-- 
2.55.0

Re: [PATCH 3/4] worktree add: trim slashes when deriving branch name from path

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> worktree_basename() sets `n` to the length of `path` without trailing
> path separators, not to the length of the basename.  This matters when
> deriving a branch name from a path with more than one component.  E.g.:
>
>    path: /new/worktree/
>    s:         ^
>    n:    |-----------|
>
> So here xstrndup(s, n) copies up to 13 characters from "worktree/",
> effectively to the end of the string, including the trailing dash.
>
> Path separators are not allowed at the end of branch names, so strip
> them off by calculating the basename length and extracting just that
> part.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/worktree.c      |  4 ++--
>  t/t2400-worktree-add.sh | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)

Hmph, so am I correct to understand that the symptom observable by
end-users of this is that we used to attempt creating "bit/" branch
when they request

    $ git worktree add waffle/bit/

and it wouldn't have worked until they said

    $ git worktree add waffle/bit

instead?  Not allowing a trailing slash when naming a directory is
nasty (it is a good practice to explicitly give a trailing slash
when naming a directory to avoid confusion), and it is a good fix.

It also should work fine with waffle/bit/// even though there is no
strong reason to allow it ;-).


> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index a53e815cc9..01c245778e 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -769,7 +769,7 @@ static char *dwim_branch(const char *path, char **new_branch)
>  	int n;
>  	int branch_exists;
>  	const char *s = worktree_basename(path, &n);
> -	char *branchname = xstrndup(s, n);
> +	char *branchname = xmemdupz(s, path + n - s);
>  	struct strbuf ref = STRBUF_INIT;
>  
>  	branch_exists = !check_branch_ref(the_repository, &ref, branchname) &&
> @@ -878,7 +878,7 @@ static int add(int ac, const char **av, const char *prefix,
>  	if (opts.orphan && !new_branch) {
>  		int n;
>  		const char *s = worktree_basename(path, &n);
> -		new_branch = new_branch_to_free = xstrndup(s, n);
> +		new_branch = new_branch_to_free = xmemdupz(s, path + n - s);
>  	} else if (opts.orphan) {
>  		; /* no-op */
>  	} else if (opts.detach) {
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index 280d2e2c07..7e2811fa77 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -298,6 +298,11 @@ test_expect_success '"add" with <branch> omitted' '
>  	test_cmp_rev HEAD bat
>  '
>  
> +test_expect_success '"add" with trailing slash and <branch> omitted' '
> +	git worktree add waffle/bit/ &&
> +	test_cmp_rev HEAD bit
> +'
> +
>  test_expect_success '"add" checks out existing branch of dwimd name' '
>  	git branch dwim HEAD~1 &&
>  	git worktree add dwim &&
> @@ -388,6 +393,14 @@ test_expect_success '"add --orphan (no -b)"' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success '"add --orphan with trailing slash (no -b)"' '
> +	test_when_finished "git worktree remove -f -f neworphan" &&
> +	git worktree add --orphan ./neworphan/ &&
> +	echo refs/heads/neworphan >expected &&
> +	git -C neworphan symbolic-ref HEAD >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success '"add --orphan --quiet"' '
>  	test_when_finished "git worktree remove -f -f orphandir" &&
>  	git worktree add --quiet --orphan -b neworphan orphandir 2>log.actual &&

Re: [PATCH 4/4] worktree add: let worktree_basename() return string copy

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> worktree_basename() requires callers to do pointer arithmetic to get the
> actual basename.  Simplify them by doing the calculations in the
> function and returning a copy of the basename directly.

OK.

> Remind programmers to free the result by renaming the function to
> worktree_basename_dup().  Two already do; convert the remaining one from

This is a bit surprising, depending on what "do" refers to, as I
read it to mean "Two callers already free what is returned by the
worktree_basename() function", which cannot be the case (or they
would be segfaulting already).  So I must have misunderstood this
sentence.  I count three callers of the function, so two do
something while the other one that needs conversion does something
else.

> resetting a shared strbuf to freeing the allocated string, which
> requires the same number of lines, but no arithmetic.  The added
> allocation is negligible because it's small and there's only one per run
> of "git worktree add".

This talks about the caller in builtin/worktree.c:add_worktree(),
and it is indeed far easier to read with this patch applied, as
there is no need to copy out only the basename part, and we no
longer need to worry about chomping trailing directory separators.

> @@ -766,10 +765,8 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
>  
>  static char *dwim_branch(const char *path, char **new_branch)
>  {
> -	int n;
>  	int branch_exists;
> -	const char *s = worktree_basename(path, &n);
> -	char *branchname = xmemdupz(s, path + n - s);
> +	char *branchname = worktree_basename_dup(path);
>  	struct strbuf ref = STRBUF_INIT;

Ah, OK, so this is what you mean by "two already do".  Not "two
already free the result", but "two already make a copy before doing
anything else anyway, so why not make worktree_basename_dup() give
them their own copies?".  Makes sense.

> @@ -876,9 +873,7 @@ static int add(int ac, const char **av, const char *prefix,
>  	}
>  
>  	if (opts.orphan && !new_branch) {
> -		int n;
> -		const char *s = worktree_basename(path, &n);
> -		new_branch = new_branch_to_free = xmemdupz(s, path + n - s);
> +		new_branch = new_branch_to_free = worktree_basename_dup(path);

Likewise.


So going back to the confusing part of the log message,

    Remind ... to worktree_basename_dup().  Among the three callers
    of worktree_basename(), two immediately make copies of the
    returned string before using and freeing it, which makes for an
    easy conversion.  Convert the other one from resetting ...

or something like that, perhaps?

Thanks.

Re: [PATCH 4/4] worktree add: let worktree_basename() return string copy

[René Scharfe]

On 8/25/26 10:04 PM, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
>> worktree_basename() requires callers to do pointer arithmetic to get the
>> actual basename.  Simplify them by doing the calculations in the
>> function and returning a copy of the basename directly.
> 
> OK.
> 
>> Remind programmers to free the result by renaming the function to
>> worktree_basename_dup().  Two already do; convert the remaining one from
> 
> This is a bit surprising, depending on what "do" refers to, as I
> read it to mean "Two callers already free what is returned by the
> worktree_basename() function", which cannot be the case (or they
> would be segfaulting already).  So I must have misunderstood this
> sentence.  I count three callers of the function, so two do
> something while the other one that needs conversion does something
> else.

It's confusing because I changed "callers" to "programmers" last
minute and forgot to adjust the next sentence.

>> resetting a shared strbuf to freeing the allocated string, which
>> requires the same number of lines, but no arithmetic.  The added
>> allocation is negligible because it's small and there's only one per run
>> of "git worktree add".

> So going back to the confusing part of the log message,
> 
>     Remind ... to worktree_basename_dup().  Among the three callers
>     of worktree_basename(), two immediately make copies of the
>     returned string before using and freeing it, which makes for an
>     easy conversion.  Convert the other one from resetting ...
> 
> or something like that, perhaps?

Yes.

René

Re: [PATCH 4/4] worktree add: let worktree_basename() return string copy

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> On 8/25/26 10:04 PM, Junio C Hamano wrote:
>> René Scharfe <l.s.r@web.de> writes:
>> 
>>> worktree_basename() requires callers to do pointer arithmetic to get the
>>> actual basename.  Simplify them by doing the calculations in the
>>> function and returning a copy of the basename directly.
>> 
>> OK.
>> 
>>> Remind programmers to free the result by renaming the function to
>>> worktree_basename_dup().  Two already do; convert the remaining one from
>> 
>> This is a bit surprising, depending on what "do" refers to, as I
>> read it to mean "Two callers already free what is returned by the
>> worktree_basename() function", which cannot be the case (or they
>> would be segfaulting already).  So I must have misunderstood this
>> sentence.  I count three callers of the function, so two do
>> something while the other one that needs conversion does something
>> else.
>
> It's confusing because I changed "callers" to "programmers" last
> minute and forgot to adjust the next sentence.
>
>>> resetting a shared strbuf to freeing the allocated string, which
>>> requires the same number of lines, but no arithmetic.  The added
>>> allocation is negligible because it's small and there's only one per run
>>> of "git worktree add".
>
>> So going back to the confusing part of the log message,
>> 
>>     Remind ... to worktree_basename_dup().  Among the three callers
>>     of worktree_basename(), two immediately make copies of the
>>     returned string before using and freeing it, which makes for an
>>     easy conversion.  Convert the other one from resetting ...
>> 
>> or something like that, perhaps?
>
> Yes.

Thanks.  We do not know if other parts of the series gets more
serious reviews that necessitates an updated version, so in the
meantime I'll reword what I have locally.