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
