Jonathan Nieder <jrnieder@xxxxxxxxx> writes:
> Jeff King wrote:
>> On Tue, Oct 03, 2017 at 03:45:01PM -0700, Jonathan Nieder wrote:
>
>>> In other words, an alternative fix would be
>>>
>>> if (*path == '.' && path[1] == '/') {
>>> ...
>>> }
>>>
>>> which would not require passing in 'len' or switching to index-based
>>> arithmetic. I think I prefer it. What do you think?
>>
>> Yes, I think that approach is much nicer. I think you could even use
>> skip_prefix. Unfortunately you have to play a few games with const-ness,
>> but I think the resulting signature for cleanup_path() is an
>> improvement:
To tie the loose end, here is what I'll queue.
-- >8 --
From: Jeff King <peff@xxxxxxxx>
Date: Tue, 3 Oct 2017 19:30:40 -0400
Subject: [PATCH] path.c: fix uninitialized memory access
In cleanup_path we're passing in a char array, run a memcmp on it, and
run through it without ever checking if something is in the array in the
first place. This can lead us to access uninitialized memory, for
example in t5541-http-push-smart.sh test 7, when run under valgrind:
==4423== Conditional jump or move depends on uninitialised value(s)
==4423== at 0x242FA9: cleanup_path (path.c:35)
==4423== by 0x242FA9: mkpath (path.c:456)
==4423== by 0x256CC7: refname_match (refs.c:364)
==4423== by 0x26C181: count_refspec_match (remote.c:1015)
==4423== by 0x26C181: match_explicit_lhs (remote.c:1126)
==4423== by 0x26C181: check_push_refs (remote.c:1409)
==4423== by 0x2ABB4D: transport_push (transport.c:870)
==4423== by 0x186703: push_with_options (push.c:332)
==4423== by 0x18746D: do_push (push.c:409)
==4423== by 0x18746D: cmd_push (push.c:566)
==4423== by 0x1183E0: run_builtin (git.c:352)
==4423== by 0x11973E: handle_builtin (git.c:539)
==4423== by 0x11973E: run_argv (git.c:593)
==4423== by 0x11973E: main (git.c:698)
==4423== Uninitialised value was created by a heap allocation
==4423== at 0x4C2CD8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4423== by 0x4C2F195: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4423== by 0x2C196B: xrealloc (wrapper.c:137)
==4423== by 0x29A30B: strbuf_grow (strbuf.c:66)
==4423== by 0x29A30B: strbuf_vaddf (strbuf.c:277)
==4423== by 0x242F9F: mkpath (path.c:454)
==4423== by 0x256CC7: refname_match (refs.c:364)
==4423== by 0x26C181: count_refspec_match (remote.c:1015)
==4423== by 0x26C181: match_explicit_lhs (remote.c:1126)
==4423== by 0x26C181: check_push_refs (remote.c:1409)
==4423== by 0x2ABB4D: transport_push (transport.c:870)
==4423== by 0x186703: push_with_options (push.c:332)
==4423== by 0x18746D: do_push (push.c:409)
==4423== by 0x18746D: cmd_push (push.c:566)
==4423== by 0x1183E0: run_builtin (git.c:352)
==4423== by 0x11973E: handle_builtin (git.c:539)
==4423== by 0x11973E: run_argv (git.c:593)
==4423== by 0x11973E: main (git.c:698)
==4423==
Avoid this by using skip_prefix(), which knows not to go beyond the
end of the string.
Reported-by: Thomas Gummerer <t.gummerer@xxxxxxxxx>
Signed-off-by: Jeff King <peff@xxxxxxxx>
Reviewed-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
path.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/path.c b/path.c
index e50d2befcf..2fecf854fe 100644
--- a/path.c
+++ b/path.c
@@ -33,11 +33,10 @@ static struct strbuf *get_pathname(void)
return sb;
}
-static char *cleanup_path(char *path)
+static const char *cleanup_path(const char *path)
{
/* Clean it up */
- if (!memcmp(path, "./", 2)) {
- path += 2;
+ if (skip_prefix(path, "./", &path)) {
while (*path == '/')
path++;
}
@@ -46,7 +45,7 @@ static char *cleanup_path(char *path)
static void strbuf_cleanup_path(struct strbuf *sb)
{
- char *path = cleanup_path(sb->buf);
+ const char *path = cleanup_path(sb->buf);
if (path > sb->buf)
strbuf_remove(sb, 0, path - sb->buf);
}
@@ -63,7 +62,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
strlcpy(buf, bad_path, n);
return buf;
}
- return cleanup_path(buf);
+ return (char *)cleanup_path(buf);
}
static int dir_prefix(const char *buf, const char *dir)
--
2.14.2-889-gd2948f6aa6
