From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 15 Feb 2011 21:18:50 GMT
Subject: Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range
Message-ID: <7vsjvpm1xh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1297688571-1962-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> There is a check (size < 64) at the beginning of the function, but
> that only covers object+type lines.
>
> Strictly speaking the current code is still correct even if it
> accesses outside 'data' because 'tail' is used right after
> prefixcmp() calls.

What do you mean by this?  I don't get it.

> diff --git a/tag.c b/tag.c
> index ecf7c1e..9318ae5 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -97,7 +97,9 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
>  		item->tagged = NULL;
>  	}
>  
> -	if (prefixcmp(bufptr, "tag "))
> +	if (bufptr + 4 < tail && !prefixcmp(bufptr, "tag "))
> +		; 		/* good */
> +	else
>  		return -1;
>  	bufptr += 4;
>  	nl = memchr(bufptr, '\n', tail - bufptr);

If there weren't enough bytes between bufptr and tail, prefixcmp may still
match with "tag " while later part of the matched string might be coming
from trailing garbage outside our memory.  Unless we correctly fail the
prefixcmp() part, memchr() would be fed negative value, no?