On Wed, Oct 04, 2017 at 01:47:29PM +0900, Junio C Hamano wrote:
> 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.
Thank you, I was planning to get to this later tonight, but now I don't
have to. :)
FWIW, I wondered if we could get rid of the extra cast by switching
cleanup_path() to return an offset rather than a string (which also
makes its interface more foolproof, since it's clear that the return
value is a subset of the original string).
But it ends up being a bit clunky I think (patch below for reference).
I guess it's possible that `cleanup_path` could learn to do other
cleanup, too (e.g., to clean up doubled slashes in the middle of the
path), in which case it really would want a non-const buffer. But since
it has remained unchanged since 26c8a533af (Add "mkpath()" helper
function, 2005-07-08), I'm happy to assume it will remain so for another
12 years.
All of which is to say:
> -- >8 --
> From: Jeff King <peff@xxxxxxxx>
> Date: Tue, 3 Oct 2017 19:30:40 -0400
> Subject: [PATCH] path.c: fix uninitialized memory access
Looks good to me.
-Peff
-- >8 --
diff --git a/path.c b/path.c
index 5aa9244eb2..eaeb9d9a17 100644
--- a/path.c
+++ b/path.c
@@ -34,22 +34,24 @@ static struct strbuf *get_pathname(void)
return sb;
}
-static char *cleanup_path(char *path)
+static size_t cleanup_path(const char *path)
{
- /* Clean it up */
- if (!memcmp(path, "./", 2)) {
- path += 2;
- while (*path == '/')
- path++;
- }
- return path;
+ const char *clean;
+
+ if (!skip_prefix(path, "./", &clean))
+ return 0;
+
+ while (*clean == '/')
+ clean++;
+
+ return clean - path;
}
static void strbuf_cleanup_path(struct strbuf *sb)
{
- char *path = cleanup_path(sb->buf);
- if (path > sb->buf)
- strbuf_remove(sb, 0, path - sb->buf);
+ size_t s = cleanup_path(sb->buf);
+ if (s)
+ strbuf_remove(sb, 0, s);
}
char *mksnpath(char *buf, size_t n, const char *fmt, ...)
@@ -64,7 +66,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
strlcpy(buf, bad_path, n);
return buf;
}
- return cleanup_path(buf);
+ return buf + cleanup_path(buf);
}
static int dir_prefix(const char *buf, const char *dir)
