{"thread":{"id":"41754","subject":"[PATCH] Fixing segmentation fault when merging FETCH_HEAD","startedAt":"2016-03-19T18:17:40Z","lastAt":"2016-03-21T19:36:45Z","messageCount":6,"participants":["Jose Ivan B. Vilarouca Filho","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"281245","messageId":"1458411460-26146-1-git-send-email-joseivan@lavid.ufpb.br","threadId":"41754","inReplyTo":null,"subject":"[PATCH] Fixing segmentation fault when merging FETCH_HEAD","fromName":"Jose Ivan B. Vilarouca Filho","fromEmail":"joseivan@lavid.ufpb.br","sentAt":"2016-03-19T18:17:40Z","receivedAt":"2016-03-19T18:17:40Z","isPatch":true,"sender":{"key":"joseivan@lavid.ufpb.br","avatar":null},"body":"From: \"Jose Ivan B. Vilarouca Filho\" \n\nA segmentaion fault is raised when trying to merge FETCH_HEAD\nformed only by \"not-for-merge\" refs.\n\nEx:\n git init .\n git remote add origin ...\n git fetch origin\n git merge FETCH_HEAD\n\nSigned-off-by: Jose Ivan B. Vilarouca Filho \n---\n builtin/merge.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 101ffef..7e419dc 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1270,9 +1270,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t \"an empty head\"));\n \t\tremoteheads = collect_parents(head_commit, &head_subsumed,\n \t\t\t\t\t argc, argv, NULL);\n-\t\tremote_head = remoteheads->item;\n-\t\tif (!remote_head)\n+\t\tif ((!remoteheads) || (!remoteheads->item))\n \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n+\t\tremote_head = remoteheads->item;\n \t\tif (remoteheads->next)\n \t\t\tdie(_(\"Can merge only exactly one commit into empty head\"));\n \t\tread_empty(remote_head->object.oid.hash, 0);\n-- \n1.7.10.4\n"},{"id":"281250","messageId":"CAPig+cRVtzLjKTW7FZ-h8thEXkTqBJtbSwpKJs3+wdRHYV1qrQ@mail.gmail.com","threadId":"41754","inReplyTo":"1458411460-26146-1-git-send-email-joseivan@lavid.ufpb.br","subject":"Re: [PATCH] Fixing segmentation fault when merging FETCH_HEAD","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-20T01:23:43Z","receivedAt":"2016-03-20T01:23:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 19, 2016 at 2:17 PM, Jose Ivan B. Vilarouca Filho\n wrote:\n> From: \"Jose Ivan B. Vilarouca Filho\" \n\nYou can drop this line since it is the same as the From: line in the\nemail envelope.\n\n> Fixing segmentation fault when merging FETCH_HEAD\n\nAlternate:\n\n merge: don't dereference NULL pointer\n\n> A segmentaion fault is raised when trying to merge FETCH_HEAD\n> formed only by \"not-for-merge\" refs.\n>\n> Ex:\n> git init .\n> git remote add origin ...\n> git fetch origin\n> git merge FETCH_HEAD\n\nCan you add a test to ensure that some future change doesn't regress\nthis fix? The above recipe would make a good basis for the new test.\n\n> Signed-off-by: Jose Ivan B. Vilarouca Filho \n> ---\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> @@ -1270,9 +1270,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \"an empty head\"));\n> remoteheads = collect_parents(head_commit, &head_subsumed,\n> argc, argv, NULL);\n> - remote_head = remoteheads->item;\n> - if (!remote_head)\n> + if ((!remoteheads) || (!remoteheads->item))\n\nStyle: drop unnecessary parantheses\n\n if (!remoteheads || !remoteheads->item)\n\n> die(_(\"%s - not something we can merge\"), argv[0]);\n> + remote_head = remoteheads->item;\n> if (remoteheads->next)\n> die(_(\"Can merge only exactly one commit into empty head\"));\n> read_empty(remote_head->object.oid.hash, 0);\n> --\n> 1.7.10.4\n"},{"id":"281315","messageId":"1458519102-32621-1-git-send-email-joseivan@lavid.ufpb.br","threadId":"41754","inReplyTo":"CAPig+cRVtzLjKTW7FZ-h8thEXkTqBJtbSwpKJs3+wdRHYV1qrQ@mail.gmail.com","subject":"Re: [PATCH] Fixing segmentation fault when merging FETCH_HEAD","fromName":"Jose Ivan B. Vilarouca Filho","fromEmail":"joseivan@lavid.ufpb.br","sentAt":"2016-03-21T00:11:42Z","receivedAt":"2016-03-21T00:11:42Z","isPatch":true,"sender":{"key":"joseivan@lavid.ufpb.br","avatar":null},"body":"From: \"Jose Ivan B. Vilarouca Filho\" \n\nHello, Eric.\n\nThanks for suggestions. I've added a test in commit replacing git fetch origin by a fake FETCH_HEAD content.\n\nmerge: don't dereference NULL pointer\n\nA segmentaion fault is raised when trying to merge FETCH_HEAD\nformed only by \"not-for-merge\" refs.\n\nEx:\n git init .\n git remote add origin ...\n git fetch origin\n git merge FETCH_HEAD\n\nSigned-off-by: Jose Ivan B. Vilarouca Filho \n---\n builtin/merge.c | 4 ++--\n test-merge-fetchhead.sh | 23 +++++++++++++++++++++++\n 2 files changed, 25 insertions(+), 2 deletions(-)\n create mode 100755 test-merge-fetchhead.sh\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 101ffef..098aa8a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1270,9 +1270,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t \"an empty head\"));\n \t\tremoteheads = collect_parents(head_commit, &head_subsumed,\n \t\t\t\t\t argc, argv, NULL);\n-\t\tremote_head = remoteheads->item;\n-\t\tif (!remote_head)\n+\t\tif (!remoteheads || !remoteheads->item)\n \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n+\t\tremote_head = remoteheads->item;\n \t\tif (remoteheads->next)\n \t\t\tdie(_(\"Can merge only exactly one commit into empty head\"));\n \t\tread_empty(remote_head->object.oid.hash, 0);\ndiff --git a/test-merge-fetchhead.sh b/test-merge-fetchhead.sh\nnew file mode 100755\nindex 0000000..383415a\n--- /dev/null\n+++ b/test-merge-fetchhead.sh\n@@ -0,0 +1,23 @@\n+#!/bin/bash\n+GIT=$(pwd)/git\n+REPO_DIR=./test-fetch-head-repo\n+\n+if [ ! -x \"${GIT}\" ]; then\n+ echo \"Missing git binary\"\n+ exit 1\n+fi\n+\n+${GIT} init ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1\n+pushd ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1\n+#Let's fake a FETCH_HEAD only formed by not-for-merge (git fetch origin)\n+echo -ne \"f954fc9919c9ec33179e11aa1af562384677e174\\tnot-for-merge\\tbranch 'master' of https://github.com/git/git.git\" > .git/FETCH_HEAD\n+${GIT} merge FETCH_HEAD\n+GRET=$?\n+popd\n+if [ ${GRET} -eq 139 ]; then\n+ rm -rf ${REPO_DIR}\n+ exit 1\n+fi\n+\n+rm -rf ${REPO_DIR}\n+exit 0\n-- \n1.7.10.4\n"},{"id":"281329","messageId":"CAPig+cTGvYLP98NyPJpOY=kMML=ieTikotZvYowhxzZwWdU0_Q@mail.gmail.com","threadId":"41754","inReplyTo":"1458519102-32621-1-git-send-email-joseivan@lavid.ufpb.br","subject":"Re: [PATCH] Fixing segmentation fault when merging FETCH_HEAD","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-21T06:40:00Z","receivedAt":"2016-03-21T06:40:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 20, 2016 at 8:11 PM, Jose Ivan B. Vilarouca Filho\n wrote:\n> Hello, Eric.\n>\n> Thanks for suggestions. I've added a test in commit replacing git fetch origin by a fake FETCH_HEAD content.\n\nThanks for the re-roll. To be \"git am\"-friendly, you should either\nplace a \"-- >8 --\" scissor line after the above commentary, or use\n\"git send-email\" to send the patch, and place the commentary just\nbelow the \"---\" line following your sign-off.\n\nMore below...\n\n> merge: don't dereference NULL pointer\n>\n> A segmentaion fault is raised when trying to merge FETCH_HEAD\n> formed only by \"not-for-merge\" refs.\n>\n> Ex:\n> git init .\n> git remote add origin ...\n> git fetch origin\n> git merge FETCH_HEAD\n>\n> Signed-off-by: Jose Ivan B. Vilarouca Filho \n> ---\n> diff --git a/test-merge-fetchhead.sh b/test-merge-fetchhead.sh\n\nAs you're new around here, I probably should have been more specific\nin my previous review and explained that you'd want your new test to\nbe incorporated into the existing test suite so that it actually gets\nexercised regularly. A good place for the new test might be at the\nbottom of t/t7600-merge.sh.\n\nWriting a new test is pretty simple, especially when you already have\na recipe for reproduction. Take a look at existing tests in that file\nto get a feel for how it should be done. Rather than all the \"|| exit\n1\"'s, chain your test statements together with &&. And, because you'll\nbe incorporating the new test into an existing script, you can drop a\nlot of the boilerplate below and just focus on the recipe.\n\nSome style comments below (most of which won't matter after you drop\nthe boilerplate but are generally good to know)...\n\n> @@ -0,0 +1,23 @@\n> +#!/bin/bash\n> +GIT=$(pwd)/git\n> +REPO_DIR=./test-fetch-head-repo\n> +\n> +if [ ! -x \"${GIT}\" ]; then\n\nUse 'test' rather than '['. Drop the semicolon. Place 'then' on its own line.\n\n> + echo \"Missing git binary\"\n> + exit 1\n> +fi\n> +\n> +${GIT} init ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1\n> +pushd ${REPO_DIR} || rm -rf ${REPO_DIR} || exit 1\n\nIn test scripts you need to take extra care when switching directories\nsince failure of a command following 'pushd' will prevent the\nsubsequent 'popd' from executing, and then tests later in the script\nwill be invoked in the wrong directory. The typical way of dealing\nwith this is to use a subhsell since 'cd' within a subshell doesn't\naffect the parent, and the parent continues at its own working\ndirectory when the subshell exits. For example:\n\n some-command &&\n (\n cd somewhere &&\n command-which-might-fail &&\n another-possible-failure\n )\n\n> +#Let's fake a FETCH_HEAD only formed by not-for-merge (git fetch origin)\n> +echo -ne \"f954fc9919c9ec33179e11aa1af562384677e174\\tnot-for-merge\\tbranch 'master' of https://github.com/git/git.git\" > .git/FETCH_HEAD\n\nNon-portable 'echo' options. Use 'printf' instead. And, you'll need to\ntake extra care with the single-quotes since the test body itself will\nbe within single-quotes.\n\n> +${GIT} merge FETCH_HEAD\n> +GRET=$?\n> +popd\n> +if [ ${GRET} -eq 139 ]; then\n\nSo, both before and after this patch, this git-merge fails, and you\nwant the test to detect that it's failing in a controlled way (via\ndie()) rather than the previous uncontrolled way (via segfault). You\ncould use test_expect_code() for this (from t/test-lib-functions.sh),\nor you could capture stderr and verify that it has the expected\nfailure message from die(). The latter is probably preferable, and may\nlook something like this:\n\ntest_expect_success 'my test title' '\n ...setup stuff... &&\n test_must_fail git merge FETCH_HEAD 2>err &&\n test_i18ngrep \"not something we can merge\" err\n'\n\n> + rm -rf ${REPO_DIR}\n> + exit 1\n> +fi\n> +\n> +rm -rf ${REPO_DIR}\n> +exit 0\n\nYou typically don't need to cleanup after your test, so this\nboilerplate can go away.\n"},{"id":"281361","messageId":"xmqq7fgvmz8o.fsf_-_@gitster.mtv.corp.google.com","threadId":"41754","inReplyTo":"1458519102-32621-1-git-send-email-joseivan@lavid.ufpb.br","subject":"merge: fix NULL pointer dereference when merging nothing into void","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-21T19:01:43Z","receivedAt":"2016-03-21T19:01:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we are on an unborn branch and merging only one foreign parent,\nwe allow \"git merge\" to fast-forward to that foreign parent commit.\n\nThis codepath incorrectly attempted to dereference the list of\nparents that the merge is going to record even when the list is\nempty. It must refuse to operate instead when there is no parent.\n\nAll other codepaths make sure the list is not empty before they\ndereference it, and are safe.\n\nReported by Jose Ivan B. Vilarouca Filho\nSigned-off-by: Junio C Hamano \n---\n\n * So here is what I came up with as a suggestion. The original\n check to see if remote_head is empty is simply bogus (an empty\n list would to have a single element whose item is NULL), so I\n rewrote it to clarify what is going on in this codepath.\n\n builtin/merge.c | 10 +++++-----\n t/t7600-merge.sh | 10 ++++++++++\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 101ffef..bf2f261 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1257,12 +1257,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_merge_options);\n \n \tif (!head_commit) {\n-\t\tstruct commit *remote_head;\n \t\t/*\n \t\t * If the merged head is a valid one there is no reason\n \t\t * to forbid \"git merge\" into a branch yet to be born.\n \t\t * We do the same for \"git pull\".\n \t\t */\n+\t\tunsigned char *remote_head_sha1;\n \t\tif (squash)\n \t\t\tdie(_(\"Squash commit into empty head not supported yet\"));\n \t\tif (fast_forward == FF_NO)\n@@ -1270,13 +1270,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t \"an empty head\"));\n \t\tremoteheads = collect_parents(head_commit, &head_subsumed,\n \t\t\t\t\t argc, argv, NULL);\n-\t\tremote_head = remoteheads->item;\n-\t\tif (!remote_head)\n+\t\tif (!remoteheads)\n \t\t\tdie(_(\"%s - not something we can merge\"), argv[0]);\n \t\tif (remoteheads->next)\n \t\t\tdie(_(\"Can merge only exactly one commit into empty head\"));\n-\t\tread_empty(remote_head->object.oid.hash, 0);\n-\t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->object.oid.hash,\n+\t\tremote_head_sha1 = remoteheads->item->object.oid.hash;\n+\t\tread_empty(remote_head_sha1, 0);\n+\t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head_sha1,\n \t\t\t NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n \t\tgoto done;\n \t}\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 302e238..9d7952f 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -725,4 +725,14 @@ test_expect_success 'merge detects mod-256 conflicts (resolve)' '\n \ttest_must_fail git merge -s resolve master\n '\n \n+test_expect_success 'merge nothing into void' '\n+\tgit init void &&\n+\t(\n+\t\tcd void &&\n+\t\tgit remote add up .. &&\n+\t\tgit fetch up &&\n+\t\ttest_must_fail git merge FETCH_HEAD\n+\t)\n+'\n+\n test_done\n"},{"id":"281369","messageId":"CAPig+cQjt==AKTWfCk6kzgVciGVb9XtKNiwPK14nPM7=4yhmjQ@mail.gmail.com","threadId":"41754","inReplyTo":"xmqq7fgvmz8o.fsf_-_@gitster.mtv.corp.google.com","subject":"Re: merge: fix NULL pointer dereference when merging nothing into void","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-21T19:36:45Z","receivedAt":"2016-03-21T19:36:45Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 21, 2016 at 3:01 PM, Junio C Hamano wrote:\n> When we are on an unborn branch and merging only one foreign parent,\n> we allow \"git merge\" to fast-forward to that foreign parent commit.\n>\n> This codepath incorrectly attempted to dereference the list of\n> parents that the merge is going to record even when the list is\n> empty. It must refuse to operate instead when there is no parent.\n>\n> All other codepaths make sure the list is not empty before they\n> dereference it, and are safe.\n>\n> Reported by Jose Ivan B. Vilarouca Filho\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> @@ -725,4 +725,14 @@ test_expect_success 'merge detects mod-256 conflicts (resolve)' '\n> +test_expect_success 'merge nothing into void' '\n> + git init void &&\n> + (\n> + cd void &&\n> + git remote add up .. &&\n> + git fetch up &&\n> + test_must_fail git merge FETCH_HEAD\n\nAh, nice. I either didn't know or had forgotten that test_must_fail is\nsmart enough to detect unexpected failures (such as segfault), so my\nadvice to Jose about capturing stderr[1] was misdirected. Thanks.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/289405\n\n> + )\n> +'\n> +\n> test_done\n"}]}