{"thread":{"id":"51808","subject":"[PATCH 0/3] commit-graph: harden against various corruptions","startedAt":"2019-09-05T22:04:55Z","lastAt":"2019-09-09T17:55:27Z","messageCount":15,"participants":["Taylor Blau","Jeff King","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"381920","messageId":"cover.1567720960.git.me@ttaylorr.com","threadId":"51808","inReplyTo":null,"subject":"[PATCH 0/3] commit-graph: harden against various corruptions","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-05T22:04:51Z","receivedAt":"2019-09-05T22:04:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi,\n\nThis patch series is a polished version of a series I posed in [1],\nbased on the subsequent discussion starting in [2]. It hardens the 'git\ncommit-graph write' code to handle two new types of object corruption\nthat previously resulted in a SIGSEGV, which are:\n\n  - a commit which lists a parent that does not exist, or is corrupt\n\n  - a commit which lists a corrupt tree, or omits the 'tree ' line\n    entirely.\n\nThe series goes as follows: first, we introduce failing tests that\ndemonstrate the above two cases, then subsequent commits to fix each of\nthem in turn.\n\nWhen reviewing this series, it is worth keeping in mind that this is\nreally only applying a band-aid to get around a quirk of the object\nparsing code, which is graciously described by Peff in more detail in\n[3].\n\nThanks in advance for your review. I'll deploy this in the meantime to\nGitHub's fork and report back on the success in the next \"What's\nCooking\".\n\nThanks,\nTaylor\n\n[1]: https://public-inbox.org/git/34e4ec793cb0d321d16b88777cd2db64ed7b772e.1567563244.git.me@ttaylorr.com/\n[2]: https://public-inbox.org/git/20190904030456.GA28836@sigill.intra.peff.net/\n[3]: https://public-inbox.org/git/20190905064723.GC21450@sigill.intra.peff.net/\n\nTaylor Blau (3):\n  t/t5318: introduce failing 'git commit-graph write' tests\n  commit-graph.c: handle commit parsing errors\n  commit-graph.c: handle corrupt/missing trees\n\n commit-graph.c          | 11 +++++++++--\n commit.c                |  3 ++-\n t/t5318-commit-graph.sh | 43 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 54 insertions(+), 3 deletions(-)\n\n--\n2.23.0\n"},{"id":"381921","messageId":"042a8ba8b2a98c269f9cd1a8e88488b80d686f0d.1567720960.git.me@ttaylorr.com","threadId":"51808","inReplyTo":"cover.1567720960.git.me@ttaylorr.com","subject":"[PATCH 1/3] t/t5318: introduce failing 'git commit-graph write' tests","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-05T22:04:53Z","receivedAt":"2019-09-05T22:04:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When invoking 'git commit-graph' in a corrupt repository, one can cause\na segfault when ancestral commits are corrupt in one way or another.\nThis is due to two function calls in the 'commit-graph.c' code that may\nreturn NULL, but are not checked for NULL-ness before dereferencing.\n\nBefore fixing the bug, introduce two failing tests that demonstrate the\nproblem. The first test corrupts an ancestral commit's parent to point\nto a non-existent object. The second test instead corrupts an ancestral\ntree by removing the 'tree' information entirely from the commit. Both\nof these cases cause segfaults, each at different lines.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5318-commit-graph.sh | 43 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 43 insertions(+)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex ab3eccf0fa..c855f81930 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -585,4 +585,47 @@ test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'corrupt commit-graph write (broken parent)' '\n+\trm -rf repo &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tempty=\"$(git mktree </dev/null)\" &&\n+\t\tcat >broken <<-EOF &&\n+\t\ttree $empty\n+\t\tparent 0000000000000000000000000000000000000000\n+\t\tauthor whatever <whatever@example.com> 1234 -0000\n+\t\tcommitter whatever <whatever@example.com> 1234 -0000\n+\n+\t\tbroken commit\n+\t\tEOF\n+\t\tbroken=\"$(git hash-object -w -t commit --literally broken)\" &&\n+\t\tgit commit-tree -p \"$broken\" -m \"good commit\" \"$empty\" >good &&\n+\t\ttest_must_fail git commit-graph write --stdin-commits \\\n+\t\t\t<good 2>test_err &&\n+\t\ttest_i18ngrep \"unable to parse commit\" test_err\n+\t)\n+'\n+\n+test_expect_failure 'corrupt commit-graph write (missing tree)' '\n+\trm -rf repo &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttree=\"$(git mktree </dev/null)\" &&\n+\t\tcat >broken <<-EOF &&\n+\t\tparent 0000000000000000000000000000000000000000\n+\t\tauthor whatever <whatever@example.com> 1234 -0000\n+\t\tcommitter whatever <whatever@example.com> 1234 -0000\n+\n+\t\tbroken commit\n+\t\tEOF\n+\t\tbroken=\"$(git hash-object -w -t commit --literally broken)\" &&\n+\t\tgit commit-tree -p \"$broken\" -m \"good\" \"$tree\" >good &&\n+\t\ttest_must_fail git commit-graph write --stdin-commits \\\n+\t\t\t<good 2>test_err &&\n+\t\ttest_i18ngrep \"unable to get tree for\" test_err\n+\t)\n+'\n+\n test_done\n-- \n2.23.0\n\n"},{"id":"381922","messageId":"7fb6853ab31f35c27e5c220f11ea2de6e229a806.1567720960.git.me@ttaylorr.com","threadId":"51808","inReplyTo":"cover.1567720960.git.me@ttaylorr.com","subject":"[PATCH 2/3] commit-graph.c: handle commit parsing errors","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-05T22:04:55Z","receivedAt":"2019-09-05T22:04:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"To write a commit graph chunk, 'write_graph_chunk_data()' takes a list\nof commits to write and parses each one before writing the necessary\ndata, and continuing on to the next commit in the list.\n\nSince the majority of these commits are not parsed ahead of time (an\nexception is made for the *last* commit in the list, which is parsed\nearly within 'copy_oids_to_commits'), it is possible that calling\n'parse_commit_no_graph()' on them may return an error. Failing to catch\nthese errors before de-referencing later calls can result in a undefined\nmemory access and a SIGSEGV.\n\nOne such example of this is 'get_commit_tree_oid()', which expects a\nparsed object as its input (in this case, the commit-graph code passes\n'*list'). If '*list' causes a parse error, the subsequent call will\nfail.\n\nPrevent such an issue by checking the return value of\n'parse_commit_no_graph()' to avoid passing an unparsed object to a\nfunction which expects a parsed object, thus preventing a segfault.\n\nIt is worth noting that this fix is really skirting around the issue in\nobject.c's 'parse_object()', which makes it difficult to tell how\ncorrupt an object is without digging into it. Presumably one could\nchange the meaning of 'parse_object' returns, but this would require\nadjusting each callsite accordingly. Instead of that, add an additional\ncheck to the object parsed.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c          | 4 +++-\n t/t5318-commit-graph.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex f2888c203b..6aa6998ecd 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -843,7 +843,9 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n \n-\t\tparse_commit_no_graph(*list);\n+\t\tif (parse_commit_no_graph(*list))\n+\t\t\tdie(_(\"unable to parse commit %s\"),\n+\t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n \n \t\tparent = (*list)->parents;\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex c855f81930..abde8d4e90 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -585,7 +585,7 @@ test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'corrupt commit-graph write (broken parent)' '\n+test_expect_success 'corrupt commit-graph write (broken parent)' '\n \trm -rf repo &&\n \tgit init repo &&\n \t(\n-- \n2.23.0\n\n"},{"id":"381923","messageId":"9fbd965e3892307bb5bb78952f017624fcc0b73a.1567720960.git.me@ttaylorr.com","threadId":"51808","inReplyTo":"cover.1567720960.git.me@ttaylorr.com","subject":"[PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-05T22:04:57Z","receivedAt":"2019-09-05T22:05:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the previous commit to handle an unchecked\ncall to 'get_commit_tree_oid()'. Previously, a NULL return value from\nthis function would be immediately dereferenced with '->hash', and then\ncause a segfault.\n\nBefore dereferencing to access the 'hash' member, check the return value\nof 'get_commit_tree_oid()' to make sure that it is not NULL.\n\nTo make this check correct, a related change is also needed in\n'commit.c', which is to check the return value of 'get_commit_tree'\nbefore taking its address. If 'get_commit_tree' returns NULL, we\nencounter an undefined behavior when taking the address of the return\nvalue of 'get_commit_tree' and then taking '->object.oid'. (On my system,\nthis is memory address 0x8, which is obviously wrong).\n\nFix this by making sure that 'get_commit_tree' returns something\nnon-NULL before digging through a structure that is not there, thus\npreventing a segfault down the line in the commit graph code.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n commit-graph.c          | 7 ++++++-\n commit.c                | 3 ++-\n t/t5318-commit-graph.sh | 2 +-\n 3 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6aa6998ecd..cea1b37493 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -839,6 +839,7 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \n \twhile (list < last) {\n \t\tstruct commit_list *parent;\n+\t\tstruct object_id *tree;\n \t\tint edge_value;\n \t\tuint32_t packedDate[2];\n \t\tdisplay_progress(ctx->progress, ++ctx->progress_cnt);\n@@ -846,7 +847,11 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\tif (parse_commit_no_graph(*list))\n \t\t\tdie(_(\"unable to parse commit %s\"),\n \t\t\t\toid_to_hex(&(*list)->object.oid));\n-\t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n+\t\ttree = get_commit_tree_oid(*list);\n+\t\tif (!tree)\n+\t\t\tdie(_(\"unable to get tree for %s\"),\n+\t\t\t\toid_to_hex(&(*list)->object.oid));\n+\t\thashwrite(f, tree->hash, hash_len);\n \n \t\tparent = (*list)->parents;\n \ndiff --git a/commit.c b/commit.c\nindex a98de16e3d..fab22cb740 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -358,7 +358,8 @@ struct tree *repo_get_commit_tree(struct repository *r,\n \n struct object_id *get_commit_tree_oid(const struct commit *commit)\n {\n-\treturn &get_commit_tree(commit)->object.oid;\n+\tstruct tree *tree = get_commit_tree(commit);\n+\treturn tree ? &tree->object.oid : NULL;\n }\n \n void release_commit_memory(struct parsed_object_pool *pool, struct commit *c)\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex abde8d4e90..5d2d88b100 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -607,7 +607,7 @@ test_expect_success 'corrupt commit-graph write (broken parent)' '\n \t)\n '\n \n-test_expect_failure 'corrupt commit-graph write (missing tree)' '\n+test_expect_success 'corrupt commit-graph write (missing tree)' '\n \trm -rf repo &&\n \tgit init repo &&\n \t(\n-- \n2.23.0\n"},{"id":"381945","messageId":"20190906061919.GA5122@sigill.intra.peff.net","threadId":"51808","inReplyTo":"9fbd965e3892307bb5bb78952f017624fcc0b73a.1567720960.git.me@ttaylorr.com","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T06:19:20Z","receivedAt":"2019-09-06T06:19:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2019 at 06:04:57PM -0400, Taylor Blau wrote:\n\n> @@ -846,7 +847,11 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n>  \t\tif (parse_commit_no_graph(*list))\n>  \t\t\tdie(_(\"unable to parse commit %s\"),\n>  \t\t\t\toid_to_hex(&(*list)->object.oid));\n> -\t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n> +\t\ttree = get_commit_tree_oid(*list);\n> +\t\tif (!tree)\n> +\t\t\tdie(_(\"unable to get tree for %s\"),\n> +\t\t\t\toid_to_hex(&(*list)->object.oid));\n> +\t\thashwrite(f, tree->hash, hash_len);\n\nYeah, I think this is a good stop-gap to protect ourselves, until a time\nwhen parse_commit() and friends consistently warn us about the breakage.\n\n> diff --git a/commit.c b/commit.c\n> index a98de16e3d..fab22cb740 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -358,7 +358,8 @@ struct tree *repo_get_commit_tree(struct repository *r,\n>  \n>  struct object_id *get_commit_tree_oid(const struct commit *commit)\n>  {\n> -\treturn &get_commit_tree(commit)->object.oid;\n> +\tstruct tree *tree = get_commit_tree(commit);\n> +\treturn tree ? &tree->object.oid : NULL;\n>  }\n\nThis one in theory benefits lots of other callsites, too, since it means\nwe'll actually return NULL instead of nonsense like \"8\". But grepping\naround for calls to this function, I found literally zero of them\nactually bother checking for a NULL result. So there are probably dozens\nof similar segfaults waiting to happen in other code paths.\nDiscouraging.\n\nThis is sort-of attributable to my 834876630b (get_commit_tree(): return\nNULL for broken tree, 2019-04-09). Before then it was a BUG(). However,\nthat state was relatively short-lived. Before 7b8a21dba1 (commit-graph:\nlazy-load trees for commits, 2018-04-06), we'd have similarly returned\nNULL (and anyway, BUG() is clearly wrong since it's a data error).\n\nNone of which argues against your patches, but it's kind of sad that the\nissue is present in so many code paths. I wonder if we could be handling\nthis in a more central way, but I don't see how short of dying.\n\n-Peff\n"},{"id":"381960","messageId":"20190906154214.GA3657@syl.local","threadId":"51808","inReplyTo":"20190906061919.GA5122@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2019-09-06T15:42:14Z","receivedAt":"2019-09-06T15:42:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Sep 06, 2019 at 02:19:20AM -0400, Jeff King wrote:\n> On Thu, Sep 05, 2019 at 06:04:57PM -0400, Taylor Blau wrote:\n>\n> > @@ -846,7 +847,11 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n> >  \t\tif (parse_commit_no_graph(*list))\n> >  \t\t\tdie(_(\"unable to parse commit %s\"),\n> >  \t\t\t\toid_to_hex(&(*list)->object.oid));\n> > -\t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n> > +\t\ttree = get_commit_tree_oid(*list);\n> > +\t\tif (!tree)\n> > +\t\t\tdie(_(\"unable to get tree for %s\"),\n> > +\t\t\t\toid_to_hex(&(*list)->object.oid));\n> > +\t\thashwrite(f, tree->hash, hash_len);\n>\n> Yeah, I think this is a good stop-gap to protect ourselves, until a time\n> when parse_commit() and friends consistently warn us about the breakage.\n>\n> > diff --git a/commit.c b/commit.c\n> > index a98de16e3d..fab22cb740 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -358,7 +358,8 @@ struct tree *repo_get_commit_tree(struct repository *r,\n> >\n> >  struct object_id *get_commit_tree_oid(const struct commit *commit)\n> >  {\n> > -\treturn &get_commit_tree(commit)->object.oid;\n> > +\tstruct tree *tree = get_commit_tree(commit);\n> > +\treturn tree ? &tree->object.oid : NULL;\n> >  }\n\nYou mentioned in the version of this series that is rebased on GitHub's\nfork that it may be worth putting this hunk in a separate commit\nentirely. I don't disagree, so if there are other comments that merit a\nreroll of this, I'm happy to pull this change out as 3/4.\n\n> This one in theory benefits lots of other callsites, too, since it means\n> we'll actually return NULL instead of nonsense like \"8\". But grepping\n> around for calls to this function, I found literally zero of them\n> actually bother checking for a NULL result. So there are probably dozens\n> of similar segfaults waiting to happen in other code paths.\n> Discouraging.\n\nDiscouraging indeed. I think that you suggest it below, but perhaps the\nright thing to do here is implement 'get_commit_tree_oid()' as follows:\n\n  struct object_id *get_commit_tree_oid(const struct commit *commit)\n  {\n    struct tree *tree = get_commit_tree(commit);\n    if (!tree)\n      die(_(\"unable to get tree from commit %s\"),\n          oid_to_hex(&commit->object.oid));\n    return &tree->object.oid;\n  }\n\nWhich then puts the onus on the *caller* to check their commit pointer\nto make sure that it has a legit tree in it, unless they're OK with\ndying.\n\nIn the commit-graph change that this whole thread is in response to,\nthat's exactly what we want: I don't want to have to check the return\nvalue of two function calls myself. I'm perfectly happy to die() in the\nmiddle of things if there is an object corruption, but the library code\nshould take care of that for me, and not allow for dozens of checks,\neach with their own unique 'die()'-ing message.\n\nAll of that said, I don't know if I think it's worth holding this series\nup on the above in the meantime. I do think that it (or something like\nit) is generally worth doing, but I'm not sure that now is the time to\ndo it.\n\n> This is sort-of attributable to my 834876630b (get_commit_tree(): return\n> NULL for broken tree, 2019-04-09). Before then it was a BUG(). However,\n> that state was relatively short-lived. Before 7b8a21dba1 (commit-graph:\n> lazy-load trees for commits, 2018-04-06), we'd have similarly returned\n> NULL (and anyway, BUG() is clearly wrong since it's a data error).\n\nHa, I was wondering why that commit message looked familiar... it turns\nout that I'm the culprit, too, via the 'Co-authored-by' trailer. Oops.\n\n> None of which argues against your patches, but it's kind of sad that the\n> issue is present in so many code paths. I wonder if we could be handling\n> this in a more central way, but I don't see how short of dying.\n>\n> -Peff\nThanks,\nTaylor\n"},{"id":"381962","messageId":"6035f5bf-3d09-f454-f9f6-c882bc0604c9@gmail.com","threadId":"51808","inReplyTo":"042a8ba8b2a98c269f9cd1a8e88488b80d686f0d.1567720960.git.me@ttaylorr.com","subject":"Re: [PATCH 1/3] t/t5318: introduce failing 'git commit-graph write' tests","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T16:48:11Z","receivedAt":"2019-09-06T16:48:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/5/2019 6:04 PM, Taylor Blau wrote:\n> When invoking 'git commit-graph' in a corrupt repository, one can cause\n> a segfault when ancestral commits are corrupt in one way or another.\n> This is due to two function calls in the 'commit-graph.c' code that may\n> return NULL, but are not checked for NULL-ness before dereferencing.\n> \n> Before fixing the bug, introduce two failing tests that demonstrate the\n> problem. The first test corrupts an ancestral commit's parent to point\n> to a non-existent object. The second test instead corrupts an ancestral\n> tree by removing the 'tree' information entirely from the commit. Both\n> of these cases cause segfaults, each at different lines.\n\nThanks for the tests! And marking them as \"test_expect_failure\" avoids\nissues with 'git bisect' in the future.\n\n-Stolee\t\n\n> \n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  t/t5318-commit-graph.sh | 43 +++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 43 insertions(+)\n> \n> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\n> index ab3eccf0fa..c855f81930 100755\n> --- a/t/t5318-commit-graph.sh\n> +++ b/t/t5318-commit-graph.sh\n> @@ -585,4 +585,47 @@ test_expect_success 'get_commit_tree_in_graph works for non-the_repository' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_failure 'corrupt commit-graph write (broken parent)' '\n> +\trm -rf repo &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tempty=\"$(git mktree </dev/null)\" &&\n> +\t\tcat >broken <<-EOF &&\n> +\t\ttree $empty\n> +\t\tparent 0000000000000000000000000000000000000000\n> +\t\tauthor whatever <whatever@example.com> 1234 -0000\n> +\t\tcommitter whatever <whatever@example.com> 1234 -0000\n> +\n> +\t\tbroken commit\n> +\t\tEOF\n> +\t\tbroken=\"$(git hash-object -w -t commit --literally broken)\" &&\n> +\t\tgit commit-tree -p \"$broken\" -m \"good commit\" \"$empty\" >good &&\n> +\t\ttest_must_fail git commit-graph write --stdin-commits \\\n> +\t\t\t<good 2>test_err &&\n> +\t\ttest_i18ngrep \"unable to parse commit\" test_err\n> +\t)\n> +'\n> +\n> +test_expect_failure 'corrupt commit-graph write (missing tree)' '\n> +\trm -rf repo &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttree=\"$(git mktree </dev/null)\" &&\n> +\t\tcat >broken <<-EOF &&\n> +\t\tparent 0000000000000000000000000000000000000000\n> +\t\tauthor whatever <whatever@example.com> 1234 -0000\n> +\t\tcommitter whatever <whatever@example.com> 1234 -0000\n> +\n> +\t\tbroken commit\n> +\t\tEOF\n> +\t\tbroken=\"$(git hash-object -w -t commit --literally broken)\" &&\n> +\t\tgit commit-tree -p \"$broken\" -m \"good\" \"$tree\" >good &&\n> +\t\ttest_must_fail git commit-graph write --stdin-commits \\\n> +\t\t\t<good 2>test_err &&\n> +\t\ttest_i18ngrep \"unable to get tree for\" test_err\n> +\t)\n> +'\n> +\n>  test_done\n> \n"},{"id":"381964","messageId":"ad34871d-bdc4-5b52-eff4-da03c6be1004@gmail.com","threadId":"51808","inReplyTo":"20190906061919.GA5122@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-06T16:51:56Z","receivedAt":"2019-09-06T16:52:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/6/2019 2:19 AM, Jeff King wrote:\n> On Thu, Sep 05, 2019 at 06:04:57PM -0400, Taylor Blau wrote:\n> \n>> @@ -846,7 +847,11 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n>>  \t\tif (parse_commit_no_graph(*list))\n>>  \t\t\tdie(_(\"unable to parse commit %s\"),\n>>  \t\t\t\toid_to_hex(&(*list)->object.oid));\n>> -\t\thashwrite(f, get_commit_tree_oid(*list)->hash, hash_len);\n>> +\t\ttree = get_commit_tree_oid(*list);\n>> +\t\tif (!tree)\n>> +\t\t\tdie(_(\"unable to get tree for %s\"),\n>> +\t\t\t\toid_to_hex(&(*list)->object.oid));\n>> +\t\thashwrite(f, tree->hash, hash_len);\n> \n> Yeah, I think this is a good stop-gap to protect ourselves, until a time\n> when parse_commit() and friends consistently warn us about the breakage.\n> \n>> diff --git a/commit.c b/commit.c\n>> index a98de16e3d..fab22cb740 100644\n>> --- a/commit.c\n>> +++ b/commit.c\n>> @@ -358,7 +358,8 @@ struct tree *repo_get_commit_tree(struct repository *r,\n>>  \n>>  struct object_id *get_commit_tree_oid(const struct commit *commit)\n>>  {\n>> -\treturn &get_commit_tree(commit)->object.oid;\n>> +\tstruct tree *tree = get_commit_tree(commit);\n>> +\treturn tree ? &tree->object.oid : NULL;\n>>  }\n> \n> This one in theory benefits lots of other callsites, too, since it means\n> we'll actually return NULL instead of nonsense like \"8\". But grepping\n> around for calls to this function, I found literally zero of them\n> actually bother checking for a NULL result. So there are probably dozens\n> of similar segfaults waiting to happen in other code paths.\n> Discouraging.\n>\n> This is sort-of attributable to my 834876630b (get_commit_tree(): return\n> NULL for broken tree, 2019-04-09). Before then it was a BUG(). However,\n> that state was relatively short-lived. Before 7b8a21dba1 (commit-graph:\n> lazy-load trees for commits, 2018-04-06), we'd have similarly returned\n> NULL (and anyway, BUG() is clearly wrong since it's a data error).\n> \n> None of which argues against your patches, but it's kind of sad that the\n> issue is present in so many code paths. I wonder if we could be handling\n> this in a more central way, but I don't see how short of dying.\n\nThis is due to the mechanical conversion from using commit->tree->oid to\nget_commit_tree_oid(commit). Those consumers were not checking if the\ntree pointer was NULL, either, but they probably assumed that the\nparse_commit() call would have failed earlier. Now that we are using this\nmethod (for performance reasons to avoid creating too many 'struct tree's)\nit makes sense to convert some of them to checking the return value more\ncarefully.\n\nIf we wanted the more invasive change of modifying every caller to check\n NULL and respond appropriately, that would be _best_, but is probably\nnot worth the effort.\n\nThanks,\n-Stolee\n"},{"id":"381965","messageId":"xmqqo8zxnz0m.fsf@gitster-ct.c.googlers.com","threadId":"51808","inReplyTo":"20190906061919.GA5122@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-06T16:57:29Z","receivedAt":"2019-09-06T16:57:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This is sort-of attributable to my 834876630b (get_commit_tree(): return\n> NULL for broken tree, 2019-04-09). Before then it was a BUG(). However,\n> that state was relatively short-lived. Before 7b8a21dba1 (commit-graph:\n> lazy-load trees for commits, 2018-04-06), we'd have similarly returned\n> NULL (and anyway, BUG() is clearly wrong since it's a data error).\n>\n> None of which argues against your patches, but it's kind of sad that the\n> issue is present in so many code paths. I wonder if we could be handling\n> this in a more central way, but I don't see how short of dying.\n\nWell, either we explicitly die in here, or let the caller segfault.\nIs there even a single caller that is prepared to react to NULL?\n\n    Answer. There is a single hit inside fsck.c that wants to report\n    an error without killing ourselves in fsck_commit_buffer().  I\n    however doubt its use of get_commit_tree() is correct in the\n    first place.  The function is about validating the commit object\n    payload manually, without trusting the result of parse_commit(),\n    and it does read the object name of the tree object; the call to\n    get_commit_tree() used for reporting the error there should\n    probably become has_object() on the tree_oid.\n\nSo, after fixing the above, we may safely be able to die inside\nget_commit_tree() instead of returning NULL.\n\nBy the way, I think get_commit_tree() and parse_commit() in fsck\nshould always use the value obtained from the underlying object and\nbypass any caches like commit graph---if they pay attention to the\ncaches, they should be fixed.  Secondary caches like commit graph\nshould of course be validated against what are recorded in the\nunderlying object, but that should be done separately.\n"},{"id":"381971","messageId":"xmqq5zm5nydu.fsf@gitster-ct.c.googlers.com","threadId":"51808","inReplyTo":"xmqqo8zxnz0m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-06T17:11:09Z","receivedAt":"2019-09-06T17:11:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...\n> Is there even a single caller that is prepared to react to NULL?\n>\n>     Answer. There is a single hit inside fsck.c that wants to report\n>     an error without killing ourselves in fsck_commit_buffer().  I\n>     however doubt its use of get_commit_tree() is correct in the\n>     first place.  The function is about validating the commit object\n>     payload manually, without trusting the result of parse_commit(),\n>     and it does read the object name of the tree object; the call to\n>     get_commit_tree() used for reporting the error there should\n>     probably become has_object() on the tree_oid.\n\nAt least we need to ensure, not just has_object(), but the object\nindeed claims to be a tree object.  It is OK if it is a corrupt\ntree object---we'll catch its brokenness separately anyway.\n\n    Hmm, the should we also tolerate the pointed object to be broken\n    in a way that it is not even able to claim to be a tree object?\n    That would mean that has_object() is sufficient to check here.\n\nOK, so... \n\n> So, after fixing the above, we may safely be able to die inside\n> get_commit_tree() instead of returning NULL.\n>\n> By the way, I think get_commit_tree() and parse_commit() in fsck\n> should always use the value obtained from the underlying object and\n> bypass any caches like commit graph---if they pay attention to the\n> caches, they should be fixed.  Secondary caches like commit graph\n> should of course be validated against what are recorded in the\n> underlying object, but that should be done separately.\n"},{"id":"381974","messageId":"20190906172851.GC23181@sigill.intra.peff.net","threadId":"51808","inReplyTo":"xmqqo8zxnz0m.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:28:52Z","receivedAt":"2019-09-06T17:28:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 09:57:29AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This is sort-of attributable to my 834876630b (get_commit_tree(): return\n> > NULL for broken tree, 2019-04-09). Before then it was a BUG(). However,\n> > that state was relatively short-lived. Before 7b8a21dba1 (commit-graph:\n> > lazy-load trees for commits, 2018-04-06), we'd have similarly returned\n> > NULL (and anyway, BUG() is clearly wrong since it's a data error).\n> >\n> > None of which argues against your patches, but it's kind of sad that the\n> > issue is present in so many code paths. I wonder if we could be handling\n> > this in a more central way, but I don't see how short of dying.\n> \n> Well, either we explicitly die in here, or let the caller segfault.\n> Is there even a single caller that is prepared to react to NULL?\n> [...]\n> So, after fixing the above, we may safely be able to die inside\n> get_commit_tree() instead of returning NULL.\n\nI think the one alternative is catching this more reliably during the\nparse phase. And then callers have the option of handling the error\n_then_, without forcing every downstream user of the struct to\nre-validate it.\n\nWe _could_ add the die() there to catch any stragglers. But that does\nmake it harder for somebody to try to examine the error situation, or\ngracefully return an error up the stack. Maybe that use case really\ndoesn't have any value. I dunno. This case did BUG() until recently, and\nwe did run into it in the real world. But the problem wasn't that the\noperation didn't succeed, but rather the BUG(). I don't know of any code\npath where the caller doesn't simply die().\n\n>     Answer. There is a single hit inside fsck.c that wants to report\n>     an error without killing ourselves in fsck_commit_buffer().  I\n>     however doubt its use of get_commit_tree() is correct in the\n>     first place.  The function is about validating the commit object\n>     payload manually, without trusting the result of parse_commit(),\n>     and it does read the object name of the tree object; the call to\n>     get_commit_tree() used for reporting the error there should\n>     probably become has_object() on the tree_oid.\n\nI actually think that check should be removed entirely. That function is\nabout checking the syntactic validity of the object itself, not about\nconnectivity (which is handled separately). We already check that we\nhave a valid \"tree\" pointer earlier in the function.\n\nThe current get_commit_tree() check is doing essentially nothing.\nparse_commit() would have parsed the same thing we already checked, and\nthe lookup_tree() call it uses to fill in the pointer is not reliable\n(it would only fail if we happened to have seen the same oid already as\na non-tree in the same process).\n\nThe history is interesting here. In the early days fsck-cache actually\ndid parse the commit object itself. Then ff5ebe39b0 ([PATCH] Port\nfsck-cache to use parsing functions, 2005-04-18) converted it to use\nparse_commit(). Then de2eb7f694 (git-fsck-cache.c: check commit objects\nmore carefully, 2005-07-27) went back to parsing it ourselves, but left\nthe struct checks in place.\n\nWe also look at commit->parents, but seemingly only to compare them to\ngrafts (and that's weird itself, because grafts aren't a property of the\nobject at all, and it seems like at best this is just verifying that we\ncorrectly loaded the grafts).\n\n> By the way, I think get_commit_tree() and parse_commit() in fsck\n> should always use the value obtained from the underlying object and\n> bypass any caches like commit graph---if they pay attention to the\n> caches, they should be fixed.  Secondary caches like commit graph\n> should of course be validated against what are recorded in the\n> underlying object, but that should be done separately.\n\nAgreed. Probably fsck should just be disabling the commit graph for the\nwhole process (it looks like there's an env variable for this, but no\ninternal global, which is what fsck would want).\n\n-Peff\n"},{"id":"381975","messageId":"20190906173043.GD23181@sigill.intra.peff.net","threadId":"51808","inReplyTo":"xmqq5zm5nydu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:30:43Z","receivedAt":"2019-09-06T17:30:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 10:11:09AM -0700, Junio C Hamano wrote:\n\n> > Is there even a single caller that is prepared to react to NULL?\n> >\n> >     Answer. There is a single hit inside fsck.c that wants to report\n> >     an error without killing ourselves in fsck_commit_buffer().  I\n> >     however doubt its use of get_commit_tree() is correct in the\n> >     first place.  The function is about validating the commit object\n> >     payload manually, without trusting the result of parse_commit(),\n> >     and it does read the object name of the tree object; the call to\n> >     get_commit_tree() used for reporting the error there should\n> >     probably become has_object() on the tree_oid.\n> \n> At least we need to ensure, not just has_object(), but the object\n> indeed claims to be a tree object.  It is OK if it is a corrupt\n> tree object---we'll catch its brokenness separately anyway.\n> \n>     Hmm, the should we also tolerate the pointed object to be broken\n>     in a way that it is not even able to claim to be a tree object?\n>     That would mean that has_object() is sufficient to check here.\n> \n> OK, so... \n\nWe'd do that check later, during fsck_walk_commit(). Which ironically\ncalls get_commit_tree() without checking for NULL, and would likely\nsegfault. ;)\n\n-Peff\n"},{"id":"381976","messageId":"20190906173436.GE23181@sigill.intra.peff.net","threadId":"51808","inReplyTo":"20190906154214.GA3657@syl.local","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:34:37Z","receivedAt":"2019-09-06T17:34:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 11:42:14AM -0400, Taylor Blau wrote:\n\n> > >  struct object_id *get_commit_tree_oid(const struct commit *commit)\n> > >  {\n> > > -\treturn &get_commit_tree(commit)->object.oid;\n> > > +\tstruct tree *tree = get_commit_tree(commit);\n> > > +\treturn tree ? &tree->object.oid : NULL;\n> > >  }\n> \n> You mentioned in the version of this series that is rebased on GitHub's\n> fork that it may be worth putting this hunk in a separate commit\n> entirely. I don't disagree, so if there are other comments that merit a\n> reroll of this, I'm happy to pull this change out as 3/4.\n\nYeah, I could go either way on that, I think. I was thinking it might be\nfixing other callsites, but it seems that nobody else bothers to check\nfor NULL anyway. But being in its own commit, we could explain that.\n\n> > This one in theory benefits lots of other callsites, too, since it means\n> > we'll actually return NULL instead of nonsense like \"8\". But grepping\n> > around for calls to this function, I found literally zero of them\n> > actually bother checking for a NULL result. So there are probably dozens\n> > of similar segfaults waiting to happen in other code paths.\n> > Discouraging.\n> \n> Discouraging indeed. I think that you suggest it below, but perhaps the\n> right thing to do here is implement 'get_commit_tree_oid()' as follows:\n> \n>   struct object_id *get_commit_tree_oid(const struct commit *commit)\n>   {\n>     struct tree *tree = get_commit_tree(commit);\n>     if (!tree)\n>       die(_(\"unable to get tree from commit %s\"),\n>           oid_to_hex(&commit->object.oid));\n>     return &tree->object.oid;\n>   }\n> \n> Which then puts the onus on the *caller* to check their commit pointer\n> to make sure that it has a legit tree in it, unless they're OK with\n> dying.\n\nYeah, I agree that would prevent segfaults (and is similar to what René\nproposed for tags with a similar situation). It does feel like a step\nbackwards in terms of lib-ification. But maybe it's a\nbelt-and-suspenders on top of trying to catch this case at the parsing\nstage, too.\n\n> All of that said, I don't know if I think it's worth holding this series\n> up on the above in the meantime. I do think that it (or something like\n> it) is generally worth doing, but I'm not sure that now is the time to\n> do it.\n\nI'd agree with that, and I think it's sensible to take your patches with\nthe extra tree check. We can rip it out later if it becomes redundant.\n\n-Peff\n"},{"id":"381977","messageId":"20190906173707.GF23181@sigill.intra.peff.net","threadId":"51808","inReplyTo":"ad34871d-bdc4-5b52-eff4-da03c6be1004@gmail.com","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:37:07Z","receivedAt":"2019-09-06T17:37:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 12:51:56PM -0400, Derrick Stolee wrote:\n\n> > This one in theory benefits lots of other callsites, too, since it means\n> > we'll actually return NULL instead of nonsense like \"8\". But grepping\n> > around for calls to this function, I found literally zero of them\n> > actually bother checking for a NULL result. So there are probably dozens\n> > of similar segfaults waiting to happen in other code paths.\n> > Discouraging.\n> >\n> > This is sort-of attributable to my 834876630b (get_commit_tree(): return\n> > NULL for broken tree, 2019-04-09). Before then it was a BUG(). However,\n> > that state was relatively short-lived. Before 7b8a21dba1 (commit-graph:\n> > lazy-load trees for commits, 2018-04-06), we'd have similarly returned\n> > NULL (and anyway, BUG() is clearly wrong since it's a data error).\n> > \n> > None of which argues against your patches, but it's kind of sad that the\n> > issue is present in so many code paths. I wonder if we could be handling\n> > this in a more central way, but I don't see how short of dying.\n> \n> This is due to the mechanical conversion from using commit->tree->oid to\n> get_commit_tree_oid(commit). Those consumers were not checking if the\n> tree pointer was NULL, either, but they probably assumed that the\n> parse_commit() call would have failed earlier. Now that we are using this\n> method (for performance reasons to avoid creating too many 'struct tree's)\n> it makes sense to convert some of them to checking the return value more\n> carefully.\n\nRight, none of this is new at all. We have historically been very loose\nabout assuming that things like commit->tree were valid. And they\n_usually_ are. Even if we're missing the object on disk, lookup_tree()\nis happy to assign it a struct (unless the object was already seen as\nanother type!).  I think turning that case into an error from\nparse_commit() would cover a lot of cases easily, without forcing each\ncaller to check for NULL.\n\n-Peff\n"},{"id":"382079","messageId":"xmqqftl5l5h5.fsf@gitster-ct.c.googlers.com","threadId":"51808","inReplyTo":"20190906172851.GC23181@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] commit-graph.c: handle corrupt/missing trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T17:55:18Z","receivedAt":"2019-09-09T17:55:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>>     Answer. There is a single hit inside fsck.c that wants to report\n>>     an error without killing ourselves in fsck_commit_buffer().  I\n>>     however doubt its use of get_commit_tree() is correct in the\n>>     first place.  The function is about validating the commit object\n>>     payload manually, without trusting the result of parse_commit(),\n>>     and it does read the object name of the tree object; the call to\n>>     get_commit_tree() used for reporting the error there should\n>>     probably become has_object() on the tree_oid.\n>\n> I actually think that check should be removed entirely. That function is\n> about checking the syntactic validity of the object itself, not about\n> connectivity (which is handled separately). We already check that we\n> have a valid \"tree\" pointer earlier in the function.\n\nOf course, you're right.\n"}]}