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

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

[Shaoxuan Yuan]

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(-)

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

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

[Jeff King]

On Thu, Sep 08, 2022 at 04:02:23PM -0700, Shaoxuan Yuan wrote:

> 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

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

[Derrick Stolee]

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

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

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> 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.

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

[Shaoxuan Yuan]

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

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

[Junio C Hamano]

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.

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


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

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

[Shaoxuan Yuan]

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(-)

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

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

[Shaoxuan Yuan]

On 9/9/2022 1:04 PM, Junio C Hamano wrote:
> 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 :-)

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

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

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

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

[Junio C Hamano]

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