Re: [PATCH v2] builtin/mv.c: fix possible segfault in add_slash()

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 9, 2022, 20:04 UTC
Message-ID: <xmqqo7voiaaf.fsf@gitster.g>
In-Reply-To: <20220909194458.264735-1-shaoxuan.yuan02@gmail.com>

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:

Show 13 quoted lines

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

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

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

Show 13 quoted lines

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

Thanks.