On 9/9/2022 1:04 PM, Junio C Hamano wrote:
> Shaoxuan Yuan <shaoxuan.yuan02@xxxxxxxxx> writes:
>
>> A possible segfault was introduced in c08830de41 (mv: check if
>> <destination> is a SKIP_WORKTREE_DIR, 2022-08-09).
>>
>> When running t7001 with SANITIZE=address, problem appears when running:
>>
>> git mv path1/path2/ .
>> or
>> git mv directory ../
>> or
>> any <destination> that makes dest_path[0] an empty string.
>>
>> The add_slash() call could segfault when dest_path[0] is an empty string,
>> because it was accessing a null value in such case.
>
> Terminology. The relevant preimage is
>
>> size_t len = strlen(path);
>> - if (path[len - 1] != '/') {
>
> An access to path[-1] is an out-of-bounds access.
Thanks for the term, new thing learned :-)
>> Change add_slash() to check the path argument is a non-empty string
>> before accessing its value. If the path is empty, return it as-is.
>
> That is not wrong per-se, but...
>
>> Explanation:
>
> ... you'd need this funny label here. If this is where your
> explanation begins, what was the reader reading before it? ;-)
>
> The logic would flow more naturally if you added your "explanation"
> material between "what is wrong in the current code" and "what to do
> to fix it", perhaps like so:
Indeed, explanation before action sounds more reasonable.
> ... could segfault when path argument to it is an empty
> string, because it makes an out-of-bounds read to decide if
> an extra slash '/' needs to be appended to it.
>
> As add_slash() is used to make sure that a valid pathname to
> a file in the given directory can be made by appending a
> filename after the value returned from it, if path is an
> empty string, we want to return it as-is. The path to a
> file "F" in the top-level of the working tree (i.e.
> path=="") is formed by appending "F" after "" (i.e. path)
> without any slash in between.
>
> So, just like the case where a non-empty path already ends
> with a slash, return an empty path as-is.
>
Thanks for the paraphrase, I put it in the v3 just sent.
>> diff --git a/builtin/mv.c b/builtin/mv.c
>> index 2d64c1e80f..3413ad1c9b 100644
>> --- a/builtin/mv.c
>> +++ b/builtin/mv.c
>> @@ -71,7 +71,7 @@ static const char **internal_prefix_pathspec(const char *prefix,
>> static const char *add_slash(const char *path)
>> {
>> size_t len = strlen(path);
>> - if (path[len - 1] != '/') {
>> + if (len && path[len - 1] != '/') {
>> char *with_slash = xmalloc(st_add(len, 2));
>> memcpy(with_slash, path, len);
>> with_slash[len++] = '/';
>
> Yup. It cannot be seen in the patch but the post-context of this
> hunk just returns path as-is, which is what we want to happen.
Yes.
Thanks,
Shaoxuan
