Re: [PATCH 1/5] avoid parse_sha1_header() accessing memory out of bound

["Shawn O. Pearce" <spearce@spearce.org>]

Liu Yubao <yubao.liu@gmail.com> wrote:
> diff --git a/sha1_file.c b/sha1_file.c
> index 6c0e251..efe6967 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1254,10 +1255,10 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>  	/*
>  	 * The type can be at most ten bytes (including the
>  	 * terminating '\0' that we add), and is followed by
> -	 * a space.
> +	 * a space, at least one byte for size, and a '\0'.
>  	 */
>  	i = 0;
> -	for (;;) {
> +	while (hdr < hdr_end - 2) {
>  		char c = *hdr++;
>  		if (c == ' ')
>  			break;
> @@ -1265,6 +1266,8 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>  		if (i >= sizeof(type))
>  			return -1;

That first hunk I am citing is unnecessary, because of the lines
right above.  All of the callers of this function pass in a buffer
that is at least 32 bytes in size; this loop aborts if it does not
find a ' ' within the first 10 bytes of the buffer.  We'll never
access memory outside of the buffer during this loop.

So IMHO your first three hunks here aren't necessary.

> @@ -1275,7 +1278,7 @@ static int parse_sha1_header(const char *hdr, unsigned long *sizep)
>  	if (size > 9)
>  		return -1;
>  	if (size) {
> -		for (;;) {
> +		while (hdr < hdr_end - 1) {
>  			unsigned long c = *hdr - '0';
>  			if (c > 9)
>  				break;

OK, there's no promise here that we don't roll off the buffer.

This can be fixed in the caller, ensuring we always have the '\0'
at some point in the initial header buffer we were asked to parse:

diff --git a/sha1_file.c b/sha1_file.c
index 6c0e251..26c6ffb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1162,9 +1162,10 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 	stream->next_in = map;
 	stream->avail_in = mapsize;
 	stream->next_out = buffer;
-	stream->avail_out = bufsiz;
 
 	if (legacy_loose_object(map)) {
+		stream->avail_out = bufsiz - 1;
+		buffer[bufsiz - 1] = '\0';
 		inflateInit(stream);
 		return inflate(stream, 0);
 	}
@@ -1186,6 +1187,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon
 	/* Set up the stream for the rest.. */
 	stream->next_in = map;
 	stream->avail_in = mapsize;
+	stream->avail_out = bufsiz;
 	inflateInit(stream);
 
 	/* And generate the fake traditional header */

-- 
Shawn.