From: Junio C Hamano <gitster@pobox.com>
To: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
Cc: derrickstolee@github.com, git@vger.kernel.org, peff@peff.net,
vdye@github.com
Subject: Re: [PATCH v2] builtin/mv.c: fix possible segfault in add_slash()
Date: Fri, 09 Sep 2022 13:04:56 -0700 [thread overview]
Message-ID: <xmqqo7voiaaf.fsf@gitster.g> (raw)
In-Reply-To: <20220909194458.264735-1-shaoxuan.yuan02@gmail.com> (Shaoxuan Yuan's message of "Fri, 9 Sep 2022 12:44:58 -0700")
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.
next prev parent reply other threads:[~2022-09-09 20:05 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-08 23:02 [PATCH] builtin/mv.c: fix possible segfault in add_slash() Shaoxuan Yuan
2022-09-09 2:21 ` Jeff King
2022-09-09 14:14 ` Derrick Stolee
2022-09-09 16:37 ` Junio C Hamano
2022-09-09 19:44 ` [PATCH v2] " Shaoxuan Yuan
2022-09-09 20:04 ` Junio C Hamano [this message]
2022-09-09 22:40 ` Shaoxuan Yuan
2022-09-09 22:27 ` [PATCH v3] " Shaoxuan Yuan
2022-09-09 22:52 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqo7voiaaf.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
--cc=shaoxuan.yuan02@gmail.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).