{"thread":{"id":"58390","subject":"[BUG] git crashes on simple rev-parse incantation","startedAt":"2022-09-01T21:16:03Z","lastAt":"2022-09-02T21:29:15Z","messageCount":14,"participants":["Ingy dot Net","Øystein Walle","Eric Sunshine","SZEDER Gábor","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"462493","messageId":"CAHJtQJ4uJc2_upHvc-SWVpA3OX2Lpu-XspswGTTDLgXWjG-Gew@mail.gmail.com","threadId":"58390","inReplyTo":null,"subject":"[BUG] git crashes on simple rev-parse incantation","fromName":"Ingy dot Net","fromEmail":"ingy@ingy.net","sentAt":"2022-09-01T21:15:46Z","receivedAt":"2022-09-01T21:16:03Z","isPatch":false,"sender":{"key":"ingy@ingy.net","avatar":null},"body":"$ git --version\ngit version 2.34.1\n$ uname -a\nLinux zed 5.15.0-48-generic #54-Ubuntu SMP Fri Aug 26 13:26:29 UTC\n2022 x86_64 x86_64 x86_64 GNU/Linux\n\nI get:\n\n$ git rev-parse --parseopt -- <<<$'x\\n--\\n=, x\\n'\nfatal: Out of memory, malloc failed (tried to allocate\n18446744073709551615 bytes)\n\n----\nHere's a less cryptic that fails the same way:\n\nOPTS_SPEC=\"\\\nsome-command [<options>] <args>...\n\nsome-command does foo and bar!\n--\nh,help    show the help\n=,equal   nooooooooooooo!\n\"\n\necho \"$OPTS_SPEC\" |\n  git rev-parse --parseopt --\n\n----\n'=' as a short form in the opts spec seems to be the culprit.\n\nI was trying to see what short options could be parsed like '-9',\n'-_', '-+', etc.\n\nIn addition to '=', '!' and '*' also cause crashes.\nI tested all the other ascii punctuation and didn't see crashes.\n"},{"id":"462510","messageId":"20220902042857.15767-1-oystwa@gmail.com","threadId":"58390","inReplyTo":"CAHJtQJ4uJc2_upHvc-SWVpA3OX2Lpu-XspswGTTDLgXWjG-Gew@mail.gmail.com","subject":"Re: [BUG] git crashes on simple rev-parse incantation","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2022-09-02T04:28:57Z","receivedAt":"2022-09-02T04:29:07Z","isPatch":false,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"FWIW, I can reproduce on master and next.\n\nØsse\n"},{"id":"462511","messageId":"20220902050621.94381-1-oystwa@gmail.com","threadId":"58390","inReplyTo":"CAHJtQJ4uJc2_upHvc-SWVpA3OX2Lpu-XspswGTTDLgXWjG-Gew@mail.gmail.com","subject":"[PATCH] rev-parse: Detect missing opt-spec","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2022-09-02T05:06:21Z","receivedAt":"2022-09-02T05:06:33Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"If a line in parseopts's input starts with one of the flag characters it\nis erroneously parsed as a opt-spec where the short name of the option\nis the flag character itself and the long name is after the end of the\nstring. This makes Git want to allocate SIZE_MAX bytes of memory at this\nline:\n\n    o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n\nSince s and sb.buf are equal the second argument is -2 (except unsigned)\nand xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n\nAvoid this by checking whether a flag character was found in the zeroth\nposition.\n\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\n builtin/rev-parse.c           | 3 +++\n t/t1502-rev-parse-parseopt.sh | 9 +++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b259d8990a..04958cf9a9 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\tif (!s)\n \t\t\ts = help;\n \n+\t\tif (s == sb.buf)\n+\t\t\tdie(_(\"Missing opt-spec before option flags\"));\n+\n \t\tif (s - sb.buf == 1) /* short option only */\n \t\t\to->short_name = *sb.buf;\n \t\telse if (sb.buf[1] != ',') /* long option only */\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 284fe18e72..15bc240027 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'test --parseopt invalid opt-spec' '\n+\ttest_write_lines x -- \"=, x\" >spec &&\n+\techo \"fatal: Missing opt-spec before option flags\" >expect &&\n+\ttest_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n \tsed -e \"s/^|//\" >spec <<-\\EOF &&\n \t|cmd [--some-option]\n@@ -337,3 +344,5 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n '\n \n test_done\n+\n+test_done\n-- \n2.34.1\n\n"},{"id":"462512","messageId":"CAPig+cTAN1F1D=DxZF9jbUiTtc4UPnx0hZLLaVFKEecAa-gMsg@mail.gmail.com","threadId":"58390","inReplyTo":"20220902050621.94381-1-oystwa@gmail.com","subject":"Re: [PATCH] rev-parse: Detect missing opt-spec","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-02T05:46:00Z","receivedAt":"2022-09-02T05:46:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 2, 2022 at 1:10 AM Øystein Walle <oystwa@gmail.com> wrote:\n> If a line in parseopts's input starts with one of the flag characters it\n> is erroneously parsed as a opt-spec where the short name of the option\n> is the flag character itself and the long name is after the end of the\n> string. This makes Git want to allocate SIZE_MAX bytes of memory at this\n> line:\n>\n>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n>\n> Since s and sb.buf are equal the second argument is -2 (except unsigned)\n> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n>\n> Avoid this by checking whether a flag character was found in the zeroth\n> position.\n>\n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n\nPerhaps add a:\n\n    Reported-by: Ingy dot Net <ingy@ingy.net>\n\ntrailer?\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n> +               if (s == sb.buf)\n> +                       die(_(\"Missing opt-spec before option flags\"));\n\nThere is a bit of a mix in this file already, but these days, we tend\nto start error messages with lowercase:\n\n    die(_(\"missing opt-spec before option flags\"));\n\n> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n> @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n> +test_expect_success 'test --parseopt invalid opt-spec' '\n> +       test_write_lines x -- \"=, x\" >spec &&\n> +       echo \"fatal: Missing opt-spec before option flags\" >expect &&\n> +       test_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&\n> +       test_cmp expect actual\n> +'\n> +\n> @@ -337,3 +344,5 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n>\n>  test_done\n> +\n> +test_done\n\nUm? Debugging leftover?\n"},{"id":"462513","messageId":"20220902063958.2516-1-oystwa@gmail.com","threadId":"58390","inReplyTo":"CAPig+cTAN1F1D=DxZF9jbUiTtc4UPnx0hZLLaVFKEecAa-gMsg@mail.gmail.com","subject":"[PATCH v2] rev-parse: Detect missing opt-spec","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2022-09-02T06:39:58Z","receivedAt":"2022-09-02T06:40:16Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"If a line in parseopts's input starts with one of the flag characters it\nis erroneously parsed as a opt-spec where the short name of the option\nis the flag character itself and the long name is after the end of the\nstring. This makes Git want to allocate SIZE_MAX bytes of memory at this\nline:\n\n    o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n\nSince s and sb.buf are equal the second argument is -2 (except unsigned)\nand xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n\nAvoid this by checking whether a flag character was found in the zeroth\nposition.\n\nReported-by: Ingy dot Net <ingy@ingy.net>\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\n\nThanks for the review, Eric (should I then add a Reviewed-by trailer?).\nFixed the casing, added the suggested trailer, and remove the\nsuperfluous test_done which indeed was a leftover. \n\n builtin/rev-parse.c           | 3 +++\n t/t1502-rev-parse-parseopt.sh | 7 +++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b259d8990a..85c271acd7 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\tif (!s)\n \t\t\ts = help;\n \n+\t\tif (s == sb.buf)\n+\t\t\tdie(_(\"missing opt-spec before option flags\"));\n+\n \t\tif (s - sb.buf == 1) /* short option only */\n \t\t\to->short_name = *sb.buf;\n \t\telse if (sb.buf[1] != ',') /* long option only */\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 284fe18e72..75f576249c 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'test --parseopt invalid opt-spec' '\n+\ttest_write_lines x -- \"=, x\" >spec &&\n+\techo \"fatal: Missing opt-spec before option flags\" >expect &&\n+\ttest_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n \tsed -e \"s/^|//\" >spec <<-\\EOF &&\n \t|cmd [--some-option]\n-- \n2.34.1\n\n"},{"id":"462514","messageId":"20220902064727.GA3606@szeder.dev","threadId":"58390","inReplyTo":"20220902050621.94381-1-oystwa@gmail.com","subject":"Re: [PATCH] rev-parse: Detect missing opt-spec","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-02T06:47:27Z","receivedAt":"2022-09-02T06:47:36Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 02, 2022 at 07:06:21AM +0200, Øystein Walle wrote:\n> If a line in parseopts's input starts with one of the flag characters it\n> is erroneously parsed as a opt-spec where the short name of the option\n> is the flag character itself and the long name is after the end of the\n> string. This makes Git want to allocate SIZE_MAX bytes of memory at this\n> line:\n> \n>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> \n> Since s and sb.buf are equal the second argument is -2 (except unsigned)\n> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n\nI suspect (but didn't actually check) that this bug was added in\n2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,\n2015-07-14).\n\n> Avoid this by checking whether a flag character was found in the zeroth\n> position.\n> \n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n> ---\n>  builtin/rev-parse.c           | 3 +++\n>  t/t1502-rev-parse-parseopt.sh | 9 +++++++++\n>  2 files changed, 12 insertions(+)\n> \n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index b259d8990a..04958cf9a9 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n>  \t\tif (!s)\n>  \t\t\ts = help;\n>  \n> +\t\tif (s == sb.buf)\n> +\t\t\tdie(_(\"Missing opt-spec before option flags\"));\n> +\n>  \t\tif (s - sb.buf == 1) /* short option only */\n>  \t\t\to->short_name = *sb.buf;\n>  \t\telse if (sb.buf[1] != ',') /* long option only */\n> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n> index 284fe18e72..15bc240027 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'test --parseopt invalid opt-spec' '\n> +\ttest_write_lines x -- \"=, x\" >spec &&\n> +\techo \"fatal: Missing opt-spec before option flags\" >expect &&\n> +\ttest_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&\n\nWhen checking an error message please don't look for it on standard\noutput; i.e. the redirection at the end should be '2>actual', or\nperheps even better '2>err'.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n>  \tsed -e \"s/^|//\" >spec <<-\\EOF &&\n>  \t|cmd [--some-option]\n> @@ -337,3 +344,5 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l\n>  '\n>  \n>  test_done\n> +\n> +test_done\n> -- \n> 2.34.1\n> \n"},{"id":"462515","messageId":"CAPig+cT0dWW9XCEsjdtUGV3as=+jftp13PVREDW=FPM-Tkgozg@mail.gmail.com","threadId":"58390","inReplyTo":"20220902063958.2516-1-oystwa@gmail.com","subject":"Re: [PATCH v2] rev-parse: Detect missing opt-spec","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-09-02T07:15:15Z","receivedAt":"2022-09-02T07:15:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 2, 2022 at 2:40 AM Øystein Walle <oystwa@gmail.com> wrote:\n> If a line in parseopts's input starts with one of the flag characters it\n> is erroneously parsed as a opt-spec where the short name of the option\n> is the flag character itself and the long name is after the end of the\n> string. This makes Git want to allocate SIZE_MAX bytes of memory at this\n> line:\n>\n>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n>\n> Since s and sb.buf are equal the second argument is -2 (except unsigned)\n> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n>\n> Avoid this by checking whether a flag character was found in the zeroth\n> position.\n>\n> Reported-by: Ingy dot Net <ingy@ingy.net>\n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n> ---\n>\n> Thanks for the review, Eric (should I then add a Reviewed-by trailer?).\n> Fixed the casing, added the suggested trailer, and remove the\n> superfluous test_done which indeed was a leftover.\n\nThanks for addressing my minor comments.\n\nSince I only scanned my eye over the commit message and patch text,\nbut didn't actually dig into the code to verify if the fix was\ncorrect, a Reviewed-by: would be misleading, so let's not add that\ntrailer.\n"},{"id":"462539","messageId":"xmqq5yi5aghf.fsf@gitster.g","threadId":"58390","inReplyTo":"20220902064727.GA3606@szeder.dev","subject":"Re: [PATCH] rev-parse: Detect missing opt-spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-02T16:27:56Z","receivedAt":"2022-09-02T16:28:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> On Fri, Sep 02, 2022 at 07:06:21AM +0200, Øystein Walle wrote:\n>> If a line in parseopts's input starts with one of the flag characters it\n>> is erroneously parsed as a opt-spec where the short name of the option\n>> is the flag character itself and the long name is after the end of the\n>> string. This makes Git want to allocate SIZE_MAX bytes of memory at this\n>> line:\n>> \n>>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n>> \n>> Since s and sb.buf are equal the second argument is -2 (except unsigned)\n>> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n>\n> I suspect (but didn't actually check) that this bug was added in\n> 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,\n> 2015-07-14).\n\nGood thing to add to the proposed log message.  Thanks.\n\nAlso, Øystein \"Detect\" -> \"detect\" on the title (you can see the\nconvention in the output from \"git shortlog --no-merges\").\n\n>>  \t\tif (!s)\n>>  \t\t\ts = help;\n>>  \n>> +\t\tif (s == sb.buf)\n>> +\t\t\tdie(_(\"Missing opt-spec before option flags\"));\n>> +\n\nOK.\n\n>> +test_expect_success 'test --parseopt invalid opt-spec' '\n>> +\ttest_write_lines x -- \"=, x\" >spec &&\n>> +\techo \"fatal: Missing opt-spec before option flags\" >expect &&\n>> +\ttest_must_fail git rev-parse --parseopt -- >out <spec >actual 2>&1 &&\n>\n> When checking an error message please don't look for it on standard\n> output; i.e. the redirection at the end should be '2>actual', or\n> perheps even better '2>err'.\n\nAgain, very good point.\n\nThanks.\n"},{"id":"462544","messageId":"YxI5qBylRhj1jsEv@coredump.intra.peff.net","threadId":"58390","inReplyTo":"20220902050621.94381-1-oystwa@gmail.com","subject":"Re: [PATCH] rev-parse: Detect missing opt-spec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-02T17:13:12Z","receivedAt":"2022-09-02T17:13:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 02, 2022 at 07:06:21AM +0200, Øystein Walle wrote:\n\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index b259d8990a..04958cf9a9 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n>  \t\tif (!s)\n>  \t\t\ts = help;\n>  \n> +\t\tif (s == sb.buf)\n> +\t\t\tdie(_(\"Missing opt-spec before option flags\"));\n> +\n>  \t\tif (s - sb.buf == 1) /* short option only */\n>  \t\t\to->short_name = *sb.buf;\n>  \t\telse if (sb.buf[1] != ',') /* long option only */\n\nI think this is the right thing to do, at least for now. Certainly it\ncatches the bug. It doesn't allow short or long option names to contain\nany flag characters, but that's probably OK in practice.\n\nI think one could make an argument that cmd_parseopt() should do a\nbetter job of parsing left-to-right. The reason it missed this case is\nthat it calls strpbrk(), expecting to jump past the short/long option\nnames, but it jumps less far than expected.\n\nIf the parsing were more left-to-right, like:\n\n  - start with pointer at beginning of sb.buf\n\n  - look for acceptable character for short option, or \",\" for none;\n    advance pointer if found, otherwise bail\n\n  - look for syntactically valid long option name; advance pointer,\n    otherwise bail\n\n  - look for valid flags\n\nthen I think it becomes much easier to reason about what is valid for\neach item. And we _could_ do things like allowing a short-option that is\nalso a flag-character, if we wanted to.\n\nBut IMHO such a refactoring can come later, or not at all. While it\nmight make the code a bit more clear, I don't think it meaningfully\nimproves the behavior. And either way, we should start with a minimal\nand easy-to-verify fix like you have here.\n\n-Peff\n"},{"id":"462546","messageId":"20220902175902.22346-1-oystwa@gmail.com","threadId":"58390","inReplyTo":"xmqq5yi5aghf.fsf@gitster.g","subject":"[PATCH] rev-parse --parseopt: detect missing opt-spec","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2022-09-02T17:59:02Z","receivedAt":"2022-09-02T17:59:19Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"After 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,\n2015-07-14) updated the parser, a line in parseopts's input can start\nwith one of the flag characters and be erroneously parsed as a opt-spec\nwhere the short name of the option is the flag character itself and the\nlong name is after the end of the string. This makes Git want to\nallocate SIZE_MAX bytes of memory at this line:\n\n    o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n\nSince s and sb.buf are equal the second argument is -2 (except unsigned)\nand xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n\nAvoid this by checking whether a flag character was found in the zeroth\nposition.\n\nReported-by: Ingy dot Net <ingy@ingy.net>\nReviewed-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Øystein Walle <oystwa@gmail.com>\n---\n\nHi guys, thanks for the review. I incorporated a reference to the old\ncommit into the message (and took the liberty of adding --parseopt to\nthe subject like it had). I tried to verify that it was in fact this\ncommit, since the code prior to this one had the exact same xmemdupz()\ncall. I wasn't able to build that commit, and reverting it also wasn't\nstraightforward. But I'm fairly confident it's the case since the old\ncall had an if similar to the one added here.\n\nI completely agree with the changes to the test; it makes little sense\nto mix stdout and stderr here.\n\nJeff, I agree that perhaps a larger rewrite would be better. I\npersonally can get easily confused by \"sporadic\" ifs like this one in\nthe middle of a piece of code. At least in this case the message within\ndie() neatly explains what's going on.\n\nØsse\n\n builtin/rev-parse.c           | 3 +++\n t/t1502-rev-parse-parseopt.sh | 7 +++++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b259d8990a..85c271acd7 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \t\tif (!s)\n \t\t\ts = help;\n \n+\t\tif (s == sb.buf)\n+\t\t\tdie(_(\"missing opt-spec before option flags\"));\n+\n \t\tif (s - sb.buf == 1) /* short option only */\n \t\t\to->short_name = *sb.buf;\n \t\telse if (sb.buf[1] != ',') /* long option only */\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 284fe18e72..de1d48f3ba 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'test --parseopt invalid opt-spec' '\n+\ttest_write_lines x -- \"=, x\" >spec &&\n+\techo \"fatal: missing opt-spec before option flags\" >expect &&\n+\ttest_must_fail git rev-parse --parseopt -- >out <spec 2>err &&\n+\ttest_cmp expect err\n+'\n+\n test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n \tsed -e \"s/^|//\" >spec <<-\\EOF &&\n \t|cmd [--some-option]\n-- \n2.34.1\n\n"},{"id":"462547","messageId":"20220902180134.23225-1-oystwa@gmail.com","threadId":"58390","inReplyTo":"20220902175902.22346-1-oystwa@gmail.com","subject":"[PATCH] rev-parse --parseopt: detect missing opt-spec","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2022-09-02T18:01:34Z","receivedAt":"2022-09-02T18:01:46Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Damnit, forgot the v3. Hope that's okay.\n\nØsse\n"},{"id":"462551","messageId":"xmqq1qst8vk9.fsf@gitster.g","threadId":"58390","inReplyTo":"20220902180134.23225-1-oystwa@gmail.com","subject":"Re: [PATCH] rev-parse --parseopt: detect missing opt-spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-02T18:45:10Z","receivedAt":"2022-09-02T18:45:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Øystein Walle <oystwa@gmail.com> writes:\n\n> Damnit, forgot the v3. Hope that's okay.\n>\n> Øsse\n\nSure, and thanks.\n"},{"id":"462558","messageId":"20220902210025.GA1839@szeder.dev","threadId":"58390","inReplyTo":"20220902175902.22346-1-oystwa@gmail.com","subject":"Re: [PATCH] rev-parse --parseopt: detect missing opt-spec","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2022-09-02T21:00:25Z","receivedAt":"2022-09-02T21:00:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Sep 02, 2022 at 07:59:02PM +0200, Øystein Walle wrote:\n> After 2d893dff4c (rev-parse --parseopt: allow [*=?!] in argument hints,\n> 2015-07-14)\n\nOh, no, that's not the first bad commit!  The segfault started\nearlier, with commit 9bab5b6061 (rev-parse --parseopt: option argument\nname hints, 2014-03-22).\n\nBefore that it printed the following for the spec input used in the\nnew test:\n\n  $ ./git rev-parse --parseopt -- <spec\n  set -- --\n\nI'm not sure what the desired behavior should have been back then.\n\nAnyway, since 2d893dff4c the documentation is clear that 'opt-spec'\n\"May not contain any of the `<flags>` characters\", so now erroring out\nis the right thing to do.\n\n> updated the parser, a line in parseopts's input can start\n> with one of the flag characters and be erroneously parsed as a opt-spec\n> where the short name of the option is the flag character itself and the\n> long name is after the end of the string. This makes Git want to\n> allocate SIZE_MAX bytes of memory at this line:\n> \n>     o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);\n> \n> Since s and sb.buf are equal the second argument is -2 (except unsigned)\n> and xmemdupz allocates len + 1 bytes, ie. -1 meaning SIZE_MAX.\n> \n> Avoid this by checking whether a flag character was found in the zeroth\n> position.\n> \n> Reported-by: Ingy dot Net <ingy@ingy.net>\n> Reviewed-by: SZEDER Gábor <szeder.dev@gmail.com>\n> Signed-off-by: Øystein Walle <oystwa@gmail.com>\n> ---\n> \n> Hi guys, thanks for the review. I incorporated a reference to the old\n> commit into the message (and took the liberty of adding --parseopt to\n> the subject like it had). I tried to verify that it was in fact this\n> commit, since the code prior to this one had the exact same xmemdupz()\n> call. I wasn't able to build that commit,\n\nYeah, that's why I didn't check it in the morning...\n\nNO_OPENSSL=1 NO_PERL_MAKEMAKER=1 sometimes helps building older\nversions on modern setups, and, FWIW, Ubuntu 16.04 can build both\ntoday's Git and v1.6.0 (my goto version when I want to check\nhistorical behavior) even with OPENSSL.\n\n>  builtin/rev-parse.c           | 3 +++\n>  t/t1502-rev-parse-parseopt.sh | 7 +++++++\n>  2 files changed, 10 insertions(+)\n> \n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index b259d8990a..85c271acd7 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -479,6 +479,9 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n>  \t\tif (!s)\n>  \t\t\ts = help;\n>  \n> +\t\tif (s == sb.buf)\n> +\t\t\tdie(_(\"missing opt-spec before option flags\"));\n> +\n>  \t\tif (s - sb.buf == 1) /* short option only */\n>  \t\t\to->short_name = *sb.buf;\n>  \t\telse if (sb.buf[1] != ',') /* long option only */\n> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n> index 284fe18e72..de1d48f3ba 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -306,6 +306,13 @@ test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\"\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'test --parseopt invalid opt-spec' '\n> +\ttest_write_lines x -- \"=, x\" >spec &&\n> +\techo \"fatal: missing opt-spec before option flags\" >expect &&\n> +\ttest_must_fail git rev-parse --parseopt -- >out <spec 2>err &&\n> +\ttest_cmp expect err\n> +'\n> +\n>  test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n>  \tsed -e \"s/^|//\" >spec <<-\\EOF &&\n>  \t|cmd [--some-option]\n> -- \n> 2.34.1\n> \n"},{"id":"462560","messageId":"xmqqedwt79eg.fsf@gitster.g","threadId":"58390","inReplyTo":"20220902210025.GA1839@szeder.dev","subject":"Re: [PATCH] rev-parse --parseopt: detect missing opt-spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-02T21:29:11Z","receivedAt":"2022-09-02T21:29:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> NO_OPENSSL=1 NO_PERL_MAKEMAKER=1 sometimes helps building older\n> versions on modern setups, and, FWIW, Ubuntu 16.04 can build both\n> today's Git and v1.6.0 (my goto version when I want to check\n> historical behavior) even with OPENSSL.\n\nThanks for a good piece of advice.\n"}]}