threads / patch / 58409

patchbuiltin/mv.c: fix possible segfault in add_slash()

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

## tl;dr

9 messages between Sep 8, 2022 and Sep 9, 2022. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Shaoxuan Yuan· Sep 8, 2022, 23:02 UTC · lore

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.

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.

Reported-by: Jeff King <peff@peff.net>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
---
Reference: https://lore.kernel.org/git/YwdJRRuST2SP8ZT7@coredump.intra.peff.net/
 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/mv.c +1 −1
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
Jeff King· Sep 9, 2022, 02:21 UTC · re: Shaoxuan Yuan · lore

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

On Thu, Sep 08, 2022 at 04:02:23PM -0700, Shaoxuan Yuan wrote:
Show 5 quoted lines
> 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.
Thanks for picking this up. I had forgotten about it.

The patch looks obviously fine to me from the perspective of stopping the segfault. I'll take your "of no use here" as a given, not being familiar with the subtleties of mv's path handling. :) Assuming that's correct, then everything looks good to me.

-Peff
Derrick Stolee· Sep 9, 2022, 14:14 UTC · re: Shaoxuan Yuan · lore

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

On 9/8/2022 7:02 PM, Shaoxuan Yuan wrote:
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 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:

Show changes to builtin/mv.c +4 −1
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
Junio C Hamano· Sep 9, 2022, 16:37 UTC · re: Derrick Stolee · lore

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

Derrick Stolee <derrickstolee@github.com> writes:
Show 38 quoted lines
> 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.
Shaoxuan Yuan· Sep 9, 2022, 19:44 UTC · re: Shaoxuan Yuan · lore

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

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(-)
Show changes to builtin/mv.c +1 −1
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
Junio C Hamano· Sep 9, 2022, 20:04 UTC · re: Shaoxuan Yuan · lore

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

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.
Shaoxuan Yuan· Sep 9, 2022, 22:40 UTC · re: Junio C Hamano · lore

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

On 9/9/2022 1:04 PM, Junio C Hamano wrote:
Show 22 quoted lines
> Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
> 
>> 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.
Thanks for the term, new thing learned :-)
Show 13 quoted lines
>> 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:
Indeed, explanation before action sounds more reasonable.
Show 15 quoted lines
> 	... 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.
> 
Thanks for the paraphrase, I put it in the v3 just sent.
Show 16 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.
Yes.

Thanks, Shaoxuan

Shaoxuan Yuan· Sep 9, 2022, 22:27 UTC · re: Shaoxuan Yuan · lore

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

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

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 v2:
1:  1120dc7e6b ! 1:  569e618013 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 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.
    +    The add_slash() call 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.
     
         Reported-by: Jeff King <peff@peff.net>
         Helped-by: Jeff King <peff@peff.net>
 builtin/mv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/mv.c +1 −1
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: a6b4b080e4ef65ebbab73e47c0100b5dc12e104c

-- 
2.37.0
Junio C Hamano· Sep 9, 2022, 22:52 UTC · re: Shaoxuan Yuan · lore

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

Shaoxuan Yuan <shaoxuan.yuan02@gmail.com> writes:
> A possible segfault was introduced in c08830de41 (mv: check if
> <destination> is a SKIP_WORKTREE_DIR, 2022-08-09).

This iteration looks good to me (v1 was sufficiently readable already but an extra explanation made it easier to grok).

Thanks, will queue and let's merge it to 'next'.

← back to recent threads