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

[Jim Meyering <jim@meyering.net>]

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