git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
Date
Sep 9, 2022, 19:44 UTC
Message-ID
<20220909194458.264735-1-shaoxuan.yuan02@gmail.com>
In-Reply-To
<20220908230223.239970-1-shaoxuan.yuan02@gmail.com>

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.

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.

Explanation:

It's OK for add_slash() to return an empty string as-is. add_slash() converts its path argument to the prefix (for "folder1/file1", "folder1/" is the prefix we mean here) for the result path. The path argument is an empty string _iff_ the result path is analyzed to be at the top level (this normalization process is done earlier by internal_prefix_pathspec()).

Because the prefix for a top-level path is an empty string, thus add_slash() should return an empty path argument as-is, both for correctness and avoiding inappropriate memory access.

Reported-by: Jeff King <peff@peff.net>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
Range-diff against v1:
1:  a5dccc030c ! 1:  82353f457d builtin/mv.c: fix possible segfault in add_slash()
    @@ Commit message
         or
                 any <destination> that makes dest_path[0] an empty string.
     
    -    The add_slash() call segfaults when dest_path[0] is 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.
     
         Change add_slash() to check the path argument is a non-empty string
    -    before accessing its value.
    +    before accessing its value. If the path is empty, return it as-is.
     
    -    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.
    +    Explanation:
    +
    +    It's OK for add_slash() to return an empty string as-is. add_slash()
    +    converts its path argument to the prefix (for "folder1/file1",
    +    "folder1/" is the prefix we mean here) for the result path. The path
    +    argument is an empty string _iff_ the result path is analyzed to be at
    +    the top level (this normalization process is done earlier by
    +    internal_prefix_pathspec()).
    +
    +    Because the prefix for a top-level path is an empty string, thus
    +    add_slash() should return an empty path argument as-is, both for
    +    correctness and avoiding inappropriate memory access.
     
         Reported-by: Jeff King <peff@peff.net>
         Helped-by: Jeff King <peff@peff.net>
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
    +    Helped-by: Derrick Stolee <derrickstolee@github.com>
         Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
     
      ## builtin/mv.c ##
 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
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++] = '/';

base-commit: e71f9b1de6941c8b449d0c0e17e457f999664bc9
-- 
2.37.0
Previous: Junio C HamanoNext: Junio C Hamano
Message 5 of 9 in “builtin/mv.c: fix possible segfault in add_slash()”
  1. builtin/mv.c: fix possible segfault in add_slash()Shaoxuan Yuan, Sep 8, 2022
  2. Jeff KingSep 9, 2022
  3. Derrick StoleeSep 9, 2022
  4. Junio C HamanoSep 9, 2022
  5. builtin/mv.c: fix possible segfault in add_slash()Shaoxuan Yuan, Sep 9, 2022
  6. Junio C HamanoSep 9, 2022
  7. Shaoxuan YuanSep 9, 2022
  8. builtin/mv.c: fix possible segfault in add_slash()Shaoxuan Yuan, Sep 9, 2022
  9. Junio C HamanoSep 9, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.