{"thread":{"id":"48557","subject":"BUG: rev-parse segfault with invalid input","startedAt":"2018-05-23T19:52:35Z","lastAt":"2018-05-25T01:07:38Z","messageCount":14,"participants":["Todd Zullinger","Elijah Newren","Jeff King","Florian Weimer","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"348385","messageId":"20180523195227.GT26695@zaya.teonanacatl.net","threadId":"48557","inReplyTo":null,"subject":"BUG: rev-parse segfault with invalid input","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2018-05-23T19:52:27Z","receivedAt":"2018-05-23T19:52:35Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nCertain invalid input causes git rev-parse to crash rather\nthan return a 'fatal: ambiguous argument ...' error.\n\nThis was reported against the Fedora git package:\n\n https://bugzilla.redhat.com/1581678\n\nSimple reproduction recipe and analysis, from the bug:\n\n $ git init\n Initialized empty Git repository in /tmp/t/.git/\n $ git rev-parse ffffffffffffffffffffffffffffffffffffffff^@\n Segmentation fault (core dumped)\n\n gdb) break lookup_commit_reference\n Breakpoint 1 at 0x555555609f00: lookup_commit_reference. (3 locations)\n (gdb) r\n Starting program: /usr/bin/git rev-parse ffffffffffffffffffffffffffffffffffffffff\\^@\n [Thread debugging using libthread_db enabled]\n Using host libthread_db library \"/lib64/libthread_db.so.1\".\n\n Breakpoint 1, lookup_commit_reference (oid=oid@entry=0x7fffffffd550) at commit.c:34\n 34 return lookup_commit_reference_gently(oid, 0);\n (gdb) finish\n Run till exit from #0 lookup_commit_reference (oid=oid@entry=0x7fffffffd550) at commit.c:34\n try_parent_shorthands (arg=0x7fffffffdd44 'f' ) at builtin/rev-parse.c:314\n 314 include_parents = 1;\n Value returned is $1 = (struct commit *) 0x0\n (gdb) c\n\n (gdb) c\n Continuing.\n\n Program received signal SIGSEGV, Segmentation fault.\n try_parent_shorthands (arg=0x7fffffffdd44 'f' ) at builtin/rev-parse.c:345\n 345 for (parents = commit->parents, parent_number = 1;\n (gdb) l 336,+15\n 336 commit = lookup_commit_reference(&oid);\n 337 if (exclude_parent &&\n 338 exclude_parent > commit_list_count(commit->parents)) {\n 339 *dotdot = '^';\n 340 return 0;\n 341 }\n 342 \n 343 if (include_rev)\n 344 show_rev(NORMAL, &oid, arg);\n 345 for (parents = commit->parents, parent_number = 1;\n 346 parents;\n 347 parents = parents->next, parent_number++) {\n 348 char *name = NULL;\n 349 \n 350 if (exclude_parent && parent_number != exclude_parent)\n 351 continue;\n\n Looks like a null pointer check is missing.\n\nThis occurs on master and as far back as 1.8.3.1 (what's in\nRHEL-6, I didn't try to test anything older). Only a string\nwith 40 valid hex characters and ^@, @-, of ^! seems to\ntrigger it.\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nI don't mind arguing with myself. It's when I lose that it bothers me.\n -- Richard Powers\n\n"},{"id":"348386","messageId":"CABPp-BFOwWvDpfLFa2yrUDU_3BU6F68oLTtO5FvQo8nr62_WtQ@mail.gmail.com","threadId":"48557","inReplyTo":"20180523195227.GT26695@zaya.teonanacatl.net","subject":"Re: BUG: rev-parse segfault with invalid input","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-23T20:23:23Z","receivedAt":"2018-05-23T20:23:40Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, May 23, 2018 at 12:52 PM, Todd Zullinger wrote:\n> Hi,\n>\n> Certain invalid input causes git rev-parse to crash rather\n> than return a 'fatal: ambiguous argument ...' error.\n>\n> This was reported against the Fedora git package:\n>\n> https://bugzilla.redhat.com/1581678\n>\n> Simple reproduction recipe and analysis, from the bug:\n>\n> $ git init\n> Initialized empty Git repository in /tmp/t/.git/\n> $ git rev-parse ffffffffffffffffffffffffffffffffffffffff^@\n> Segmentation fault (core dumped)\n>\n> gdb) break lookup_commit_reference\n> Breakpoint 1 at 0x555555609f00: lookup_commit_reference. (3 locations)\n> (gdb) r\n> Starting program: /usr/bin/git rev-parse ffffffffffffffffffffffffffffffffffffffff\\^@\n> [Thread debugging using libthread_db enabled]\n> Using host libthread_db library \"/lib64/libthread_db.so.1\".\n>\n> Breakpoint 1, lookup_commit_reference (oid=oid@entry=0x7fffffffd550) at commit.c:34\n> 34 return lookup_commit_reference_gently(oid, 0);\n> (gdb) finish\n> Run till exit from #0 lookup_commit_reference (oid=oid@entry=0x7fffffffd550) at commit.c:34\n> try_parent_shorthands (arg=0x7fffffffdd44 'f' ) at builtin/rev-parse.c:314\n> 314 include_parents = 1;\n> Value returned is $1 = (struct commit *) 0x0\n> (gdb) c\n>\n> (gdb) c\n> Continuing.\n>\n> Program received signal SIGSEGV, Segmentation fault.\n> try_parent_shorthands (arg=0x7fffffffdd44 'f' ) at builtin/rev-parse.c:345\n> 345 for (parents = commit->parents, parent_number = 1;\n> (gdb) l 336,+15\n> 336 commit = lookup_commit_reference(&oid);\n> 337 if (exclude_parent &&\n> 338 exclude_parent > commit_list_count(commit->parents)) {\n> 339 *dotdot = '^';\n> 340 return 0;\n> 341 }\n> 342\n> 343 if (include_rev)\n> 344 show_rev(NORMAL, &oid, arg);\n> 345 for (parents = commit->parents, parent_number = 1;\n> 346 parents;\n> 347 parents = parents->next, parent_number++) {\n> 348 char *name = NULL;\n> 349\n> 350 if (exclude_parent && parent_number != exclude_parent)\n> 351 continue;\n>\n> Looks like a null pointer check is missing.\n>\n> This occurs on master and as far back as 1.8.3.1 (what's in\n> RHEL-6, I didn't try to test anything older). Only a string\n> with 40 valid hex characters and ^@, @-, of ^! seems to\n> trigger it.\n\nThanks for the detailed report. This apparently goes back to\ngit-1.6.0 with commit 2122f8b963d4 (\"rev-parse: Add support for the ^!\nand ^@ syntax\", 2008-07-26). We aren't checking that the commit from\nlookup_commit_reference() is non-NULL before proceeding. Looks like\nit's simple to fix. I'll send a patch shortly...\n"},{"id":"348387","messageId":"20180523204502.GU26695@zaya.teonanacatl.net","threadId":"48557","inReplyTo":"CABPp-BFOwWvDpfLFa2yrUDU_3BU6F68oLTtO5FvQo8nr62_WtQ@mail.gmail.com","subject":"Re: BUG: rev-parse segfault with invalid input","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2018-05-23T20:45:03Z","receivedAt":"2018-05-23T20:45:09Z","isPatch":false,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Hi,\n\nElijah Newren wrote:\n> Thanks for the detailed report. This apparently goes back to\n> git-1.6.0 with commit 2122f8b963d4 (\"rev-parse: Add support for the ^!\n> and ^@ syntax\", 2008-07-26). We aren't checking that the commit from\n> lookup_commit_reference() is non-NULL before proceeding. Looks like\n> it's simple to fix. I'll send a patch shortly...\n\nThanks Elijah! I thought it was likely to be a simple fix.\nBut I also don't know the area well and that kept me from\nbeing too ambitious about suggesting a fix or the difficulty\nof one. :)\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nI believe in the noble, aristocratic art of doing absolutely nothing.\nAnd someday, I hope to be in a position where I can do even less.\n\n"},{"id":"348388","messageId":"20180523204613.11333-1-newren@gmail.com","threadId":"48557","inReplyTo":"CABPp-BFOwWvDpfLFa2yrUDU_3BU6F68oLTtO5FvQo8nr62_WtQ@mail.gmail.com","subject":"[PATCH 1/2] t6101: add a test for rev-parse $garbage^@","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-23T20:46:12Z","receivedAt":"2018-05-23T20:46:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Reported by Florian Weimer and Todd Zullinger.\n\nSigned-off-by: Elijah Newren \n---\n t/t6101-rev-parse-parents.sh | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex 8c617981a3..7b1b2dbdf2 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -214,4 +214,8 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '\n \ttest_must_fail git rev-list merge^-1x\n '\n \n+test_expect_failure 'rev-parse $garbage^@ should not segfault' '\n+\tgit rev-parse ffffffffffffffffffffffffffffffffffffffff^@\n+'\n+\n test_done\n-- \n2.17.0.1025.g36b5c64692\n\n"},{"id":"348389","messageId":"20180523204613.11333-2-newren@gmail.com","threadId":"48557","inReplyTo":"20180523204613.11333-1-newren@gmail.com","subject":"[PATCH 2/2] rev-parse: verify that commit looked up is not NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-23T20:46:13Z","receivedAt":"2018-05-23T20:46:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"In commit 2122f8b963d4 (\"rev-parse: Add support for the ^! and ^@ syntax\",\n2008-07-26), try_parent_shorthands() was introduced to parse the special\n^! and ^@ syntax. However, it did not check the commit returned from\nlookup_commit_reference() before proceeding to use it. If it is NULL,\nbail early and notify the caller that this cannot be a valid revision\nrange.\n\nSigned-off-by: Elijah Newren \n---\n builtin/rev-parse.c | 2 ++\n t/t6101-rev-parse-parents.sh | 2 +-\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 55c0b90441..4e9ba9641a 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -334,6 +334,8 @@ static int try_parent_shorthands(const char *arg)\n \t}\n \n \tcommit = lookup_commit_reference(&oid);\n+\tif (!commit)\n+\t\treturn 1;\n \tif (exclude_parent &&\n \t exclude_parent > commit_list_count(commit->parents)) {\n \t\t*dotdot = '^';\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex 7b1b2dbdf2..f91cc417bd 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -214,7 +214,7 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '\n \ttest_must_fail git rev-list merge^-1x\n '\n \n-test_expect_failure 'rev-parse $garbage^@ should not segfault' '\n+test_expect_success 'rev-parse $garbage^@ should not segfault' '\n \tgit rev-parse ffffffffffffffffffffffffffffffffffffffff^@\n '\n \n-- \n2.17.0.1025.g36b5c64692\n\n"},{"id":"348390","messageId":"20180523220915.GB32171@sigill.intra.peff.net","threadId":"48557","inReplyTo":"20180523204613.11333-2-newren@gmail.com","subject":"Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-23T22:09:15Z","receivedAt":"2018-05-23T22:09:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 23, 2018 at 01:46:13PM -0700, Elijah Newren wrote:\n\n> In commit 2122f8b963d4 (\"rev-parse: Add support for the ^! and ^@ syntax\",\n> 2008-07-26), try_parent_shorthands() was introduced to parse the special\n> ^! and ^@ syntax. However, it did not check the commit returned from\n> lookup_commit_reference() before proceeding to use it. If it is NULL,\n> bail early and notify the caller that this cannot be a valid revision\n> range.\n\nYep, this is definitely the right track. But...\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 55c0b90441..4e9ba9641a 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -334,6 +334,8 @@ static int try_parent_shorthands(const char *arg)\n> \t}\n> \n> \tcommit = lookup_commit_reference(&oid);\n> +\tif (!commit)\n> +\t\treturn 1;\n> \tif (exclude_parent &&\n> \t exclude_parent > commit_list_count(commit->parents)) {\n> \t\t*dotdot = '^';\n\n...I don't think this is quite right. I see two issues:\n\n 1. We need to restore \"*dotdot\" like the other exit code-paths do.\n\n 2. I think a return of 1 means \"yes, I handled this\". We want to\n return 0 so that the bogus name eventually triggers an error.\n\nI also wondered if we need to print an error message, but since we are\nusing the non-gentle form of lookup_commit_reference(), it will complain\nfor us (and then the caller will issue some errors as well).\n\nIt might make sense to just lump this into the get_oid check above.\nE.g., something like:\n\n if (get_oid_committish(arg, &oid) ||\n !(commit = lookup_commit_reference(&oid))) {\n *dotdot = '^';\n\t return 0;\n }\n\nthough I am fine with it either way.\n\n> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\n> index 7b1b2dbdf2..f91cc417bd 100755\n> --- a/t/t6101-rev-parse-parents.sh\n> +++ b/t/t6101-rev-parse-parents.sh\n> @@ -214,7 +214,7 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '\n> \ttest_must_fail git rev-list merge^-1x\n> '\n> \n> -test_expect_failure 'rev-parse $garbage^@ should not segfault' '\n> +test_expect_success 'rev-parse $garbage^@ should not segfault' '\n> \tgit rev-parse ffffffffffffffffffffffffffffffffffffffff^@\n> '\n\nOnce we flip the return value as above, I think this needs to be\ntest_must_fail, which matches how I'd expect it to behave.\n\nThis code (sadly) duplicates the functionality in revision.c. I checked\nthere to see if it has the same problem, but it's fine.\n\nUnfortunately I think rev-parse has one other instance, though:\n\n bogus=ffffffffffffffffffffffffffffffffffffffff\n\n # this is ok; we just normalize to \"$bogus ^$bogus\" without looking at\n # the object, which is OK\n git rev-parse $bogus..$bogus\n\n # this segfaults, because we try to feed NULL to get_merge_bases()\n git rev-parse $bogus...$bogus\n\nWe should probably fix that at the same time.\n\n-Peff\n"},{"id":"348391","messageId":"20180523221249.GC32171@sigill.intra.peff.net","threadId":"48557","inReplyTo":"20180523204613.11333-1-newren@gmail.com","subject":"Re: [PATCH 1/2] t6101: add a test for rev-parse $garbage^@","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-23T22:12:49Z","receivedAt":"2018-05-23T22:12:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 23, 2018 at 01:46:12PM -0700, Elijah Newren wrote:\n\n> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\n> index 8c617981a3..7b1b2dbdf2 100755\n> --- a/t/t6101-rev-parse-parents.sh\n> +++ b/t/t6101-rev-parse-parents.sh\n> @@ -214,4 +214,8 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '\n> \ttest_must_fail git rev-list merge^-1x\n> '\n> \n> +test_expect_failure 'rev-parse $garbage^@ should not segfault' '\n> +\tgit rev-parse ffffffffffffffffffffffffffffffffffffffff^@\n> +'\n\nTwo small nits. :)\n\nIt may just be me, but for a trivial test+fix like this, I'd rather see\nthem in the same commit (both for reviewing, and when I'm digging in the\nhistory later).\n\nThe second nit is that we may want to use something a little more\nsymbolic and easier to read here. Thirty-nine f's behaves quite\ndifferently than forty. And eventually we'd like to move away from\nhaving hard-coded commit ids anyway (this is obviously a fake one, but\nthe length may end up changing).\n\nPerhaps \"git rev-parse $EMPTY_TREE^@\", which triggers the same bug?\n\n-Peff\n"},{"id":"348392","messageId":"20180523221901.GV26695@zaya.teonanacatl.net","threadId":"48557","inReplyTo":"20180523204613.11333-2-newren@gmail.com","subject":"Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2018-05-23T22:19:02Z","receivedAt":"2018-05-23T22:19:08Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"Elijah Newren wrote:\n> In commit 2122f8b963d4 (\"rev-parse: Add support for the ^! and ^@ syntax\",\n> 2008-07-26), try_parent_shorthands() was introduced to parse the special\n> ^! and ^@ syntax. However, it did not check the commit returned from\n> lookup_commit_reference() before proceeding to use it. If it is NULL,\n> bail early and notify the caller that this cannot be a valid revision\n> range.\n\nThanks. This fixes the segfault. While I was testing this,\nI wondered if the following cases should differ:\n\n# f*40\n$ ./git-rev-parse ffffffffffffffffffffffffffffffffffffffff^@ ; echo $?\n0\n\n# f*39\n$ ./git-rev-parse fffffffffffffffffffffffffffffffffffffff^@ ; echo $?\nfffffffffffffffffffffffffffffffffffffff^@\nfatal: ambiguous argument 'fffffffffffffffffffffffffffffffffffffff^@': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n128\n\nLooking a little further, this is deeper than the rev-parse\nhandling. The difference in how these invalid refs are\nhandled appears in 'git show' as well. With 'git show' a\n(different) fatal error is returned in both cases.\n\n# f*40\n$ git show ffffffffffffffffffffffffffffffffffffffff\nfatal: bad object ffffffffffffffffffffffffffffffffffffffff\n\n# 39*f\n$ git show fffffffffffffffffffffffffffffffffffffff\nfatal: ambiguous argument 'fffffffffffffffffffffffffffffffffffffff': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n\nShould rev-parse return an error as well, rather than\nsilenty succeeding?\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nI refuse to spend my life worrying about what I eat. There is no\npleasure worth foregoing just for an extra three years in the\ngeriatric ward.\n -- John Mortimer\n\n"},{"id":"348395","messageId":"20180523222358.GW26695@zaya.teonanacatl.net","threadId":"48557","inReplyTo":"20180523221901.GV26695@zaya.teonanacatl.net","subject":"Re: [PATCH 2/2] rev-parse: verify that commit looked up is not NULL","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2018-05-23T22:23:58Z","receivedAt":"2018-05-23T22:24:07Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"I wrote:\n> Thanks. This fixes the segfault. While I was testing this,\n> I wondered if the following cases should differ:\n\nNevermind me. Jeff beat me to a reply and included much\nmore useful details about why this occurs and suggestions\nfor fixing it. :)\n\n> # f*40\n> $ ./git-rev-parse ffffffffffffffffffffffffffffffffffffffff^@ ; echo $?\n> 0\n> \n> # f*39\n> $ ./git-rev-parse fffffffffffffffffffffffffffffffffffffff^@ ; echo $?\n> fffffffffffffffffffffffffffffffffffffff^@\n> fatal: ambiguous argument 'fffffffffffffffffffffffffffffffffffffff^@': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> 128\n> \n> Looking a little further, this is deeper than the rev-parse\n> handling. The difference in how these invalid refs are\n> handled appears in 'git show' as well. With 'git show' a\n> (different) fatal error is returned in both cases.\n> \n> # f*40\n> $ git show ffffffffffffffffffffffffffffffffffffffff\n> fatal: bad object ffffffffffffffffffffffffffffffffffffffff\n> \n> # 39*f\n> $ git show fffffffffffffffffffffffffffffffffffffff\n> fatal: ambiguous argument 'fffffffffffffffffffffffffffffffffffffff': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions, like this:\n> 'git [...] -- [...]'\n> \n> Should rev-parse return an error as well, rather than\n> silenty succeeding?\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nHow can I tell that the past isn't a fiction designed to account for\nthe discrepancy between my immediate physical sensation and my state\nof mind?\n -- Douglas Adams\n\n"},{"id":"348405","messageId":"20180524062733.5412-1-newren@gmail.com","threadId":"48557","inReplyTo":"20180523220915.GB32171@sigill.intra.peff.net","subject":"[PATCH v2] rev-parse: check lookup'ed commit references for NULL","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-24T06:27:33Z","receivedAt":"2018-05-24T06:27:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Commits 2122f8b963d4 (\"rev-parse: Add support for the ^! and ^@ syntax\",\n2008-07-26) and 3dd4e7320d (\"Teach rev-parse the ... syntax.\", 2006-07-04)\ntaught rev-parse new syntax, and used lookup_commit_reference() as part of\ntheir logic. Neither usage checked the returned commit to see if it was\nnon-NULL before using it. Check for NULL and ensure an appropriate error\nis reported to the user.\n\nReported by Florian Weimer and Todd Zullinger.\n\nHelped-by: Jeff King \nSigned-off-by: Elijah Newren \n---\n\nI would have used a Reported-by tag for Florian and Todd, but looking at\nthe bugzilla.redhat.com bug report doesn't show me Florian's email\naddress. I grepped through git logs and found two associated with that\nname, but didn't know if they were still accurate, or were a different\nFlorian. So I just went with the sentence instead.\n\n builtin/rev-parse.c | 8 ++++++--\n t/t6101-rev-parse-parents.sh | 8 ++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex a1e680b5e9..a0a0ace38d 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -282,6 +282,10 @@ static int try_difference(const char *arg)\n \t\t\tstruct commit *a, *b;\n \t\t\ta = lookup_commit_reference(&start_oid);\n \t\t\tb = lookup_commit_reference(&end_oid);\n+\t\t\tif (!a || !b) {\n+\t\t\t\t*dotdot = '.';\n+\t\t\t\treturn 0;\n+\t\t\t}\n \t\t\texclude = get_merge_bases(a, b);\n \t\t\twhile (exclude) {\n \t\t\t\tstruct commit *commit = pop_commit(&exclude);\n@@ -328,12 +332,12 @@ static int try_parent_shorthands(const char *arg)\n \t\treturn 0;\n \n \t*dotdot = 0;\n-\tif (get_oid_committish(arg, &oid)) {\n+\tif (get_oid_committish(arg, &oid) ||\n+\t !(commit = lookup_commit_reference(&oid))) {\n \t\t*dotdot = '^';\n \t\treturn 0;\n \t}\n \n-\tcommit = lookup_commit_reference(&oid);\n \tif (exclude_parent &&\n \t exclude_parent > commit_list_count(commit->parents)) {\n \t\t*dotdot = '^';\ndiff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\nindex 8c617981a3..7683e4a114 100755\n--- a/t/t6101-rev-parse-parents.sh\n+++ b/t/t6101-rev-parse-parents.sh\n@@ -214,4 +214,12 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '\n \ttest_must_fail git rev-list merge^-1x\n '\n \n+test_expect_success 'rev-parse $garbage^@ does not segfault' '\n+\ttest_must_fail git rev-parse $EMPTY_TREE^@\n+'\n+\n+test_expect_success 'rev-parse $garbage...$garbage does not segfault' '\n+\ttest_must_fail git rev-parse $EMPTY_TREE...$EMPTY_BLOB\n+'\n+\n test_done\n-- \n2.17.0.1.gda85003413\n\n"},{"id":"348429","messageId":"20180524140454.GC26695@zaya.teonanacatl.net","threadId":"48557","inReplyTo":"20180524062733.5412-1-newren@gmail.com","subject":"Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL","fromName":"Todd Zullinger","fromEmail":"tmz@pobox.com","sentAt":"2018-05-24T14:04:54Z","receivedAt":"2018-05-24T14:05:03Z","isPatch":true,"sender":{"key":"tmz@pobox.com","avatar":"https://avatars.githubusercontent.com/u/806319?v=4"},"body":"[Added Florian to Cc]\n\nElijah Newren wrote:\n> Commits 2122f8b963d4 (\"rev-parse: Add support for the ^! and ^@ syntax\",\n> 2008-07-26) and 3dd4e7320d (\"Teach rev-parse the ... syntax.\", 2006-07-04)\n> taught rev-parse new syntax, and used lookup_commit_reference() as part of\n> their logic. Neither usage checked the returned commit to see if it was\n> non-NULL before using it. Check for NULL and ensure an appropriate error\n> is reported to the user.\n> \n> Reported by Florian Weimer and Todd Zullinger.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Elijah Newren \n> ---\n\nThe output is now much more consistent with other invalid\ninput. The only (minor) difference I noticed was when using\nthe fff...fff form. With exactly 40 chars, rev-parse prints\nboth refs separately and then the full input string before\nthe \"fatal:\" error. I doubt it's terribly important.\n\n# exactly 40 chars\n$ ./git-rev-parse ffffffffffffffffffffffffffffffffffffffff...ffffffffffffffffffffffffffffffffffffffff\nffffffffffffffffffffffffffffffffffffffff\nffffffffffffffffffffffffffffffffffffffff\nffffffffffffffffffffffffffffffffffffffff...ffffffffffffffffffffffffffffffffffffffff\nfatal: ambiguous argument 'ffffffffffffffffffffffffffffffffffffffff...ffffffffffffffffffffffffffffffffffffffff': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n\n# not 40 chars\n$ ./git-rev-parse fffffffffffffffffffffffffffffffffffffff...fffffffffffffffffffffffffffffffffffffff\nfffffffffffffffffffffffffffffffffffffff...fffffffffffffffffffffffffffffffffffffff\nfatal: ambiguous argument 'fffffffffffffffffffffffffffffffffffffff...fffffffffffffffffffffffffffffffffffffff': unknown revision or path not in the working tree.\nUse '--' to separate paths from revisions, like this:\n'git [...] -- [...]'\n\n> I would have used a Reported-by tag for Florian and Todd, but looking at\n> the bugzilla.redhat.com bug report doesn't show me Florian's email\n> address. I grepped through git logs and found two associated with that\n> name, but didn't know if they were still accurate, or were a different\n> Florian. So I just went with the sentence instead.\n\nI added Florian to Cc, in case he wants to provide a\npreferred address. (The Red Hat Bugzilla only shows\nemail addresses if you're logged in.)\n\nThanks Elijah and Peff.\n\n> builtin/rev-parse.c | 8 ++++++--\n> t/t6101-rev-parse-parents.sh | 8 ++++++++\n> 2 files changed, 14 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index a1e680b5e9..a0a0ace38d 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -282,6 +282,10 @@ static int try_difference(const char *arg)\n> \t\t\tstruct commit *a, *b;\n> \t\t\ta = lookup_commit_reference(&start_oid);\n> \t\t\tb = lookup_commit_reference(&end_oid);\n> +\t\t\tif (!a || !b) {\n> +\t\t\t\t*dotdot = '.';\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> \t\t\texclude = get_merge_bases(a, b);\n> \t\t\twhile (exclude) {\n> \t\t\t\tstruct commit *commit = pop_commit(&exclude);\n> @@ -328,12 +332,12 @@ static int try_parent_shorthands(const char *arg)\n> \t\treturn 0;\n> \n> \t*dotdot = 0;\n> -\tif (get_oid_committish(arg, &oid)) {\n> +\tif (get_oid_committish(arg, &oid) ||\n> +\t !(commit = lookup_commit_reference(&oid))) {\n> \t\t*dotdot = '^';\n> \t\treturn 0;\n> \t}\n> \n> -\tcommit = lookup_commit_reference(&oid);\n> \tif (exclude_parent &&\n> \t exclude_parent > commit_list_count(commit->parents)) {\n> \t\t*dotdot = '^';\n> diff --git a/t/t6101-rev-parse-parents.sh b/t/t6101-rev-parse-parents.sh\n> index 8c617981a3..7683e4a114 100755\n> --- a/t/t6101-rev-parse-parents.sh\n> +++ b/t/t6101-rev-parse-parents.sh\n> @@ -214,4 +214,12 @@ test_expect_success 'rev-list merge^-1x (garbage after ^-1)' '\n> \ttest_must_fail git rev-list merge^-1x\n> '\n> \n> +test_expect_success 'rev-parse $garbage^@ does not segfault' '\n> +\ttest_must_fail git rev-parse $EMPTY_TREE^@\n> +'\n> +\n> +test_expect_success 'rev-parse $garbage...$garbage does not segfault' '\n> +\ttest_must_fail git rev-parse $EMPTY_TREE...$EMPTY_BLOB\n> +'\n> +\n> test_done\n\n-- \nTodd\n~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\nIf the triangles were to make a God they would give him three sides.\n -- Montesquieu\n\n"},{"id":"348434","messageId":"291c385c-c1e5-8c26-fa10-a59b342751ff@redhat.com","threadId":"48557","inReplyTo":"20180524140454.GC26695@zaya.teonanacatl.net","subject":"Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL","fromName":"Florian Weimer","fromEmail":"fweimer@redhat.com","sentAt":"2018-05-24T15:11:36Z","receivedAt":"2018-05-24T15:11:42Z","isPatch":true,"sender":{"key":"fweimer@redhat.com","avatar":null},"body":"On 05/24/2018 04:04 PM, Todd Zullinger wrote:\n> I added Florian to Cc, in case he wants to provide a\n> preferred address.\n\nSorry, using this address is fine.\n\nThanks,\nFlorian\n"},{"id":"348462","messageId":"20180524170600.GB14876@sigill.intra.peff.net","threadId":"48557","inReplyTo":"20180524062733.5412-1-newren@gmail.com","subject":"Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-24T17:06:00Z","receivedAt":"2018-05-24T17:06:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 23, 2018 at 11:27:33PM -0700, Elijah Newren wrote:\n\n> Commits 2122f8b963d4 (\"rev-parse: Add support for the ^! and ^@ syntax\",\n> 2008-07-26) and 3dd4e7320d (\"Teach rev-parse the ... syntax.\", 2006-07-04)\n> taught rev-parse new syntax, and used lookup_commit_reference() as part of\n> their logic. Neither usage checked the returned commit to see if it was\n> non-NULL before using it. Check for NULL and ensure an appropriate error\n> is reported to the user.\n> \n> Reported by Florian Weimer and Todd Zullinger.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Elijah Newren \n\nThis version looks good to me. Thanks for taking care of this!\n\n-Peff\n"},{"id":"348503","messageId":"xmqq36ygr19n.fsf@gitster-ct.c.googlers.com","threadId":"48557","inReplyTo":"20180524062733.5412-1-newren@gmail.com","subject":"Re: [PATCH v2] rev-parse: check lookup'ed commit references for NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-25T01:07:32Z","receivedAt":"2018-05-25T01:07:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I would have used a Reported-by tag for Florian and Todd, but looking at\n> the bugzilla.redhat.com bug report doesn't show me Florian's email\n> address. I grepped through git logs and found two associated with that\n> name, but didn't know if they were still accurate, or were a different\n> Florian. So I just went with the sentence instead.\n\nOr write names after reported-by without any address? There is no\nlaw that says that a trailer's contents must be proper e-mail\naddresses. People are already known to put garbage on Cc:, for\nexample.\n\n> builtin/rev-parse.c | 8 ++++++--\n> t/t6101-rev-parse-parents.sh | 8 ++++++++\n> 2 files changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index a1e680b5e9..a0a0ace38d 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -282,6 +282,10 @@ static int try_difference(const char *arg)\n> \t\t\tstruct commit *a, *b;\n> \t\t\ta = lookup_commit_reference(&start_oid);\n> \t\t\tb = lookup_commit_reference(&end_oid);\n> +\t\t\tif (!a || !b) {\n> +\t\t\t\t*dotdot = '.';\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n\nWe thought A..B or X...Y were a commit range, but it turns out that\nit is not the case, since at least one end is not a committish. We\nsimply restore the original and tell \"No, this is not a range, try\nto parse it as something else\" to the caller by returning 0.\n\nMakes sense.\n\n> @@ -328,12 +332,12 @@ static int try_parent_shorthands(const char *arg)\n> \t\treturn 0;\n> \n> \t*dotdot = 0;\n> -\tif (get_oid_committish(arg, &oid)) {\n> +\tif (get_oid_committish(arg, &oid) ||\n> +\t !(commit = lookup_commit_reference(&oid))) {\n> \t\t*dotdot = '^';\n> \t\treturn 0;\n> \t}\n> \n> -\tcommit = lookup_commit_reference(&oid);\n\nOK, the logic flows the same way for things like foo^@ here, which\nmakes sense.\n\nLooks good. Thanks.\n\n"}]}