Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Feb 16, 2011, 03:39 UTC
Message-ID: <AANLkTin+zR81H0aahveMMOwKyxVV_wRoab=pqk51joop@mail.gmail.com>
In-Reply-To: <7vsjvpm1xh.fsf@alter.siamese.dyndns.org>

2011/2/16 Junio C Hamano <gitster@pobox.com>:

Show 12 quoted lines

>> -     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).

Show 10 quoted lines

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

-- 
Duy