[PATCH] checkout, clone: die if tree cannot be parsed

[Glen Choo <chooglen@google.com>]

When a tree oid is invalid, parse_tree_indirect() can return NULL. Check
for NULL instead of proceeding as though it were a valid pointer and
segfaulting.

Signed-off-by: Glen Choo <chooglen@google.com>
---
At $DAYJOB, this bug was discovered due to some interactions between
"git clone --filter=tree:0" and a buggy server that failed to transfer
certain commits.

In the 'checkout' step of "git clone --filter=tree:0", the repo tries to
get the HEAD commit from the server (since it's not present locally),
but this fails due to an unrelated bug in the server. Since the commit
tree is invalid, parse_tree_indirect() returns NULL, causing
parse_tree(NULL) to segfault.

I tried to write a test for this segfault, but I couldn't quite figure
out how:

- Invalid trees are typically caught pretty early, so I suspect that any
  reproduction scenario would need to replicate the partial clone +
  buggy server setup.
- I couldn't figure out how to replicate the aforementioned buggy setup

I'd appreciate any suggestions on how to test this though :)

Note that there are many other callsites that don't check for NULLs from
parse_tree_indirect(), and some of which are fairly subtle. I wasn't
confident in changing those, so I stayed on the conservative side and
only changed the ones that I could get to segfault.

 builtin/checkout.c | 13 ++++++++++---
 builtin/clone.c    |  2 ++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index d9b31bbb6d..c1035304a5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -738,6 +738,7 @@
 		struct tree_desc trees[2];
 		struct tree *tree;
 		struct unpack_trees_options topts;
+		const struct object_id *old_commit_oid;
 
 		memset(&topts, 0, sizeof(topts));
 		topts.head_idx = -1;
@@ -765,9 +766,15 @@
 				       &new_branch_info->commit->object.oid :
 				       &new_branch_info->oid, NULL);
 		topts.preserve_ignored = !opts->overwrite_ignore;
-		tree = parse_tree_indirect(old_branch_info->commit ?
-					   &old_branch_info->commit->object.oid :
-					   the_hash_algo->empty_tree);
+
+		old_commit_oid = old_branch_info->commit ?
+			&old_branch_info->commit->object.oid :
+			the_hash_algo->empty_tree;
+		tree = parse_tree_indirect(old_commit_oid);
+		if (!tree)
+			die(_("unable to parse commit %s"),
+				oid_to_hex(old_commit_oid));
+
 		init_tree_desc(&trees[0], tree->buffer, tree->size);
 		parse_tree(new_tree);
 		tree = new_tree;
diff --git a/builtin/clone.c b/builtin/clone.c
index a572cda503..0aea177660 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -700,6 +700,8 @@
 	init_checkout_metadata(&opts.meta, head, &oid, NULL);
 
 	tree = parse_tree_indirect(&oid);
+	if (!tree)
+		die(_("unable to parse commit %s"), oid_to_hex(&oid));
 	parse_tree(tree);
 	init_tree_desc(&t, tree->buffer, tree->size);
 	if (unpack_trees(1, &t, &opts) < 0)

base-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7
-- 
2.33.GIT