[PATCH] checkout, clone: die if tree cannot be parsed
- From
Glen Choo <chooglen@google.com>
- Date
- Mar 2, 2022, 00:36 UTC
- Message-ID
- <20220302003613.15567-1-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