Re: [PATCH 1/2] worktree: don't read out of bounds

["René Scharfe" <l.s.r@web.de>]

On 7/25/26 6:51 PM, Junio C Hamano wrote:
> "Matthias Aßhauer via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> `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`.
> 
> OK, so you do want to handle a case where path is something silly
> like "///".
> 
>> 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.
> 
> OK.
> 
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index 4bc7b4f6e7..d8188035db 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -297,17 +297,21 @@ static void remove_junk_on_signal(int signo)
>>  static const char *worktree_basename(const char *path, int *olen)
>>  {
>>  	const char *name;
>> -	int len;
>> +	int len, len2;
>>  
>> -	len = strlen(path);
>> +	len2 = len = strlen(path);
>>  	while (len && is_dir_sep(path[len - 1]))
>>  		len--;
> 
> These two 'len' variables should have clear names to distinguish
> what each length represents.  Rather than introducing a cryptic
> 'len2', give it a more meaningful name, and rename 'len' as well if
> necessary.
> 
> I suspect that it is to remember the original length of the 'path'
> before stripping the trailing directory separators?
> 
>> -	for (name = path + len - 1; name > path; name--)
>> -		if (is_dir_sep(*name)) {
>> -			name++;
>> -			break;
>> -		}
> 
> When 'len' is 0, the original code sets 'name' to '&path[-1]' and
> does not enter the loop.  However, '*olen' is set to 0, and 'name',
> pointing before the start of the string, is returned.  If left
> unfixed, callers pass it to xstrndup(), strbuf_add(), and the like,
> reading memory before the start of the string, which is horrible and
> worth fixing.
> 
>> +	if(len) {
>> +		for (name = path + len - 1; name > path; name--)
>> +			if (is_dir_sep(*name)) {
>> +				name++;
>> +				break;
>> +			}
>> +	}
>> +	else
>> +		name = path + len2;
> 
> Style:
> 
>  (1) Missing SP between 'if' and '(len'.
> 
>  (2) 'else' sits on the same line as '}' that closes the 'if'
>      clause.
> 
>  (3) When any one branch of an 'if'...'else if'...'else' cascade
>      needs a pair of braces to group multiple statements, all other
>      branches must use braces as well.
> 
> Taken together:
> 
> 	if (len) {
> 		...
> 	} else {
> 		...
> 	}
> 
> As for what the patch intends to do, setting 'name = path + len2'
> when 'len' is 0 breaks when 'path' consists only of directory
> separators (for example, "/" or "///"), no?
> 
> In that case, 'len2' is positive (for example, 3) while 'len' is 0.
> In add_worktree(), 'path + len - name' evaluates to (path + 0) -
> (path + 3) = -3.  Passed as size_t to strbuf_add(), this wraps
> around to SIZE_MAX - 2 (approx. 18 exabytes), leading to a buffer
> allocation failure or a crash.
> 
> Rather than calculating 'path - 1' out of bounds or introducing
> 'len2', worktree_basename() can simply keep 'name = path' when 'len'
> is 0.  Using an integer index loop 'for (int i = len - 1; 0 <= i;
> i--)' avoids pointer arithmetic before the start of the buffer
> entirely, I would think.  Or am I missing something?
Interesting.  This function has two types of callers.  add_worktree()
does:

	name = worktree_basename(path, &len);
	strbuf_add(&sb, name, path + len - name);

While dwim_branch() and add() do basically:

	name = worktree_basename(path, &len);
	copy = xstrndup(name, len);

If path is empty or all separators, len is 0 and name invalid, as noted.
add_worktree() breaks, the other callers are mostly fine because
xstrndup() calls memchr(3) and memcpy(3) with a size of 0 internally
and thus doesn't dereference the pointer in practice.  So patch 2 should
suffice to prevent the out of bounds read.

If path contains one component ("foo/"), add_worktree() adds "foo" to
the strbuf and the others similarly duplicate "foo".  OK.

If path contains more components ("foo/bar/"), add_worktree() adds
"bar", while the others duplicate "bar/", because len is the length of
path without any trailing separator (7 in this example).

xstrndup("bar/", 7) could read out of bounds because it's calling
memchr(3) internally, which doesn't have to stop at the found byte.  But
more practically, do we really want to keep trailing separators?

worktree_basename() would be harder to misuse if it gave the length of
the basename instead.  Then add_worktree() wouldn't have to do any
pointer arithmetic and the others wouldn't risk reading out of bounds.

René