{"thread":{"id":"48861","subject":"BUG: Segfault on \"git pull\" on \"bad object HEAD\"","startedAt":"2018-07-11T11:01:02Z","lastAt":"2018-07-11T17:09:08Z","messageCount":7,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"352229","messageId":"87k1q2c9zq.fsf@evledraar.gmail.com","threadId":"48861","inReplyTo":null,"subject":"BUG: Segfault on \"git pull\" on \"bad object HEAD\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-11T11:00:57Z","receivedAt":"2018-07-11T11:01:02Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This segfaults, but should print an error instead, have a repo with a\ncorrupt HEAD:\n\n    (\n        rm -rf /tmp/git &&\n        git clone --single-branch --branch todo git@github.com:git/git.git /tmp/git &&\n        echo 1111111111111111111111111111111111111111 >/tmp/git/.git/refs/heads/todo &&\n        git -C /tmp/git pull\n    )\n\nOn this repository e.g. \"git log\" will print \"fatal: bad object HEAD\",\nbut for some reason \"git pull\" makes it this far:\n\n    $ git pull\n    Segmentation fault\n\nThe immediate reason is that in run_diff_index() we have this:\n\n\tent = revs->pending.objects;\n\nAnd that in this case that's NULL:\n\n    (gdb) bt\n    #0  0x000055555565993f in run_diff_index (revs=0x7fffffffcb90, cached=1) at diff-lib.c:524\n    #1  0x00005555557633da in has_uncommitted_changes (ignore_submodules=1) at wt-status.c:2345\n    #2  0x00005555557634c9 in require_clean_work_tree (action=0x555555798f18 \"pull with rebase\", hint=0x555555798efb \"please commit or stash them.\", ignore_submodules=1, gently=0) at wt-status.c:2370\n    #3  0x00005555555dbdee in cmd_pull (argc=0, argv=0x7fffffffd868, prefix=0x0) at builtin/pull.c:885\n    #4  0x000055555556c9da in run_builtin (p=0x555555a2de50 <commands+1872>, argc=1, argv=0x7fffffffd868) at git.c:417\n    #5  0x000055555556cce2 in handle_builtin (argc=1, argv=0x7fffffffd868) at git.c:633\n    #6  0x000055555556ce8a in run_argv (argcp=0x7fffffffd71c, argv=0x7fffffffd710) at git.c:685\n    #7  0x000055555556d03f in cmd_main (argc=1, argv=0x7fffffffd868) at git.c:762\n    #8  0x0000555555611786 in main (argc=3, argv=0x7fffffffd858) at common-main.c:45\n    (gdb) p revs\n    $4 = (struct rev_info *) 0x7fffffffcb90\n    (gdb) p revs->pending\n    $5 = {nr = 0, alloc = 0, objects = 0x0}\n    (gdb)\n\nThis has been an issue since at least v2.8.0 (didn't test back\nfurther). I'm not familiar with the status / diff code, so I'm not sure\nwhere the assertion should be added.\n\nThis came up in the wild due to a user with a corrupt repo (don't know\nhow it got corrupt) trying \"git pull\" and seeing git segfault.\n"},{"id":"352242","messageId":"20180711133402.GD23835@sigill.intra.peff.net","threadId":"48861","inReplyTo":"87k1q2c9zq.fsf@evledraar.gmail.com","subject":"Re: BUG: Segfault on \"git pull\" on \"bad object HEAD\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T13:34:02Z","receivedAt":"2018-07-11T13:34:06Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 01:00:57PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This segfaults, but should print an error instead, have a repo with a\n> corrupt HEAD:\n> \n>     (\n>         rm -rf /tmp/git &&\n>         git clone --single-branch --branch todo git@github.com:git/git.git /tmp/git &&\n>         echo 1111111111111111111111111111111111111111 >/tmp/git/.git/refs/heads/todo &&\n>         git -C /tmp/git pull\n>     )\n\nIt took me a minute to reproduce this. It needs \"pull --rebase\" if you\ndon't have that setup in your config.\n\n> The immediate reason is that in run_diff_index() we have this:\n> \n> \tent = revs->pending.objects;\n> \n> And that in this case that's NULL:\n> \n>     (gdb) bt\n>     #0  0x000055555565993f in run_diff_index (revs=0x7fffffffcb90, cached=1) at diff-lib.c:524\n>     #1  0x00005555557633da in has_uncommitted_changes (ignore_submodules=1) at wt-status.c:2345\n\nThese two are the interesting functions. has_uncommitted_changes() calls\nadd_head_to_pending(). So it could realize then that there is no valid\nHEAD to compare against.\n\nBut as you note, it's run_diff_index() that blindly dereferences\nrevs->pending.objects without seeing if it's non-empty. Normally\nsetup_revisions() would barf on a bad object, but the manual\nadd_head_to_pending() quietly returns (as it must for some cases, like\nunborn branches).\n\nSo I feel like the right answer here is probably this:\n\ndiff --git a/wt-status.c b/wt-status.c\nindex d1c05145a4..5fcaa3d0f8 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2340,7 +2340,16 @@ int has_uncommitted_changes(int ignore_submodules)\n \tif (ignore_submodules)\n \t\trev_info.diffopt.flags.ignore_submodules = 1;\n \trev_info.diffopt.flags.quick = 1;\n+\n \tadd_head_to_pending(&rev_info);\n+\tif (!rev_info.pending.nr) {\n+\t\t/*\n+\t\t * We have no head (or it's corrupt), but the index is not\n+\t\t * unborn; declare it as uncommitted changes.\n+\t\t */\n+\t\treturn 1;\n+\t}\n+\n \tdiff_setup_done(&rev_info.diffopt);\n \tresult = run_diff_index(&rev_info, 1);\n \treturn diff_result_code(&rev_info.diffopt, result);\n\nThat does quietly paper over the corruption, but it does the\nconservative thing, and a follow-up \"git status\" would yield \"bad\nobject: HEAD\".\n\nI do worry that other callers of run_diff_index() might have similar\nproblems, though. Grepping around, the other callers seem to fall into\none of three categories:\n\n - they resolve the object themselves and put it in the pending list\n   (and often fallback to the empty tree, which is more or less what the\n   patch above is doing)\n\n - they resolve the object themselves and avoid calling run_diff_index()\n   if it's not valid\n\n - they use setup_revisions(), which will barf on the broken object\n\nSo I think this may be sufficient. We probably should also add an\nassertion to run_diff_index(), since that's better than segfaulting.\n\n-Peff\n"},{"id":"352245","messageId":"20180711141406.GE23835@sigill.intra.peff.net","threadId":"48861","inReplyTo":"20180711133402.GD23835@sigill.intra.peff.net","subject":"[PATCH] has_uncommitted_changes(): fall back to empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T14:14:06Z","receivedAt":"2018-07-11T14:14:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 09:34:02AM -0400, Jeff King wrote:\n\n> I do worry that other callers of run_diff_index() might have similar\n> problems, though. Grepping around, the other callers seem to fall into\n> one of three categories:\n> \n>  - they resolve the object themselves and put it in the pending list\n>    (and often fallback to the empty tree, which is more or less what the\n>    patch above is doing)\n> \n>  - they resolve the object themselves and avoid calling run_diff_index()\n>    if it's not valid\n> \n>  - they use setup_revisions(), which will barf on the broken object\n> \n> So I think this may be sufficient. We probably should also add an\n> assertion to run_diff_index(), since that's better than segfaulting.\n\nHere's a patch to do that. I tweaked it slightly from what I showed\nearlier to use the empty tree, which matches what other code (e.g.,\ngit-diff) would do.\n\n-- >8 --\nSubject: has_uncommitted_changes(): fall back to empty tree\n\nIf has_uncommitted_changes() can't resolve HEAD (e.g.,\nbecause it's unborn or corrupt), then we end up calling\nrun_diff_index() with an empty revs.pending array. This\ncauses a segfault, as run_diff_index() blindly looks at the\nfirst pending item.\n\nFixing this raises a question of fault: should\nrun_diff_index() handle this case, or is the caller wrong to\npass an empty pending list?\n\nLooking at the other callers of run_diff_index(), they\nhandle this in one of three ways:\n\n - they resolve the object themselves, and avoid doing the\n   diff if it's not valid\n\n - they resolve the object themselves, and fall back to the\n   empty tree\n\n - they use setup_revisions(), which will die() if the\n   object isn't valid\n\nSince this is the only broken caller, that argues that the\nfix should go there. Falling back to the empty tree makes\nsense here, as we'd claim uncommitted changes if and only if\nthe index is non-empty. This may be a little funny in the\ncase of corruption (the corrupt HEAD probably _isn't_\nempty), but:\n\n  - we don't actually know the reason here that HEAD didn't\n    resolve (the much more likely case is that we have an\n    unborn HEAD, in which case the empty tree comparison is\n    the right thing)\n\n  - this matches how other code, like \"git diff\", behaves\n\nWhile we're thinking about it, let's add an assertion to\nrun_diff_index(). It should always be passed a single\nobject, and as this bug shows, it's easy to get it wrong\n(and an assertion is easier to hunt down than a segfault, or\na quietly ignored extra tree).\n\nReported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff-lib.c      |  3 +++\n t/t5520-pull.sh | 12 ++++++++++++\n wt-status.c     | 10 ++++++++++\n 3 files changed, 25 insertions(+)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a9f38eb5a3..732f684a49 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -520,6 +520,9 @@ int run_diff_index(struct rev_info *revs, int cached)\n \tstruct object_array_entry *ent;\n \tuint64_t start = getnanotime();\n \n+\tif (revs->pending.nr != 1)\n+\t\tBUG(\"run_diff_index must be passed exactly one tree\");\n+\n \tent = revs->pending.objects;\n \tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n \t\texit(128);\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 59c4b778d3..68aa5f0340 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -618,6 +618,18 @@ test_expect_success 'pull --rebase fails on unborn branch with staged changes' '\n \t)\n '\n \n+test_expect_success 'pull --rebase fails on corrupt HEAD' '\n+\ttest_when_finished \"rm -rf corrupt\" &&\n+\tgit init corrupt &&\n+\t(\n+\t\tcd corrupt &&\n+\t\ttest_commit one &&\n+\t\tobj=$(git rev-parse --verify HEAD | sed \"s#^..#&/#\") &&\n+\t\trm -f .git/objects/$obj &&\n+\t\ttest_must_fail git pull --rebase\n+\t)\n+'\n+\n test_expect_success 'setup for detecting upstreamed changes' '\n \tmkdir src &&\n \t(cd src &&\ndiff --git a/wt-status.c b/wt-status.c\nindex d1c05145a4..d89c41ba10 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2340,7 +2340,17 @@ int has_uncommitted_changes(int ignore_submodules)\n \tif (ignore_submodules)\n \t\trev_info.diffopt.flags.ignore_submodules = 1;\n \trev_info.diffopt.flags.quick = 1;\n+\n \tadd_head_to_pending(&rev_info);\n+\tif (!rev_info.pending.nr) {\n+\t\t/*\n+\t\t * We have no head (or it's corrupt); use the empty tree,\n+\t\t * which will complain if the index is non-empty.\n+\t\t */\n+\t\tstruct tree *tree = lookup_tree(the_hash_algo->empty_tree);\n+\t\tadd_pending_object(&rev_info, &tree->object, \"\");\n+\t}\n+\n \tdiff_setup_done(&rev_info.diffopt);\n \tresult = run_diff_index(&rev_info, 1);\n \treturn diff_result_code(&rev_info.diffopt, result);\n-- \n2.18.0.400.g702e398724\n\n"},{"id":"352250","messageId":"87in5ldedd.fsf@evledraar.gmail.com","threadId":"48861","inReplyTo":"20180711141406.GE23835@sigill.intra.peff.net","subject":"Re: [PATCH] has_uncommitted_changes(): fall back to empty tree","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-11T14:41:02Z","receivedAt":"2018-07-11T14:41:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 11 2018, Jeff King wrote:\n\n> On Wed, Jul 11, 2018 at 09:34:02AM -0400, Jeff King wrote:\n>\n>> I do worry that other callers of run_diff_index() might have similar\n>> problems, though. Grepping around, the other callers seem to fall into\n>> one of three categories:\n>>\n>>  - they resolve the object themselves and put it in the pending list\n>>    (and often fallback to the empty tree, which is more or less what the\n>>    patch above is doing)\n>>\n>>  - they resolve the object themselves and avoid calling run_diff_index()\n>>    if it's not valid\n>>\n>>  - they use setup_revisions(), which will barf on the broken object\n>>\n>> So I think this may be sufficient. We probably should also add an\n>> assertion to run_diff_index(), since that's better than segfaulting.\n>\n> Here's a patch to do that. I tweaked it slightly from what I showed\n> earlier to use the empty tree, which matches what other code (e.g.,\n> git-diff) would do.\n>\n> -- >8 --\n> Subject: has_uncommitted_changes(): fall back to empty tree\n>\n> If has_uncommitted_changes() can't resolve HEAD (e.g.,\n> because it's unborn or corrupt), then we end up calling\n> run_diff_index() with an empty revs.pending array. This\n> causes a segfault, as run_diff_index() blindly looks at the\n> first pending item.\n>\n> Fixing this raises a question of fault: should\n> run_diff_index() handle this case, or is the caller wrong to\n> pass an empty pending list?\n>\n> Looking at the other callers of run_diff_index(), they\n> handle this in one of three ways:\n>\n>  - they resolve the object themselves, and avoid doing the\n>    diff if it's not valid\n>\n>  - they resolve the object themselves, and fall back to the\n>    empty tree\n>\n>  - they use setup_revisions(), which will die() if the\n>    object isn't valid\n>\n> Since this is the only broken caller, that argues that the\n> fix should go there. Falling back to the empty tree makes\n> sense here, as we'd claim uncommitted changes if and only if\n> the index is non-empty. This may be a little funny in the\n> case of corruption (the corrupt HEAD probably _isn't_\n> empty), but:\n>\n>   - we don't actually know the reason here that HEAD didn't\n>     resolve (the much more likely case is that we have an\n>     unborn HEAD, in which case the empty tree comparison is\n>     the right thing)\n>\n>   - this matches how other code, like \"git diff\", behaves\n>\n> While we're thinking about it, let's add an assertion to\n> run_diff_index(). It should always be passed a single\n> object, and as this bug shows, it's easy to get it wrong\n> (and an assertion is easier to hunt down than a segfault, or\n> a quietly ignored extra tree).\n>\n> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  diff-lib.c      |  3 +++\n>  t/t5520-pull.sh | 12 ++++++++++++\n>  wt-status.c     | 10 ++++++++++\n>  3 files changed, 25 insertions(+)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index a9f38eb5a3..732f684a49 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -520,6 +520,9 @@ int run_diff_index(struct rev_info *revs, int cached)\n>  \tstruct object_array_entry *ent;\n>  \tuint64_t start = getnanotime();\n>\n> +\tif (revs->pending.nr != 1)\n> +\t\tBUG(\"run_diff_index must be passed exactly one tree\");\n> +\n>  \tent = revs->pending.objects;\n>  \tif (diff_cache(revs, &ent->item->oid, ent->name, cached))\n>  \t\texit(128);\n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 59c4b778d3..68aa5f0340 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -618,6 +618,18 @@ test_expect_success 'pull --rebase fails on unborn branch with staged changes' '\n>  \t)\n>  '\n>\n> +test_expect_success 'pull --rebase fails on corrupt HEAD' '\n> +\ttest_when_finished \"rm -rf corrupt\" &&\n> +\tgit init corrupt &&\n> +\t(\n> +\t\tcd corrupt &&\n> +\t\ttest_commit one &&\n> +\t\tobj=$(git rev-parse --verify HEAD | sed \"s#^..#&/#\") &&\n> +\t\trm -f .git/objects/$obj &&\n> +\t\ttest_must_fail git pull --rebase\n> +\t)\n> +'\n> +\n>  test_expect_success 'setup for detecting upstreamed changes' '\n>  \tmkdir src &&\n>  \t(cd src &&\n> diff --git a/wt-status.c b/wt-status.c\n> index d1c05145a4..d89c41ba10 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -2340,7 +2340,17 @@ int has_uncommitted_changes(int ignore_submodules)\n>  \tif (ignore_submodules)\n>  \t\trev_info.diffopt.flags.ignore_submodules = 1;\n>  \trev_info.diffopt.flags.quick = 1;\n> +\n>  \tadd_head_to_pending(&rev_info);\n> +\tif (!rev_info.pending.nr) {\n> +\t\t/*\n> +\t\t * We have no head (or it's corrupt); use the empty tree,\n> +\t\t * which will complain if the index is non-empty.\n> +\t\t */\n> +\t\tstruct tree *tree = lookup_tree(the_hash_algo->empty_tree);\n> +\t\tadd_pending_object(&rev_info, &tree->object, \"\");\n> +\t}\n> +\n>  \tdiff_setup_done(&rev_info.diffopt);\n>  \tresult = run_diff_index(&rev_info, 1);\n>  \treturn diff_result_code(&rev_info.diffopt, result);\n\nThanks a lot. I've tested this and it looks good to me and fixes the\nbug.\n\n> [From upthread]: It took me a minute to reproduce this. It needs \"pull\n> --rebase\" if you don't have that setup in your config.\n\nYeah, sorry about that. I tested without --rebase while simplifying the\ntestcase, but forgot that I have pull.rebase=true in my ~/.gitconfig, so\nof course that didn't imply --no-rebase.\n"},{"id":"352254","messageId":"20180711150012.GD15269@sigill.intra.peff.net","threadId":"48861","inReplyTo":"87in5ldedd.fsf@evledraar.gmail.com","subject":"Re: [PATCH] has_uncommitted_changes(): fall back to empty tree","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-11T15:00:13Z","receivedAt":"2018-07-11T15:00:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 11, 2018 at 04:41:02PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > [From upthread]: It took me a minute to reproduce this. It needs \"pull\n> > --rebase\" if you don't have that setup in your config.\n> \n> Yeah, sorry about that. I tested without --rebase while simplifying the\n> testcase, but forgot that I have pull.rebase=true in my ~/.gitconfig, so\n> of course that didn't imply --no-rebase.\n\nIn the grand scheme of things, it was still a pretty good bug report. :)\n\nHaving the stack trace meant that the head-scratching was \"why am I not\ncalling the problem function\", rather than \"why am I not reproducing\",\nwhich is much easier to track down.\n\n-Peff\n"},{"id":"352263","messageId":"CACsJy8BM7zJxgeM37YQ4Yh4bH=4iS5CJ2P_-4Vm3oA+cuQYkJA@mail.gmail.com","threadId":"48861","inReplyTo":"87k1q2c9zq.fsf@evledraar.gmail.com","subject":"Re: BUG: Segfault on \"git pull\" on \"bad object HEAD\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-11T15:56:56Z","receivedAt":"2018-07-11T15:57:26Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jul 11, 2018 at 1:02 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> This segfaults, but should print an error instead, have a repo with a\n> corrupt HEAD:\n>\n>     (\n>         rm -rf /tmp/git &&\n>         git clone --single-branch --branch todo git@github.com:git/git.git /tmp/git &&\n>         echo 1111111111111111111111111111111111111111 >/tmp/git/.git/refs/heads/todo &&\n>         git -C /tmp/git pull\n>     )\n>\n> On this repository e.g. \"git log\" will print \"fatal: bad object HEAD\",\n> but for some reason \"git pull\" makes it this far:\n>\n>     $ git pull\n>     Segmentation fault\n>\n> The immediate reason is that in run_diff_index() we have this:\n>\n>         ent = revs->pending.objects;\n>\n> And that in this case that's NULL:\n\nProbably because add_head_to_pending() in has_uncommitted_change()\ndoes not add anything to the \"pending\" list because HEAD is broken.\n\nI think if we make add_head_to_pending() return a boolean, then we can\ncheck that if no HEAD is added, there's no point to run_diff_index and\nhas_uncommitted_changes() can return 0 immediately.\n\nA new BUG() could still be added in run_diff_index() though, to check\nif revs->pending.nr is non-zero before attempting to access\nrevs->pending.objects.\n\n>\n>     (gdb) bt\n>     #0  0x000055555565993f in run_diff_index (revs=0x7fffffffcb90, cached=1) at diff-lib.c:524\n>     #1  0x00005555557633da in has_uncommitted_changes (ignore_submodules=1) at wt-status.c:2345\n>     #2  0x00005555557634c9 in require_clean_work_tree (action=0x555555798f18 \"pull with rebase\", hint=0x555555798efb \"please commit or stash them.\", ignore_submodules=1, gently=0) at wt-status.c:2370\n>     #3  0x00005555555dbdee in cmd_pull (argc=0, argv=0x7fffffffd868, prefix=0x0) at builtin/pull.c:885\n>     #4  0x000055555556c9da in run_builtin (p=0x555555a2de50 <commands+1872>, argc=1, argv=0x7fffffffd868) at git.c:417\n>     #5  0x000055555556cce2 in handle_builtin (argc=1, argv=0x7fffffffd868) at git.c:633\n>     #6  0x000055555556ce8a in run_argv (argcp=0x7fffffffd71c, argv=0x7fffffffd710) at git.c:685\n>     #7  0x000055555556d03f in cmd_main (argc=1, argv=0x7fffffffd868) at git.c:762\n>     #8  0x0000555555611786 in main (argc=3, argv=0x7fffffffd858) at common-main.c:45\n>     (gdb) p revs\n>     $4 = (struct rev_info *) 0x7fffffffcb90\n>     (gdb) p revs->pending\n>     $5 = {nr = 0, alloc = 0, objects = 0x0}\n>     (gdb)\n>\n> This has been an issue since at least v2.8.0 (didn't test back\n> further). I'm not familiar with the status / diff code, so I'm not sure\n> where the assertion should be added.\n>\n> This came up in the wild due to a user with a corrupt repo (don't know\n> how it got corrupt) trying \"git pull\" and seeing git segfault.\n\n\n\n-- \nDuy\n"},{"id":"352274","messageId":"xmqqo9fd3djk.fsf@gitster-ct.c.googlers.com","threadId":"48861","inReplyTo":"20180711133402.GD23835@sigill.intra.peff.net","subject":"Re: BUG: Segfault on \"git pull\" on \"bad object HEAD\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-11T17:09:03Z","receivedAt":"2018-07-11T17:09:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I feel like the right answer here is probably this:\n>\n> diff --git a/wt-status.c b/wt-status.c\n> index d1c05145a4..5fcaa3d0f8 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -2340,7 +2340,16 @@ int has_uncommitted_changes(int ignore_submodules)\n>  \tif (ignore_submodules)\n>  \t\trev_info.diffopt.flags.ignore_submodules = 1;\n>  \trev_info.diffopt.flags.quick = 1;\n> +\n>  \tadd_head_to_pending(&rev_info);\n> +\tif (!rev_info.pending.nr) {\n> +\t\t/*\n> +\t\t * We have no head (or it's corrupt), but the index is not\n> +\t\t * unborn; declare it as uncommitted changes.\n> +\t\t */\n> +\t\treturn 1;\n> +\t}\n> +\n>  \tdiff_setup_done(&rev_info.diffopt);\n>  \tresult = run_diff_index(&rev_info, 1);\n>  \treturn diff_result_code(&rev_info.diffopt, result);\n>\n> That does quietly paper over the corruption, but it does the\n> conservative thing, and a follow-up \"git status\" would yield \"bad\n> object: HEAD\".\n\nSounds quite sensible.\n\n"}]}