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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 3, 2017, 22:45 UTC
Message-ID: <20171003224501.GD19555@aiede.mtv.corp.google.com>
In-Reply-To: <20171003195713.13395-2-t.gummerer@gmail.com>

Hi,

Thomas Gummerer wrote:

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

[...]

Show 7 quoted lines

> Avoid this by checking passing in the length of the string in the char
> array, and checking that we never run over it.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  path.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

When I first read the above, I thought it was going to be about a NUL-terminated string that was missing a NUL. But in fact, the issue is that strlen(path) can be < 2.

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?

Thanks and hope that helps, Jonathan

diff --git i/path.c w/path.c
index b533ec938d..3a1fbee1e0 100644
--- i/path.c
+++ w/path.c
@@ -37,7 +37,7 @@ static struct strbuf *get_pathname(void)
 static char *cleanup_path(char *path)
 {
 	/* Clean it up */
-	if (!memcmp(path, "./", 2)) {
+	if (*path == '.' && path[1] == '/') {
 		path += 2;
 		while (*path == '/')
 			path++;