Re: [PATCH] Don't dereference NULL upon lookup_tree failure.

From: Jim Meyering <jim@meyering.net>
Date: Dec 22, 2007, 13:41 UTC
Message-ID: <87prwyu9r0.fsf@rho.meyering.net>
In-Reply-To: <7vzlw3xeqy.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> wrote: ...

> Your fix is all we can sensibly do. I however think you would
> need similar fix to the same function for other object types, as
> they dereference a potentially NULL pointer the same way.

Good point. Patch below. I wondered if these lookup functions have always been able to return NULL, since there are many remaining uses where the possibility of a NULL return value is not handled. Here are a few that stood out in the output of a quick search:

  git-grep -E -A3 'lookup_(commit|tag|blob|tree) *\('

Note: I did not look at cases where the return value is an argument
to some other function.

------------------
builtin-fmt-merge-msg.c:
		head = lookup_commit(head_sha1);
		...
		for (i = 0; i < origins.nr; i++)
			shortlog(origins.list[i], origins.payload[i],
					head, &rev, limit);
------------------
builtin-read-tree.c:                    struct tree *subtree = lookup_tree(entry.
sha1);
builtin-read-tree.c-                    if (!subtree->object.parsed)

------------------ combine-diff.c: struct commit *commit = lookup_commit(sha1); combine-diff.c- struct commit_list *parents;

------------------ http-push.c: struct commit *head = lookup_commit(head_sha1); http-push.c: struct commit *branch = lookup_commit(branch_sha1); http-push.c- struct commit_list *merge_bases = get_merge_bases(head, branch, 1);

------------------ reachable.c: struct tree *tree = lookup_tree(sha1); reachable.c- add_pending_object(revs, &tree->object, "");

------------------ tag.c: item->tagged = &lookup_blob(sha1)->object; tag.c- } else if (!strcmp(type, tree_type)) { tag.c: item->tagged = &lookup_tree(sha1)->object; tag.c- } else if (!strcmp(type, commit_type)) { tag.c: item->tagged = &lookup_commit(sha1)->object; tag.c- } else if (!strcmp(type, tag_type)) { tag.c: item->tagged = &lookup_tag(sha1)->object; tag.c- } else {

-------------------------- unpack-trees.c: struct tree *tree = lookup_tree(posns[i]->sha1); ... unpack-trees.c- parse_tree(tree);

Here's the revised patch:

==================================================================
From 94152217e8e57d3932b4ba6f7ee014da1f4346d3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Fri, 21 Dec 2007 11:56:32 +0100
Subject: [PATCH] Don't dereference NULL upon lookup failure.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 object.c |   42 +++++++++++++++++++++++++++++-------------
 1 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/object.c b/object.c
index 16793d9..9945b25 100644
--- a/object.c
+++ b/object.c
@@ -138,27 +138,43 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t

 	if (type == OBJ_BLOB) {
 		struct blob *blob = lookup_blob(sha1);
-		parse_blob_buffer(blob, buffer, size);
-		obj = &blob->object;
+		if (!blob)
+		    obj = NULL;
+		else {
+		    parse_blob_buffer(blob, buffer, size);
+		    obj = &blob->object;
+		}
 	} else if (type == OBJ_TREE) {
 		struct tree *tree = lookup_tree(sha1);
-		obj = &tree->object;
-		if (!tree->object.parsed) {
-			parse_tree_buffer(tree, buffer, size);
-			eaten = 1;
+		if (!tree)
+		    obj = NULL;
+		else {
+		    obj = &tree->object;
+		    if (!tree->object.parsed) {
+			    parse_tree_buffer(tree, buffer, size);
+			    eaten = 1;
+		    }
 		}
 	} else if (type == OBJ_COMMIT) {
 		struct commit *commit = lookup_commit(sha1);
-		parse_commit_buffer(commit, buffer, size);
-		if (!commit->buffer) {
-			commit->buffer = buffer;
-			eaten = 1;
+		if (!commit)
+		    obj = NULL;
+		else {
+		    parse_commit_buffer(commit, buffer, size);
+		    if (!commit->buffer) {
+			    commit->buffer = buffer;
+			    eaten = 1;
+		    }
+		    obj = &commit->object;
 		}
-		obj = &commit->object;
 	} else if (type == OBJ_TAG) {
 		struct tag *tag = lookup_tag(sha1);
-		parse_tag_buffer(tag, buffer, size);
-		obj = &tag->object;
+		if (!tag)
+		    obj = NULL;
+		else {
+		    parse_tag_buffer(tag, buffer, size);
+		    obj = &tag->object;
+		}
 	} else {
 		warning("object %s has unknown type id %d\n", sha1_to_hex(sha1), type);
 		obj = NULL;
--
1.5.4.rc1.16.g60f3b