Derrick Stolee <derrickstolee@xxxxxxxxxx> writes:
> 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 "../".
While "could segfault" is of course more correct, I do not see a
huge difference here, but that is only because I learned to equate
"segfaults" in our log messages with "makes an access to
inappropriate memory address".
If I were to suggest updating the proposed log message, I would
rather spend a bit more bytes to explain what callers expect
add_slash() to do, why they call the helper for. It would make it
obvious why it is the right behaviour the callers expect for the
function to return an empty string as-is.
I _think_ the reason is that the caller of add_slash has the name of
a directory in the working tree (relative to the root of the working
tree) and wants to add strings to form pathnames to things in the
directory. They have "Documentation" directory and are told to move
"Makefile" from somewhere into it, so they pass "Documentation",
want "Documentation/" back, to form "Documentation/Makefile" by
concatenating. If they are told to move something to the toplevel,
the target would be originally given as "." and while driving the
machinery to rename something to "./Makefile" might also work,
because the pathnames are normalized fairly early by removing excess
dots and resolving double-dots, the actual 'path' passed to
add_slash() by the caller in this case is an empty string, not a
single dot. And "move this Makefile sitting somewhere else to ."
means "the path to the resulting file is Makefile" (as opposed to
"the path to the resulting file is ./Makefile"), which is correct.
Of course, I expect the log message to explain it a lot more
concisely, instead of spending more than a dozen lines ;-)
Thanks.
