threads / patch / 12182

patch, 12 partsadd_one_tree: handle NULL from lookup_tree

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

## tl;dr

13 messages between Feb 18, 2008 and Feb 19, 2008. Diffs are folded; open one to read it.

replies: 12people: 2as markdown or json

Martin Koegler· Feb 18, 2008, 20:47 UTC · lore

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

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 sha1_file.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to sha1_file.c +2 −1
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
Martin Koegler· Feb 18, 2008, 20:47 UTC · re: Martin Koegler · lore

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

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 sha1_name.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to sha1_name.c +2 −1
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
Martin Koegler· Feb 18, 2008, 20:47 UTC · re: Martin Koegler · lore

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

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(-)
Show changes to revision.c +4 −0
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
Martin Koegler· Feb 18, 2008, 20:47 UTC · re: Martin Koegler · lore
Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 reachable.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to reachable.c +2 −1
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
Martin Koegler· Feb 18, 2008, 20:47 UTC · re: Martin Koegler · lore

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

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(-)
Show changes to list-objects.c +4 −0
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
Martin Koegler· Feb 18, 2008, 20:47 UTC · re: Martin Koegler · lore

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

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(-)
Show changes to commit.c +6 −3
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
Martin Koegler· Feb 18, 2008, 20:47 UTC · re: Martin Koegler · lore

[PATCH 07/12] peel_onion: handle NULL

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 sha1_name.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Show changes to sha1_name.c +4 −1
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
Martin Koegler· Feb 18, 2008, 20:47 UTC · re: Martin Koegler · lore

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

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 reachable.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to reachable.c +2 −1
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
Martin Koegler· Feb 18, 2008, 20:48 UTC · re: Martin Koegler · lore

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

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(-)
Show changes to reachable.c +4 −0
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
Martin Koegler· Feb 18, 2008, 20:48 UTC · re: Martin Koegler · lore

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

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 revision.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Show changes to revision.c +4 −0
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
Martin Koegler· Feb 18, 2008, 20:48 UTC · re: Martin Koegler · lore

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

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(-)
Show changes to commit.c +2 −0
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
Martin Koegler· Feb 18, 2008, 20:48 UTC · re: Martin Koegler · lore

[PATCH 12/12] check return value from parse_commit

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(-)
Show changes to 3 files +5 −4

commit.c, shallow.c, upload-pack.c

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
Junio C Hamano· Feb 19, 2008, 21:31 UTC · re: Martin Koegler · lore

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

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
Show 30 quoted lines
> 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.

← back to recent threads