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

Nguyễn Thái Ngọc Duy [off-list ref] 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.

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?

2011/2/16 Junio C Hamano [off-list ref]:

quoted

-     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 [off-list ref] writes:

quoted

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

Am 16.02.2011 04:39, schrieb Nguyen Thai Ngoc Duy:

2011/2/16 Junio C Hamano[off-list ref]:

quoted

quoted

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

quoted

Nguyễn Thái Ngọc Duy[off-list ref]  writes:

quoted

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é

There is a check (size < 64) at the beginning of the function, but
that only covers object+type lines. Code for parsing "tag" and
"tagger" may access outside buffer. Fix it.

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 On Thu, Feb 17, 2011 at 7:43 PM, René Scharfe [off-list ref] wrote:
 > 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..

 I did not notice that. Fixed commit message.

 tag.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tag.c b/tag.c
index ecf7c1e..7d38cc0 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);

@@ -106,7 +108,7 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
 	item->tag = xmemdupz(bufptr, nl - bufptr);
 	bufptr = nl + 1;
 
-	if (!prefixcmp(bufptr, "tagger "))
+	if (bufptr + 7 < tail && !prefixcmp(bufptr, "tagger "))
 		item->date = parse_tag_date(bufptr, tail);
 	else
 		item->date = 0;

-- 
1.7.4.74.g639db