From: Nguyen Thai Ngoc Duy Date: Wed, 16 Feb 2011 03:39:24 GMT Subject: Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range Message-ID: In-Reply-To: <7vsjvpm1xh.fsf@alter.siamese.dyndns.org> 2011/2/16 Junio C Hamano : >> -     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 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