Re: [PATCH 1/3] path.c: fix uninitialized memory access

From: Jeff King <peff@peff.net>
Date: Oct 4, 2017, 05:21 UTC
Message-ID: <20171004052151.4ntqmqe7wdbkbvx7@sigill.intra.peff.net>
In-Reply-To: <xmqqfuazecym.fsf@gitster.mtv.corp.google.com>

On Wed, Oct 04, 2017 at 01:47:29PM +0900, Junio C Hamano wrote:

Show 20 quoted lines

> Jonathan Nieder <jrnieder@gmail.com> 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@peff.net>
> 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)