git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 1/2] Add const to parse_{commit,tag}_buffer()
@ 2011-02-05 10:52 Nguyễn Thái Ngọc Duy
  2011-02-05 10:52 ` [PATCH 2/2] Make hash-object more robust against malformed objects Nguyễn Thái Ngọc Duy
  0 siblings, 1 reply; 9+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2011-02-05 10:52 UTC (permalink / raw)
  To: git, Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy


Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 commit.c |    6 +++---
 commit.h |    2 +-
 tag.c    |    2 +-
 tag.h    |    2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/commit.c b/commit.c
index 74d6601..ac337c7 100644
--- a/commit.c
+++ b/commit.c
@@ -245,10 +245,10 @@ int unregister_shallow(const unsigned char *sha1)
 	return 0;
 }
 
-int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
+int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size)
 {
-	char *tail = buffer;
-	char *bufptr = buffer;
+	const char *tail = buffer;
+	const char *bufptr = buffer;
 	unsigned char parent[20];
 	struct commit_list **pptr;
 	struct commit_graft *graft;
diff --git a/commit.h b/commit.h
index eb6c5af..659c87c 100644
--- a/commit.h
+++ b/commit.h
@@ -38,7 +38,7 @@ struct commit *lookup_commit_reference_gently(const unsigned char *sha1,
 					      int quiet);
 struct commit *lookup_commit_reference_by_name(const char *name);
 
-int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size);
+int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);
 int parse_commit(struct commit *item);
 
 /* Find beginning and length of commit subject. */
diff --git a/tag.c b/tag.c
index f789744..ecf7c1e 100644
--- a/tag.c
+++ b/tag.c
@@ -56,7 +56,7 @@ static unsigned long parse_tag_date(const char *buf, const char *tail)
 	return strtoul(dateptr, NULL, 10);
 }
 
-int parse_tag_buffer(struct tag *item, void *data, unsigned long size)
+int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)
 {
 	unsigned char sha1[20];
 	char type[20];
diff --git a/tag.h b/tag.h
index 8522370..5ee88e6 100644
--- a/tag.h
+++ b/tag.h
@@ -13,7 +13,7 @@ struct tag {
 };
 
 extern struct tag *lookup_tag(const unsigned char *sha1);
-extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);
+extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long size);
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
 extern size_t parse_signature(const char *buf, unsigned long size);
-- 
1.7.3.4.878.g439c7

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] Make hash-object more robust against malformed objects
  2011-02-05 10:52 [PATCH 1/2] Add const to parse_{commit,tag}_buffer() Nguyễn Thái Ngọc Duy
@ 2011-02-05 10:52 ` Nguyễn Thái Ngọc Duy
  2011-02-12 11:42   ` Thomas Rast
  0 siblings, 1 reply; 9+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2011-02-05 10:52 UTC (permalink / raw)
  To: git, Junio C Hamano; +Cc: Nguyễn Thái Ngọc Duy

Commits, trees and tags have structure. Don't let users feed git
with malformed ones. Sooner or later git will die() when
encountering them.

Note that this patch does not check semantics. A tree that points
to non-existent objects is perfectly OK (and should be so, users
may choose to add commit first, then its associated tree for example)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/hash-object.c  |    2 +-
 cache.h                |    2 +-
 read-cache.c           |    2 +-
 sha1_file.c            |   54 +++++++++++++++++++++++++++++++++++++++++------
 t/t1007-hash-object.sh |   13 +++++++++++
 5 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 080af1a..c90acdd 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -15,7 +15,7 @@ static void hash_fd(int fd, const char *type, int write_object, const char *path
 	struct stat st;
 	unsigned char sha1[20];
 	if (fstat(fd, &st) < 0 ||
-	    index_fd(sha1, fd, &st, write_object, type_from_string(type), path))
+	    index_fd(sha1, fd, &st, write_object, type_from_string(type), path, 1))
 		die(write_object
 		    ? "Unable to add %s to database"
 		    : "Unable to hash %s", path);
diff --git a/cache.h b/cache.h
index d83d68c..9186a56 100644
--- a/cache.h
+++ b/cache.h
@@ -501,7 +501,7 @@ extern int ie_match_stat(const struct index_state *, struct cache_entry *, struc
 extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
 
 extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);
-extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);
+extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path, int format_check);
 extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);
 extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
 
diff --git a/read-cache.c b/read-cache.c
index 4f2e890..fbc12f3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -92,7 +92,7 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st)
 
 	if (fd >= 0) {
 		unsigned char sha1[20];
-		if (!index_fd(sha1, fd, st, 0, OBJ_BLOB, ce->name))
+		if (!index_fd(sha1, fd, st, 0, OBJ_BLOB, ce->name, 0))
 			match = hashcmp(sha1, ce->sha1);
 		/* index_fd() closed the file descriptor already */
 	}
diff --git a/sha1_file.c b/sha1_file.c
index d86a8db..58ca858 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -13,6 +13,7 @@
 #include "commit.h"
 #include "tag.h"
 #include "tree.h"
+#include "tree-walk.h"
 #include "refs.h"
 #include "pack-revindex.h"
 #include "sha1-lookup.h"
@@ -2471,8 +2472,37 @@ int has_sha1_file(const unsigned char *sha1)
 	return has_loose_object(sha1);
 }
 
+static void check_tree(const void *buf, size_t size)
+{
+	struct tree_desc desc;
+	struct name_entry entry;
+
+	init_tree_desc(&desc, buf, size);
+	while (tree_entry(&desc, &entry))
+		/* do nothing
+		 * tree_entry() will die() on malformed entries */
+		;
+}
+
+static void check_commit(const void *buf, size_t size)
+{
+	struct commit c;
+	memset(&c, 0, sizeof(c));
+	if (parse_commit_buffer(&c, buf, size))
+		die("corrupt commit");
+}
+
+static void check_tag(const void *buf, size_t size)
+{
+	struct tag t;
+	memset(&t, 0, sizeof(t));
+	if (parse_tag_buffer(&t, buf, size))
+		die("corrupt tag");
+}
+
 static int index_mem(unsigned char *sha1, void *buf, size_t size,
-		     int write_object, enum object_type type, const char *path)
+		     int write_object, enum object_type type,
+		     const char *path, int format_check)
 {
 	int ret, re_allocated = 0;
 
@@ -2490,6 +2520,14 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 			re_allocated = 1;
 		}
 	}
+	if (format_check) {
+		if (type == OBJ_TREE)
+			check_tree(buf, size);
+		if (type == OBJ_COMMIT)
+			check_commit(buf, size);
+		if (type == OBJ_TAG)
+			check_tag(buf, size);
+	}
 
 	if (write_object)
 		ret = write_sha1_file(buf, size, typename(type), sha1);
@@ -2503,7 +2541,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 #define SMALL_FILE_SIZE (32*1024)
 
 int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
-	     enum object_type type, const char *path)
+	     enum object_type type, const char *path, int format_check)
 {
 	int ret;
 	size_t size = xsize_t(st->st_size);
@@ -2512,23 +2550,25 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 		struct strbuf sbuf = STRBUF_INIT;
 		if (strbuf_read(&sbuf, fd, 4096) >= 0)
 			ret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,
-					type, path);
+					type, path, format_check);
 		else
 			ret = -1;
 		strbuf_release(&sbuf);
 	} else if (!size) {
-		ret = index_mem(sha1, NULL, size, write_object, type, path);
+		ret = index_mem(sha1, NULL, size, write_object, type, path,
+				format_check);
 	} else if (size <= SMALL_FILE_SIZE) {
 		char *buf = xmalloc(size);
 		if (size == read_in_full(fd, buf, size))
 			ret = index_mem(sha1, buf, size, write_object, type,
-					path);
+					path, format_check);
 		else
 			ret = error("short read %s", strerror(errno));
 		free(buf);
 	} else {
 		void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-		ret = index_mem(sha1, buf, size, write_object, type, path);
+		ret = index_mem(sha1, buf, size, write_object, type, path,
+				format_check);
 		munmap(buf, size);
 	}
 	close(fd);
@@ -2546,7 +2586,7 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write
 		if (fd < 0)
 			return error("open(\"%s\"): %s", path,
 				     strerror(errno));
-		if (index_fd(sha1, fd, st, write_object, OBJ_BLOB, path) < 0)
+		if (index_fd(sha1, fd, st, write_object, OBJ_BLOB, path, 0) < 0)
 			return error("%s: failed to insert into database",
 				     path);
 		break;
diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh
index dd32432..6d52b82 100755
--- a/t/t1007-hash-object.sh
+++ b/t/t1007-hash-object.sh
@@ -188,4 +188,17 @@ for args in "-w --stdin-paths" "--stdin-paths -w"; do
 	pop_repo
 done
 
+test_expect_success 'corrupt tree' '
+	echo abc >malformed-tree
+	test_must_fail git hash-object -t tree malformed-tree
+'
+
+test_expect_success 'corrupt commit' '
+	test_must_fail git hash-object -t commit --stdin </dev/null
+'
+
+test_expect_success 'corrupt tag' '
+	test_must_fail git hash-object -t tag --stdin </dev/null
+'
+
 test_done
-- 
1.7.3.4.878.g439c7

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] Make hash-object more robust against malformed objects
  2011-02-05 10:52 ` [PATCH 2/2] Make hash-object more robust against malformed objects Nguyễn Thái Ngọc Duy
@ 2011-02-12 11:42   ` Thomas Rast
  2011-02-12 14:47     ` Nguyen Thai Ngoc Duy
  0 siblings, 1 reply; 9+ messages in thread
From: Thomas Rast @ 2011-02-12 11:42 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano

Nguyễn Thái Ngọc Duy wrote:
> Commits, trees and tags have structure. Don't let users feed git
> with malformed ones. Sooner or later git will die() when
> encountering them.
> 
> Note that this patch does not check semantics. A tree that points
> to non-existent objects is perfectly OK (and should be so, users
> may choose to add commit first, then its associated tree for example)
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This patch makes t9350 fail under valgrind:

expecting success: 

        TAG=$(git hash-object -t tag -w tag-content) &&
        git update-ref refs/tags/sonnenschein $TAG &&
        git fast-export -C -C --signed-tags=strip --all > output &&
        test $(grep -c "^tag " output) = 4 &&
        ! grep "Unspecified Tagger" output &&
        git fast-export -C -C --signed-tags=strip --all \
                --fake-missing-tagger > output &&
        test $(grep -c "^tag " output) = 4 &&
        grep "Unspecified Tagger" output


==2862== Invalid read of size 1
==2862==    at 0x4F0C34: prefixcmp (strbuf.c:9)
==2862==    by 0x4F4FB3: parse_tag_buffer (tag.c:109)
==2862==    by 0x4EC686: check_tag (sha1_file.c:2499)
==2862==    by 0x4EC77F: index_mem (sha1_file.c:2529)
==2862==    by 0x4EC934: index_fd (sha1_file.c:2563)
==2862==    by 0x4379C9: hash_fd (hash-object.c:17)
==2862==    by 0x437A86: hash_object (hash-object.c:33)
==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)
==2862==    by 0x404771: run_builtin (git.c:290)
==2862==    by 0x4048FC: handle_internal_command (git.c:448)
==2862==    by 0x4049E7: run_argv (git.c:492)
==2862==    by 0x404B43: main (git.c:565)
==2862==  Address 0x55abb77 is 0 bytes after a block of size 71 alloc'd
==2862==    at 0x4C20E1C: malloc (vg_replace_malloc.c:195)
==2862==    by 0x504A7C: xmalloc (wrapper.c:35)
==2862==    by 0x4EC8E8: index_fd (sha1_file.c:2561)
==2862==    by 0x4379C9: hash_fd (hash-object.c:17)
==2862==    by 0x437A86: hash_object (hash-object.c:33)
==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)
==2862==    by 0x404771: run_builtin (git.c:290)
==2862==    by 0x4048FC: handle_internal_command (git.c:448)
==2862==    by 0x4049E7: run_argv (git.c:492)
==2862==    by 0x404B43: main (git.c:565)
==2862== 
==2862== Invalid read of size 1
==2862==    at 0x4F0C50: prefixcmp (strbuf.c:10)
==2862==    by 0x4F4FB3: parse_tag_buffer (tag.c:109)
==2862==    by 0x4EC686: check_tag (sha1_file.c:2499)
==2862==    by 0x4EC77F: index_mem (sha1_file.c:2529)
==2862==    by 0x4EC934: index_fd (sha1_file.c:2563)
==2862==    by 0x4379C9: hash_fd (hash-object.c:17)
==2862==    by 0x437A86: hash_object (hash-object.c:33)
==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)
==2862==    by 0x404771: run_builtin (git.c:290)
==2862==    by 0x4048FC: handle_internal_command (git.c:448)
==2862==    by 0x4049E7: run_argv (git.c:492)
==2862==    by 0x404B43: main (git.c:565)
==2862==  Address 0x55abb77 is 0 bytes after a block of size 71 alloc'd
==2862==    at 0x4C20E1C: malloc (vg_replace_malloc.c:195)
==2862==    by 0x504A7C: xmalloc (wrapper.c:35)
==2862==    by 0x4EC8E8: index_fd (sha1_file.c:2561)
==2862==    by 0x4379C9: hash_fd (hash-object.c:17)
==2862==    by 0x437A86: hash_object (hash-object.c:33)
==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)
==2862==    by 0x404771: run_builtin (git.c:290)
==2862==    by 0x4048FC: handle_internal_command (git.c:448)
==2862==    by 0x4049E7: run_argv (git.c:492)
==2862==    by 0x404B43: main (git.c:565)
==2862== 
not ok - 15 cope with tagger-less tags
#       
#       
#               TAG=$(git hash-object -t tag -w tag-content) &&
#               git update-ref refs/tags/sonnenschein $TAG &&
#               git fast-export -C -C --signed-tags=strip --all > output &&
#               test $(grep -c "^tag " output) = 4 &&
#               ! grep "Unspecified Tagger" output &&
#               git fast-export -C -C --signed-tags=strip --all \
#                       --fake-missing-tagger > output &&
#               test $(grep -c "^tag " output) = 4 &&
#               grep "Unspecified Tagger" output
#       
#       

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] Make hash-object more robust against malformed objects
  2011-02-12 11:42   ` Thomas Rast
@ 2011-02-12 14:47     ` Nguyen Thai Ngoc Duy
  2011-02-14 13:02       ` [PATCH] parse_tag_buffer(): do not prefixcmp() out of range Nguyễn Thái Ngọc Duy
  0 siblings, 1 reply; 9+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2011-02-12 14:47 UTC (permalink / raw)
  To: Thomas Rast; +Cc: git, Junio C Hamano

On Sat, Feb 12, 2011 at 12:42:21PM +0100, Thomas Rast wrote:
> This patch makes t9350 fail under valgrind:
> 
> expecting success: 
> 
>         TAG=$(git hash-object -t tag -w tag-content) &&
>         git update-ref refs/tags/sonnenschein $TAG &&
>         git fast-export -C -C --signed-tags=strip --all > output &&
>         test $(grep -c "^tag " output) = 4 &&
>         ! grep "Unspecified Tagger" output &&
>         git fast-export -C -C --signed-tags=strip --all \
>                 --fake-missing-tagger > output &&
>         test $(grep -c "^tag " output) = 4 &&
>         grep "Unspecified Tagger" output
> 
> 
> ==2862== Invalid read of size 1
> ==2862==    at 0x4F0C34: prefixcmp (strbuf.c:9)
> ==2862==    by 0x4F4FB3: parse_tag_buffer (tag.c:109)

Nice. Does this fix it?

--8<--
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 + 5 < 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 + 8 < tail && !prefixcmp(bufptr, "tagger "))
 		item->date = parse_tag_date(bufptr, tail);
 	else
 		item->date = 0;
--8<--
-- 
Duy

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH] parse_tag_buffer(): do not prefixcmp() out of range
  2011-02-12 14:47     ` Nguyen Thai Ngoc Duy
@ 2011-02-14 13:02       ` Nguyễn Thái Ngọc Duy
  2011-02-15 21:18         ` Junio C Hamano
  0 siblings, 1 reply; 9+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2011-02-14 13:02 UTC (permalink / raw)
  To: git, Junio C Hamano, Thomas Rast; +Cc: Nguyễn Thái Ngọc Duy

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.

Anyway accessing out of range is not good. Avoid it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Unfortunately I installed valgrind but could not reproduce t9350.15
 failure.

 Another option is to add prefixncmp(const char *, const char *,int).
 Probably not worth it.

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

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);
@@ -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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range
  2011-02-14 13:02       ` [PATCH] parse_tag_buffer(): do not prefixcmp() out of range Nguyễn Thái Ngọc Duy
@ 2011-02-15 21:18         ` Junio C Hamano
  2011-02-16  3:39           ` Nguyen Thai Ngoc Duy
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2011-02-15 21:18 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano, Thomas Rast

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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range
  2011-02-15 21:18         ` Junio C Hamano
@ 2011-02-16  3:39           ` Nguyen Thai Ngoc Duy
  2011-02-17 12:43             ` René Scharfe
  0 siblings, 1 reply; 9+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2011-02-16  3:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Thomas Rast

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range
  2011-02-16  3:39           ` Nguyen Thai Ngoc Duy
@ 2011-02-17 12:43             ` René Scharfe
  2011-02-18 12:49               ` [PATCH] parse_tag_buffer(): avoid out of bound access Nguyễn Thái Ngọc Duy
  0 siblings, 1 reply; 9+ messages in thread
From: René Scharfe @ 2011-02-17 12:43 UTC (permalink / raw)
  To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, git, Thomas Rast

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é

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] parse_tag_buffer(): avoid out of bound access
  2011-02-17 12:43             ` René Scharfe
@ 2011-02-18 12:49               ` Nguyễn Thái Ngọc Duy
  0 siblings, 0 replies; 9+ messages in thread
From: Nguyễn Thái Ngọc Duy @ 2011-02-18 12:49 UTC (permalink / raw)
  To: git, René Scharfe, Thomas Rast, Junio C Hamano
  Cc: Nguyễn Thái Ngọc Duy

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 <pclouds@gmail.com>
---
 On Thu, Feb 17, 2011 at 7:43 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> 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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2011-02-18 12:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-05 10:52 [PATCH 1/2] Add const to parse_{commit,tag}_buffer() Nguyễn Thái Ngọc Duy
2011-02-05 10:52 ` [PATCH 2/2] Make hash-object more robust against malformed objects Nguyễn Thái Ngọc Duy
2011-02-12 11:42   ` Thomas Rast
2011-02-12 14:47     ` Nguyen Thai Ngoc Duy
2011-02-14 13:02       ` [PATCH] parse_tag_buffer(): do not prefixcmp() out of range Nguyễn Thái Ngọc Duy
2011-02-15 21:18         ` Junio C Hamano
2011-02-16  3:39           ` Nguyen Thai Ngoc Duy
2011-02-17 12:43             ` René Scharfe
2011-02-18 12:49               ` [PATCH] parse_tag_buffer(): avoid out of bound access Nguyễn Thái Ngọc Duy

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).