{"thread":{"id":"45297","subject":"[PATCH] pull: do not segfault when HEAD refers to missing object file","startedAt":"2017-03-05T23:42:50Z","lastAt":"2017-03-06T07:33:26Z","messageCount":6,"participants":["André Laszlo","brian m. carlson","Jeff King","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313302","messageId":"20170305234222.4590-1-andre@laszlo.nu","threadId":"45297","inReplyTo":null,"subject":"[PATCH] pull: do not segfault when HEAD refers to missing object file","fromName":"André Laszlo","fromEmail":"andre@laszlo.nu","sentAt":"2017-03-05T23:42:22Z","receivedAt":"2017-03-05T23:42:50Z","isPatch":true,"sender":{"key":"andre@laszlo.nu","avatar":null},"body":"git pull --rebase on a corrupted HEAD used to segfault; it has been\ncorrected to error out with a message. A test has also been added to\nverify this fix.\n\nSigned-off-by: André Laszlo <andre@laszlo.nu>\n---\n\nNotes:\n    When add_head_to_pending fails to add a pending object, git pull\n    --rebase segfaults. This can happen if HEAD is referring to a corrupt\n    or missing object.\n    \n    I discovered this segfault on my machine after pulling a repo from\n    GitHub, but I have been unable to reproduce the sequence of events\n    that lead to the corrupted HEAD (I think it may have been caused by a\n    lost network connection in my case).\n    \n    The following commands use add_head_to_pending:\n    \n    format-patch  setup_revisions before add_head_to_pending\n    diff          checks rev.pending.nr\n    shortlog      checks rev.pending.nr\n    log           uses resolve_ref_unsafe\n    \n    All of the above return an error code of 128 and print \"fatal: bad\n    object HEAD\" instead of segfaulting, which I think is correct\n    behavior. The check and error message have been added to\n    has_uncommitted_changes, where they were missing, as well as to\n    diff-lib.c (without the error message).\n\n diff-lib.c      |  2 +-\n t/t5520-pull.sh | 12 ++++++++++++\n wt-status.c     |  5 +++++\n 3 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 52447466b..9d26b18c3 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -512,7 +512,7 @@ int run_diff_index(struct rev_info *revs, int cached)\n \tstruct object_array_entry *ent;\n \n \tent = revs->pending.objects;\n-\tif (diff_cache(revs, ent->item->oid.hash, ent->name, cached))\n+\tif (!ent || diff_cache(revs, ent->item->oid.hash, ent->name, cached))\n \t\texit(128);\n \n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"c/\", cached ? \"i/\" : \"w/\");\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 17f4d0fe4..1edb6a97a 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -664,4 +664,16 @@ test_expect_success 'git pull --rebase against local branch' '\n \ttest file = \"$(cat file2)\"\n '\n \n+test_expect_success 'git pull --rebase with corrupt HEAD does not segfault' '\n+\tmkdir corrupted &&\n+\t(cd corrupted &&\n+\tgit init &&\n+\techo one >file && git add file &&\n+\tgit commit -m one &&\n+\tREV=$(git rev-parse HEAD) &&\n+\trm -f .git/objects/${REV:0:2}/${REV:2} &&\n+\ttest_expect_code 128 git pull --rebase > /dev/null\n+\t)\n+'\n+\n test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex d47012048..3d60eaff5 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -2252,6 +2252,11 @@ int has_uncommitted_changes(int ignore_submodules)\n \t\tDIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n \tDIFF_OPT_SET(&rev_info.diffopt, QUICK);\n \tadd_head_to_pending(&rev_info);\n+\n+\t/* The add_head_to_pending call might not have added anything. */\n+\tif (!rev_info.pending.nr)\n+\t\tdie(\"bad object %s\", \"HEAD\");\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.12.0\n\n"},{"id":"313304","messageId":"20170305235222.vxia7jw2n5uj2h2e@genre.crustytoothpaste.net","threadId":"45297","inReplyTo":"20170305234222.4590-1-andre@laszlo.nu","subject":"Re: [PATCH] pull: do not segfault when HEAD refers to missing object file","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-03-05T23:52:22Z","receivedAt":"2017-03-05T23:54:20Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Mar 06, 2017 at 12:42:22AM +0100, André Laszlo wrote:\n> +test_expect_success 'git pull --rebase with corrupt HEAD does not segfault' '\n> +\tmkdir corrupted &&\n> +\t(cd corrupted &&\n> +\tgit init &&\n> +\techo one >file && git add file &&\n> +\tgit commit -m one &&\n> +\tREV=$(git rev-parse HEAD) &&\n> +\trm -f .git/objects/${REV:0:2}/${REV:2} &&\n\nI think this is a bashism.  On dash, I get the following:\n\n  genre ok % dash -c 'foo=abcdefg; echo ${foo:0:2}; echo ${foo:2}'\n  dash: 1: Bad substitution\n\n> +\ttest_expect_code 128 git pull --rebase > /dev/null\n> +\t)\n> +'\n> +\n>  test_done\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"313307","messageId":"20170306035152.c7bh5jiqrfncyudl@sigill.intra.peff.net","threadId":"45297","inReplyTo":"20170305235222.vxia7jw2n5uj2h2e@genre.crustytoothpaste.net","subject":"Re: [PATCH] pull: do not segfault when HEAD refers to missing object file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-06T03:51:52Z","receivedAt":"2017-03-06T03:54:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 05, 2017 at 11:52:22PM +0000, brian m. carlson wrote:\n\n> On Mon, Mar 06, 2017 at 12:42:22AM +0100, André Laszlo wrote:\n> > +test_expect_success 'git pull --rebase with corrupt HEAD does not segfault' '\n> > +\tmkdir corrupted &&\n> > +\t(cd corrupted &&\n> > +\tgit init &&\n> > +\techo one >file && git add file &&\n> > +\tgit commit -m one &&\n> > +\tREV=$(git rev-parse HEAD) &&\n> > +\trm -f .git/objects/${REV:0:2}/${REV:2} &&\n> \n> I think this is a bashism.  On dash, I get the following:\n> \n>   genre ok % dash -c 'foo=abcdefg; echo ${foo:0:2}; echo ${foo:2}'\n>   dash: 1: Bad substitution\n\nYeah, it is. You can do it easily with 'sed', of course, but if you want\nto avoid the extra process and do it in pure shell, it's more like:\n\n  last38=${REV#??}\n  first2=${REV%$last38}\n  rm -f .git/objects/$first2/$last38\n\n-Peff\n"},{"id":"313310","messageId":"6726b36d-6f50-d258-12e3-8b7b56159631@kdbg.org","threadId":"45297","inReplyTo":"20170306035152.c7bh5jiqrfncyudl@sigill.intra.peff.net","subject":"Re: [PATCH] pull: do not segfault when HEAD refers to missing object file","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-03-06T06:52:10Z","receivedAt":"2017-03-06T06:57:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 06.03.2017 um 04:51 schrieb Jeff King:\n> On Sun, Mar 05, 2017 at 11:52:22PM +0000, brian m. carlson wrote:\n>\n>> On Mon, Mar 06, 2017 at 12:42:22AM +0100, André Laszlo wrote:\n>>> +test_expect_success 'git pull --rebase with corrupt HEAD does not segfault' '\n>>> +\tmkdir corrupted &&\n>>> +\t(cd corrupted &&\n\nWe usally indent this like so:\n\n\t(\n\t\tcd corrupted &&\n\t\techo one >file &&\n\t\tgit add file &&\n...\n\t) &&\n\n>>> +\tgit init &&\n>>> +\techo one >file && git add file &&\n>>> +\tgit commit -m one &&\n>>> +\tREV=$(git rev-parse HEAD) &&\n>>> +\trm -f .git/objects/${REV:0:2}/${REV:2} &&\n>>\n>> I think this is a bashism.  On dash, I get the following:\n>>\n>>   genre ok % dash -c 'foo=abcdefg; echo ${foo:0:2}; echo ${foo:2}'\n>>   dash: 1: Bad substitution\n>\n> Yeah, it is. You can do it easily with 'sed', of course, but if you want\n> to avoid the extra process and do it in pure shell, it's more like:\n>\n>   last38=${REV#??}\n>   first2=${REV%$last38}\n>   rm -f .git/objects/$first2/$last38\n\nIs it \"HEAD points to non-existent object\" or \".git/HEAD contains junk\"? \nIn both cases there are simpler solutions than to remove an object. For \nexample, `echo \"$_x40\" >.git/HEAD` or `echo \"this is junk\" >.git/HEAD`?\n\n-- Hannes\n\n"},{"id":"313311","messageId":"20170306034639.tji4s4m5mnzou2m2@sigill.intra.peff.net","threadId":"45297","inReplyTo":"20170305234222.4590-1-andre@laszlo.nu","subject":"Re: [PATCH] pull: do not segfault when HEAD refers to missing object file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-06T03:46:40Z","receivedAt":"2017-03-06T07:04:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 06, 2017 at 12:42:22AM +0100, André Laszlo wrote:\n\n> git pull --rebase on a corrupted HEAD used to segfault;it has been\n> corrected to error out with a message. A test has also been added to\n> verify this fix.\n\nYour commit message mostly tells us the \"what\" that we can see from the\ndiff. But why are we segfaulting? What assumption is being violated, and\nwhy is the fix the right thing?\n\nYou've stuck some of the details in your notes, and they should probably\nmake it into the commit message.\n\n>     When add_head_to_pending fails to add a pending object, git pull\n>     --rebase segfaults. This can happen if HEAD is referring to a corrupt\n>     or missing object.\n\nThe other obvious time when HEAD is not valid is when you are on an\nunborn branch. So we should also consider how such a case interacts with\nthe callsites you are touching.\n\nI think it is primarily this hunk:\n\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -2252,6 +2252,11 @@ int has_uncommitted_changes(int ignore_submodules)\n>  \t\tDIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);\n>  \tDIFF_OPT_SET(&rev_info.diffopt, QUICK);\n>  \tadd_head_to_pending(&rev_info);\n> +\n> +\t/* The add_head_to_pending call might not have added anything. */\n> +\tif (!rev_info.pending.nr)\n> +\t\tdie(\"bad object %s\", \"HEAD\");\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 is the fix. We assume that add_head_to_pending() put something into\nrev_info.pending, but it might not.\n\nIn the case of corruption, \"bad object HEAD\" is a reasonable outcome.\n\nIn the case of an unborn branch, we'd probably want to compare against\nthe empty tree. This trick is used elsewhere in wt-status.c, as in\nwt_status_collect_changes_index().\n\nI'm not sure if we need to deal with that here or not. I wasn't able to\ntrigger this code with an unborn branch. If you have no index, then the\nis_cache_unborn() check triggers earlier in the function. If you do have\nan index, then we have an earlier check:\n\n  if (is_null_sha1(orig_head) && !is_cache_unborn())\n\tdie(_(\"Updating an unborn branch with changes added to the index.\"));\n\nthat covers this case. I am not 100% sure that check is correct, though.\nIf I do:\n\n  git init\n  echo content >foo\n  git add foo\n  git rm -f foo\n\nthen I have an index which is not \"unborn\", but also does not contain\nany changes. I think the conditional above should just be checking\n\"active_nr\". If we were to fix that, then I think\nhas_uncommitted_changes() would need to be adjusted as well.\n\nI admit that this is probably an absurd corner case.  So I think I am OK\nwith your fix for now, and we can revisit the logic later if anybody\nstarts to care.\n\n>     I discovered this segfault on my machine after pulling a repo from\n>     GitHub, but I have been unable to reproduce the sequence of events\n>     that lead to the corrupted HEAD (I think it may have been caused by a\n>     lost network connection in my case).\n\nYikes. That should never lead to a corrupted HEAD (unless you are on a\nnetworked filesystem). I can't think of another read add_head_to_pending\nwould fail, though, aside from a missing or corrupted object.\n\nArguably add_head_to_pending() should die loudly if it can't parse the\nobject pointed to by HEAD, rather than quietly returning without adding\nanything.\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 52447466b..9d26b18c3 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -512,7 +512,7 @@ int run_diff_index(struct rev_info *revs, int cached)\n>  \tstruct object_array_entry *ent;\n>  \n>  \tent = revs->pending.objects;\n> -\tif (diff_cache(revs, ent->item->oid.hash, ent->name, cached))\n> +\tif (!ent || diff_cache(revs, ent->item->oid.hash, ent->name, cached))\n>  \t\texit(128);\n\nSo if I understand correctly, this hunk should not trigger anymore,\nbecause we would never call run_diff_index() without something in\npending.\n\nI think it would be a programming error elsewhere to do so, and we\nshould treat this as an assertion by complaining loudly (for this and\nany other confusing cases):\n\n  if (revs->pending.nr != 1)\n          die(\"BUG: run_diff_index requires exactly one rev (got %d)\",\n\t      revs->pending.nr);\n\nLastly, I think this is your first patch. So welcome, and thank you. :)\nI know there was a lot of discussion and critique above, but I think\noverall your patch is going in the right direction.\n\n-Peff\n\n"},{"id":"313312","messageId":"20170306073317.p2txecat56zvzbmz@sigill.intra.peff.net","threadId":"45297","inReplyTo":"6726b36d-6f50-d258-12e3-8b7b56159631@kdbg.org","subject":"Re: [PATCH] pull: do not segfault when HEAD refers to missing object file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-06T07:33:17Z","receivedAt":"2017-03-06T07:33:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 06, 2017 at 07:52:10AM +0100, Johannes Sixt wrote:\n\n> > Yeah, it is. You can do it easily with 'sed', of course, but if you want\n> > to avoid the extra process and do it in pure shell, it's more like:\n> > \n> >   last38=${REV#??}\n> >   first2=${REV%$last38}\n> >   rm -f .git/objects/$first2/$last38\n> \n> Is it \"HEAD points to non-existent object\" or \".git/HEAD contains junk\"? In\n> both cases there are simpler solutions than to remove an object. For\n> example, `echo \"$_x40\" >.git/HEAD` or `echo \"this is junk\" >.git/HEAD`?\n\nGood point. Unfortunately, it's a little tricky. You can't use \"this is\njunk\" because then git does not recognize it as a git repo at all. You\ncan't use $_x40 because the null sha1 is used internally to signal an\nunborn branch, so we mix it up with that case.\n\nSomething like \"111...\" for 40 characters would work, though at that\npoint I think just simulating an actual corruption might be less\nconvoluted.\n\n-Peff\n"}]}