[PATCH 01/12] read_object_with_reference: don't read beyond the buffer

[PATCH 01/12] read_object_with_reference: don't read beyond the buffer

[Martin Koegler]

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 sha1_file.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4179949..d9da7c8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1943,7 +1943,8 @@ void *read_object_with_reference(const unsigned char *sha1,
 		}
 		ref_length = strlen(ref_type);
 
-		if (memcmp(buffer, ref_type, ref_length) ||
+		if (ref_length + 40 > isize ||
+		    memcmp(buffer, ref_type, ref_length) ||
 		    get_sha1_hex((char *) buffer + ref_length, actual_sha1)) {
 			free(buffer);
 			return NULL;
-- 
1.5.4.1.g96b77

[PATCH 02/12] get_sha1_oneline: check return value of parse_object

[Martin Koegler]

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 sha1_name.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index ed3c867..f8506bf 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -620,7 +620,8 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1)
 		unsigned long size;
 
 		commit = pop_most_recent_commit(&list, ONELINE_SEEN);
-		parse_object(commit->object.sha1);
+		if (!parse_object(commit->object.sha1))
+			continue;
 		if (temp_commit_buffer)
 			free(temp_commit_buffer);
 		if (commit->buffer)
-- 
1.5.4.1.g96b77

[PATCH 03/12] mark_blob/tree_uninteresting: check for NULL

[Martin Koegler]

As these functions are directly called with the result
from lookup_tree/blob, they must handle NULL.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 revision.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 6e85aaa..484e5e7 100644
--- a/revision.c
+++ b/revision.c
@@ -46,6 +46,8 @@ void add_object(struct object *obj,
 
 static void mark_blob_uninteresting(struct blob *blob)
 {
+	if (!blob)
+		return;
 	if (blob->object.flags & UNINTERESTING)
 		return;
 	blob->object.flags |= UNINTERESTING;
@@ -57,6 +59,8 @@ void mark_tree_uninteresting(struct tree *tree)
 	struct name_entry entry;
 	struct object *obj = &tree->object;
 
+	if (!tree)
+		return;
 	if (obj->flags & UNINTERESTING)
 		return;
 	obj->flags |= UNINTERESTING;
-- 
1.5.4.1.g96b77

[PATCH 04/12] add_one_tree: handle NULL from lookup_tree

[Martin Koegler]

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 reachable.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/reachable.c b/reachable.c
index 823e324..937af57 100644
--- a/reachable.c
+++ b/reachable.c
@@ -150,7 +150,8 @@ static int add_one_reflog(const char *path, const unsigned char *sha1, int flag,
 static void add_one_tree(const unsigned char *sha1, struct rev_info *revs)
 {
 	struct tree *tree = lookup_tree(sha1);
-	add_pending_object(revs, &tree->object, "");
+	if (tree)
+		add_pending_object(revs, &tree->object, "");
 }
 
 static void add_cache_tree(struct cache_tree *it, struct rev_info *revs)
-- 
1.5.4.1.g96b77

[PATCH 05/12] process_tree/blob: check for NULL

[Martin Koegler]

As these functions are directly called with the result
from lookup_tree/blob, they must handle NULL.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 list-objects.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/list-objects.c b/list-objects.c
index 4ef58e7..c8b8375 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -18,6 +18,8 @@ static void process_blob(struct rev_info *revs,
 
 	if (!revs->blob_objects)
 		return;
+	if (!obj)
+		die("bad blob object");
 	if (obj->flags & (UNINTERESTING | SEEN))
 		return;
 	obj->flags |= SEEN;
@@ -69,6 +71,8 @@ static void process_tree(struct rev_info *revs,
 
 	if (!revs->tree_objects)
 		return;
+	if (!obj)
+		die("bad tree object");
 	if (obj->flags & (UNINTERESTING | SEEN))
 		return;
 	if (parse_tree(tree) < 0)
-- 
1.5.4.1.g96b77

[PATCH 06/12] check results of parse_commit in merge_bases

[Martin Koegler]

An error is signaled by returning NULL.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 commit.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/commit.c b/commit.c
index 8b8fb04..70f1266 100644
--- a/commit.c
+++ b/commit.c
@@ -552,8 +552,10 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)
 		 */
 		return commit_list_insert(one, &result);
 
-	parse_commit(one);
-	parse_commit(two);
+	if (parse_commit(one))
+		return NULL;
+	if (parse_commit(two))
+		return NULL;
 
 	one->object.flags |= PARENT1;
 	two->object.flags |= PARENT2;
@@ -586,7 +588,8 @@ static struct commit_list *merge_bases(struct commit *one, struct commit *two)
 			parents = parents->next;
 			if ((p->object.flags & flags) == flags)
 				continue;
-			parse_commit(p);
+			if (parse_commit(p))
+				return NULL;
 			p->object.flags |= flags;
 			insert_by_date(p, &list);
 		}
-- 
1.5.4.1.g96b77

[PATCH 07/12] peel_onion: handle NULL

[Martin Koegler]

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 sha1_name.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index f8506bf..c2805e7 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -494,8 +494,11 @@ static int peel_onion(const char *name, int len, unsigned char *sha1)
 				return error("%.*s: expected %s type, but the object dereferences to %s type",
 					     len, name, typename(expected_type),
 					     typename(o->type));
+			if (!o)
+				return -1;
 			if (!o->parsed)
-				parse_object(o->sha1);
+				if (!parse_object(o->sha1))
+					return -1;
 		}
 	}
 	return 0;
-- 
1.5.4.1.g96b77

[PATCH 08/12] process_tag: handle tag->tagged == NULL

[Martin Koegler]

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 reachable.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/reachable.c b/reachable.c
index 937af57..339be7f 100644
--- a/reachable.c
+++ b/reachable.c
@@ -79,7 +79,8 @@ static void process_tag(struct tag *tag, struct object_array *p, const char *nam
 
 	if (parse_tag(tag) < 0)
 		die("bad tag object %s", sha1_to_hex(obj->sha1));
-	add_object(tag->tagged, p, NULL, name);
+	if (tag->tagged)
+		add_object(tag->tagged, p, NULL, name);
 }
 
 static void walk_commit_list(struct rev_info *revs)
-- 
1.5.4.1.g96b77

[PATCH 09/12] process_tree/blob: check for NULL

[Martin Koegler]

As these functions are directly called with the result
from lookup_tree/blob, they must handle NULL.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 reachable.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/reachable.c b/reachable.c
index 339be7f..3b1c18f 100644
--- a/reachable.c
+++ b/reachable.c
@@ -15,6 +15,8 @@ static void process_blob(struct blob *blob,
 {
 	struct object *obj = &blob->object;
 
+	if (!blob)
+		die("bad blob object");
 	if (obj->flags & SEEN)
 		return;
 	obj->flags |= SEEN;
@@ -39,6 +41,8 @@ static void process_tree(struct tree *tree,
 	struct name_entry entry;
 	struct name_path me;
 
+	if (!tree)
+		die("bad tree object");
 	if (obj->flags & SEEN)
 		return;
 	obj->flags |= SEEN;
-- 
1.5.4.1.g96b77

[PATCH 10/12] revision.c: handle tag->tagged == NULL

[Martin Koegler]

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 revision.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/revision.c b/revision.c
index 484e5e7..b1aebf8 100644
--- a/revision.c
+++ b/revision.c
@@ -177,6 +177,8 @@ static struct commit *handle_commit(struct rev_info *revs, struct object *object
 		struct tag *tag = (struct tag *) object;
 		if (revs->tag_objects && !(flags & UNINTERESTING))
 			add_pending_object(revs, object, tag->tag);
+		if (!tag->tagged)
+			die("bad tag");
 		object = parse_object(tag->tagged->sha1);
 		if (!object)
 			die("bad object %s", sha1_to_hex(tag->tagged->sha1));
@@ -689,6 +691,8 @@ static int add_parents_only(struct rev_info *revs, const char *arg, int flags)
 		it = get_reference(revs, arg, sha1, 0);
 		if (it->type != OBJ_TAG)
 			break;
+		if (!((struct tag*)it)->tagged)
+			return 0;
 		hashcpy(sha1, ((struct tag*)it)->tagged->sha1);
 	}
 	if (it->type != OBJ_COMMIT)
-- 
1.5.4.1.g96b77

[PATCH 11/12] parse_commit: don't fail, if object is NULL

[Martin Koegler]

Some codepaths (eg. builtin-rev-parse -> get_merge_bases -> parse_commit)
can pass NULL.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 commit.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/commit.c b/commit.c
index 70f1266..5d57450 100644
--- a/commit.c
+++ b/commit.c
@@ -311,6 +311,8 @@ int parse_commit(struct commit *item)
 	unsigned long size;
 	int ret;
 
+	if (!item)
+		return -1;
 	if (item->object.parsed)
 		return 0;
 	buffer = read_sha1_file(item->object.sha1, &type, &size);
-- 
1.5.4.1.g96b77

[PATCH 12/12] check return value from parse_commit

[Martin Koegler]

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 commit.c      |    3 +--
 shallow.c     |    3 ++-
 upload-pack.c |    3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/commit.c b/commit.c
index 5d57450..22ce776 100644
--- a/commit.c
+++ b/commit.c
@@ -387,8 +387,7 @@ struct commit *pop_most_recent_commit(struct commit_list **list,
 
 	while (parents) {
 		struct commit *commit = parents->item;
-		parse_commit(commit);
-		if (!(commit->object.flags & mark)) {
+		if (!parse_commit(commit) && !(commit->object.flags & mark)) {
 			commit->object.flags |= mark;
 			insert_by_date(commit, list);
 		}
diff --git a/shallow.c b/shallow.c
index 212e62b..4d90eda 100644
--- a/shallow.c
+++ b/shallow.c
@@ -70,7 +70,8 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 				cur_depth = *(int *)commit->util;
 			}
 		}
-		parse_commit(commit);
+		if (parse_commit(commit))
+			die("invalid commit");
 		commit->object.flags |= not_shallow_flag;
 		cur_depth++;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
diff --git a/upload-pack.c b/upload-pack.c
index 53676ee..20d5462 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -534,7 +534,8 @@ static void receive_needs(void)
 				/* make sure the real parents are parsed */
 				unregister_shallow(object->sha1);
 				object->parsed = 0;
-				parse_commit((struct commit *)object);
+				if (parse_commit((struct commit *)object))
+					die("invalid commit");
 				parents = ((struct commit *)object)->parents;
 				while (parents) {
 					add_object_array(&parents->item->object,
-- 
1.5.4.1.g96b77

Re: [PATCH 05/12] process_tree/blob: check for NULL

[Junio C Hamano]

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

> As these functions are directly called with the result
> from lookup_tree/blob, they must handle NULL.
>
> Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> ---
>  list-objects.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/list-objects.c b/list-objects.c
> index 4ef58e7..c8b8375 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -18,6 +18,8 @@ static void process_blob(struct rev_info *revs,
>  
>  	if (!revs->blob_objects)
>  		return;
> +	if (!obj)
> +		die("bad blob object");
>  	if (obj->flags & (UNINTERESTING | SEEN))
>  		return;
>  	obj->flags |= SEEN;
> @@ -69,6 +71,8 @@ static void process_tree(struct rev_info *revs,
>  
>  	if (!revs->tree_objects)
>  		return;
> +	if (!obj)
> +		die("bad tree object");
>  	if (obj->flags & (UNINTERESTING | SEEN))
>  		return;
>  	if (parse_tree(tree) < 0)

I think these are in line with process_tree() that barfs like this:

	if (parse_tree(tree) < 0)
		die("bad tree object %s", sha1_to_hex(obj->sha1));

in the existing codepath, but these new die() callsites lose
information.

It would be nicer if we can report what entry (name) in which
tree object (sha1) lead to this die().  The same comment applies
to [09/12].

Nevertheless, this is an improvement compared to accessing
NULL->flags and dying with segv.  I'll queue.

Thanks.