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

[Derrick Stolee <derrickstolee@github.com>]

On 9/8/2022 7:02 PM, Shaoxuan Yuan wrote:
> 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 segfaults when dest_path[0] is an empty string,
> because it was accessing a null value in such case.

It doesn't _always_ seg fault, since we have tests that cover this
case. Adding this change will cause t7001-mv.sh to start failing
in many places:

diff --git a/builtin/mv.c b/builtin/mv.c
index 2d64c1e80fe..8216680ad3c 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -71,6 +71,10 @@ static const char **internal_prefix_pathspec(const char *prefix,
 static const char *add_slash(const char *path)
 {
 	size_t len = strlen(path);
+
+	if (!len)
+		die("segfault?");
+
 	if (path[len - 1] != '/') {
 		char *with_slash = xmalloc(st_add(len, 2));
 		memcpy(with_slash, path, len);

I suppose it is better to say "could segfault". Running the test
under --valgrind also causes a failure. It covers both cases, "."
and "../".

This is all to say that there is some subtlety to the situation, which
helps justify the lack of a new test case (the tests cover this case,
but require extra steps to show a failure).

> Change add_slash() to check the path argument is a non-empty string
> before accessing its value.
> 
> The purpose of add_slash() is adding a slash to the end of a string to
> construct a directory path. And, because adding a slash to an empty
> string is of no use here, and checking the string value without checking
> it is non-empty leads to segfault, we should make sure the length of the
> string is positive to solve both problems.

I agree that the code change is correct.

Thanks,
-Stolee