Re: [PATCH v1.5] worktree: Fix out of bounds read that causes data loss and reject invalid empty input in worktree add

René Scharfe <[email protected]> writes:

> From: =?UTF-8?q?Matthias=20A=C3=9Fhauer?= <[email protected]>
>
> `worktree_basename` tries to read from memory before the passed `path`
> string, if `path` is empty (or only consists of directory separators).
> That results in unexpected nonsense data being returned to the caller,
> which can lead to issues, such as `git worktree add ""` recursively
> deleting the current working directory, including `.git`.
>
> Stop reading out of bounds in these cases to avoid that behaviour.
>
> This leads to `git worktree add ""` consistently exiting with the
> message `BUG: How come '' becomes empty after sanitization?`, which is
> still undesirable, but at least it doesn't result in data loss anymore.
>
> This fixes https://github.com/git-for-windows/git/issues/6346
>
> Signed-off-by: René Scharfe <[email protected]>
> ---
> How about this while we're waiting for a reroll?  It implements what the
> commit message says, nothing more.  Follows the style of the first loop.

This one I think is obvious and clear.  Why not take the authorship
too so that we do not have to worry about DCO?

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