{"thread":{"id":"57493","subject":"[PATCH] checkout, clone: die if tree cannot be parsed","startedAt":"2022-03-02T00:36:29Z","lastAt":"2022-03-09T22:20:43Z","messageCount":4,"participants":["Glen Choo","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"450029","messageId":"20220302003613.15567-1-chooglen@google.com","threadId":"57493","inReplyTo":null,"subject":"[PATCH] checkout, clone: die if tree cannot be parsed","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-03-02T00:36:13Z","receivedAt":"2022-03-02T00:36:29Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"When a tree oid is invalid, parse_tree_indirect() can return NULL. Check\nfor NULL instead of proceeding as though it were a valid pointer and\nsegfaulting.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\nAt $DAYJOB, this bug was discovered due to some interactions between\n\"git clone --filter=tree:0\" and a buggy server that failed to transfer\ncertain commits.\n\nIn the 'checkout' step of \"git clone --filter=tree:0\", the repo tries to\nget the HEAD commit from the server (since it's not present locally),\nbut this fails due to an unrelated bug in the server. Since the commit\ntree is invalid, parse_tree_indirect() returns NULL, causing\nparse_tree(NULL) to segfault.\n\nI tried to write a test for this segfault, but I couldn't quite figure\nout how:\n\n- Invalid trees are typically caught pretty early, so I suspect that any\n  reproduction scenario would need to replicate the partial clone +\n  buggy server setup.\n- I couldn't figure out how to replicate the aforementioned buggy setup\n\nI'd appreciate any suggestions on how to test this though :)\n\nNote that there are many other callsites that don't check for NULLs from\nparse_tree_indirect(), and some of which are fairly subtle. I wasn't\nconfident in changing those, so I stayed on the conservative side and\nonly changed the ones that I could get to segfault.\n\n builtin/checkout.c | 13 ++++++++++---\n builtin/clone.c    |  2 ++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex d9b31bbb6d..c1035304a5 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -738,6 +738,7 @@\n \t\tstruct tree_desc trees[2];\n \t\tstruct tree *tree;\n \t\tstruct unpack_trees_options topts;\n+\t\tconst struct object_id *old_commit_oid;\n \n \t\tmemset(&topts, 0, sizeof(topts));\n \t\ttopts.head_idx = -1;\n@@ -765,9 +766,15 @@\n \t\t\t\t       &new_branch_info->commit->object.oid :\n \t\t\t\t       &new_branch_info->oid, NULL);\n \t\ttopts.preserve_ignored = !opts->overwrite_ignore;\n-\t\ttree = parse_tree_indirect(old_branch_info->commit ?\n-\t\t\t\t\t   &old_branch_info->commit->object.oid :\n-\t\t\t\t\t   the_hash_algo->empty_tree);\n+\n+\t\told_commit_oid = old_branch_info->commit ?\n+\t\t\t&old_branch_info->commit->object.oid :\n+\t\t\tthe_hash_algo->empty_tree;\n+\t\ttree = parse_tree_indirect(old_commit_oid);\n+\t\tif (!tree)\n+\t\t\tdie(_(\"unable to parse commit %s\"),\n+\t\t\t\toid_to_hex(old_commit_oid));\n+\n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n \t\tparse_tree(new_tree);\n \t\ttree = new_tree;\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a572cda503..0aea177660 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -700,6 +700,8 @@\n \tinit_checkout_metadata(&opts.meta, head, &oid, NULL);\n \n \ttree = parse_tree_indirect(&oid);\n+\tif (!tree)\n+\t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&oid));\n \tparse_tree(tree);\n \tinit_tree_desc(&t, tree->buffer, tree->size);\n \tif (unpack_trees(1, &t, &opts) < 0)\n\nbase-commit: 715d08a9e51251ad8290b181b6ac3b9e1f9719d7\n-- \n2.33.GIT\n\n"},{"id":"450062","messageId":"xmqq5yowolvs.fsf@gitster.g","threadId":"57493","inReplyTo":"20220302003613.15567-1-chooglen@google.com","subject":"Re: [PATCH] checkout, clone: die if tree cannot be parsed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-02T07:26:47Z","receivedAt":"2022-03-02T07:26:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> Note that there are many other callsites that don't check for NULLs from\n> parse_tree_indirect(), and some of which are fairly subtle. I wasn't\n> confident in changing those, so I stayed on the conservative side and\n> only changed the ones that I could get to segfault.\n\nThanks.\n\n> -\t\ttree = parse_tree_indirect(old_branch_info->commit ?\n> -\t\t\t\t\t   &old_branch_info->commit->object.oid :\n> -\t\t\t\t\t   the_hash_algo->empty_tree);\n> +\n> +\t\told_commit_oid = old_branch_info->commit ?\n> +\t\t\t&old_branch_info->commit->object.oid :\n> +\t\t\tthe_hash_algo->empty_tree;\n\nI guess this is done only so that you can use the object name in the\nerror message, which is fine.\n\n> +\t\ttree = parse_tree_indirect(old_commit_oid);\n> +\t\tif (!tree)\n> +\t\t\tdie(_(\"unable to parse commit %s\"),\n> +\t\t\t\toid_to_hex(old_commit_oid));\n\n\"unable to parse commit\" is a bit of a white lie.  In fact, there is\nnothing that makes oid_commit_oid the name of a commit object.\n\n\"unable to parse object '%s' as a tree\" would be more technically\ncorrect, but one random-looking hexadecimal string is almost\nindistinguishable from another, and neither would be a very useful\nmessage from the end user's point of view.  I am wondering if we can\nuse old_branch_info to formulate something easier to understand for\nthe user.  update_refs_for_switch() seems to compute old_desc as a\nhuman readable name by using old_branch_info->name if available\nbefore falling back to old_branch_info->commit object name, which\nmight be a source of inspiration.\n\n>  \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n>  \t\tparse_tree(new_tree);\n>  \t\ttree = new_tree;\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index a572cda503..0aea177660 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -700,6 +700,8 @@\n>  \tinit_checkout_metadata(&opts.meta, head, &oid, NULL);\n>  \n>  \ttree = parse_tree_indirect(&oid);\n> +\tif (!tree)\n> +\t\tdie(_(\"unable to parse commit %s\"), oid_to_hex(&oid));\n\nIf we follow the code path, we can positively tell that oid here has\nalways came from reading HEAD, so \"unable to parse HEAD as a tree\"\nwould be an easier version of the message, I think.\n\nThanks.\n"},{"id":"450159","messageId":"kl6lilswdu68.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57493","inReplyTo":"xmqq5yowolvs.fsf@gitster.g","subject":"Re: [PATCH] checkout, clone: die if tree cannot be parsed","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-03-02T19:35:27Z","receivedAt":"2022-03-02T19:35:34Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Glen Choo <chooglen@google.com> writes:\n>\n>> -\t\ttree = parse_tree_indirect(old_branch_info->commit ?\n>> -\t\t\t\t\t   &old_branch_info->commit->object.oid :\n>> -\t\t\t\t\t   the_hash_algo->empty_tree);\n>> +\n>> +\t\told_commit_oid = old_branch_info->commit ?\n>> +\t\t\t&old_branch_info->commit->object.oid :\n>> +\t\t\tthe_hash_algo->empty_tree;\n>\n> I guess this is done only so that you can use the object name in the\n> error message, which is fine.\n\nThat's correct.\n\n>> +\t\ttree = parse_tree_indirect(old_commit_oid);\n>> +\t\tif (!tree)\n>> +\t\t\tdie(_(\"unable to parse commit %s\"),\n>> +\t\t\t\toid_to_hex(old_commit_oid));\n>\n> \"unable to parse commit\" is a bit of a white lie.  In fact, there is\n> nothing that makes oid_commit_oid the name of a commit object.\n>\n> \"unable to parse object '%s' as a tree\" would be more technically\n> correct,\n\nHm, yes. With regards to parse_tree_indirect(), \"unable to parse object\n'%s' as a tree\" is a more accurate description of the failure. But since\nwe know that the oid is a commit in this context, I'm not sure if we\nneed to offload that much information to the user - if we failed to\nparse the given object id in the appropriate manner, the user would\nstill be directed to figure out what's wrong with the object.\n\n>          but one random-looking hexadecimal string is almost\n> indistinguishable from another, and neither would be a very useful\n> message from the end user's point of view.  I am wondering if we can\n> use old_branch_info to formulate something easier to understand for\n> the user.  update_refs_for_switch() seems to compute old_desc as a\n> human readable name by using old_branch_info->name if available\n> before falling back to old_branch_info->commit object name, which\n> might be a source of inspiration.\n\nI think it's actually more helpful to have the oid instead of a\nhuman-readable description like old_branch_info->name.\n\nFor an advanced user/repo admin, seeing the oid makes it very obvious\nthat there's a particular problem with the given object, and this would\ndirect them to hunt down the object locally (without partial clone) or\non the remote (with partial clone, as in the original motivation). From\nthere, it's easy to figure out which branch points to the offending\nobject. The branch name might be misleading - the user would presumably\nstart with hunting down the ref, then explore several possibilities\nbefore realizing the problem is actually with the _object_.\n\nFor a novice user, neither the branch name nor the object id is\nactionable because they probably wouldn't be able to fix the issue\nanyway. The advantage of the opaque hex string is that by being\nintimidating and unrecognizable, it indicates to the user that they\nshouldn't try to debug the issue and so they might give up sooner and\nask for help from someone who might be able to fix it.\n"},{"id":"450944","messageId":"nycvar.QRO.7.76.6.2203092319060.357@tvgsbejvaqbjf.bet","threadId":"57493","inReplyTo":"kl6lilswdu68.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] checkout, clone: die if tree cannot be parsed","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-09T22:20:30Z","receivedAt":"2022-03-09T22:20:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Glen,\n\nOn Wed, 2 Mar 2022, Glen Choo wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> >          but one random-looking hexadecimal string is almost\n> > indistinguishable from another, and neither would be a very useful\n> > message from the end user's point of view.  I am wondering if we can\n> > use old_branch_info to formulate something easier to understand for\n> > the user.  update_refs_for_switch() seems to compute old_desc as a\n> > human readable name by using old_branch_info->name if available\n> > before falling back to old_branch_info->commit object name, which\n> > might be a source of inspiration.\n>\n> I think it's actually more helpful to have the oid instead of a\n> human-readable description like old_branch_info->name.\n\nThe most helpful would be to have both. That way, it would at least be\npotentially possible to figure out from a ref how to fetch a non-corrupt\nversion from elsewhere.\n\nCiao,\nDscho\n"}]}