From: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Date: Thu, 17 Feb 2011 12:43:35 GMT
Subject: Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range
Message-ID: <4D5D17F7.4010003@lsrfire.ath.cx>
In-Reply-To: <AANLkTin+zR81H0aahveMMOwKyxVV_wRoab=pqk51joop@mail.gmail.com>

Am 16.02.2011 04:39, schrieb Nguyen Thai Ngoc Duy:
> 2011/2/16 Junio C Hamano<gitster@pobox.com>:
>>> -     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?
>
> Yes, memchr() would be fed negative, but prefixcmp() already steps
> outside allocated memory. I believe that caused valgrind error Thomas
> reported (although I couldn't reproduce it).
>
>> 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.
>
> Because memchr() would be fed negative, memchr() would fail so the
> code is still correct.

memchr() won't notice if a negative value has been passed as third 
parameter because its type is size_t, which is unsigned.  Negative 
values are converted to big positive ones..

René