{"thread":{"id":"56019","subject":"segfault on invalid 'git grep' invocation","startedAt":"2021-06-28T12:17:54Z","lastAt":"2021-06-30T16:12:51Z","messageCount":6,"participants":["Matthew Hughes","René Scharfe","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"428604","messageId":"20210628121748.f3yrc72v4mynknl3@debianXPS.lan","threadId":"56019","inReplyTo":null,"subject":"segfault on invalid 'git grep' invocation","fromName":"Matthew Hughes","fromEmail":"matthewhughes934@gmail.com","sentAt":"2021-06-28T12:17:48Z","receivedAt":"2021-06-28T12:17:54Z","isPatch":false,"sender":{"key":"matthewhughes934@gmail.com","avatar":"https://avatars.githubusercontent.com/u/34972397?v=4"},"body":"Hi all,\n\nIt's my first time writing to this mailing list, so apologies in advance\nfor any mistakes/faux pas.\n\nSummary\n=======\n\nSegfault seen when running:\n\n    $ ./git grep --and -e 'pattern' -- not_a_path\n\nExpected Behaviour\n==================\n\nThe above invocation doesn't appear particularly valid to me, I\naccidentally ran it while editing a command. Not sure if this needs\naddressing, but perhaps the expected behaviour would be an error message\nabout it not being valid?\n\nCompare with, running without '--':\n\n    $ ./git grep --and -e 'pattern' not_a_path\n    fatal: ambiguous argument 'not_a_path': unknown revision or path not in the working tree.\n    Use '--' to separate paths from revisions, like this:\n    'git <command> [<revision>...] -- [<file>...]'\n\nRunning with a valid path:\n\n    $ ./git grep --and -e 'pattern' -- common-main.c\n    fatal: Not a valid grep expression\n\n    $ ./git grep --and -e 'pattern' common-main.c\n    fatal: Not a valid grep expression\n\n\nBackground\n==========\n\ngit version:\ngit version 2.32.0.93.g670b81a890\ncpu: x86_64\nbuilt from commit: 670b81a890388c60b7032a4f5b879f2ece8c4558\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 4.19.0-16-amd64 #1 SMP Debian 4.19.181-1 (2021-03-19) x86_64\ncompiler info: gnuc: 8.3\nlibc info: glibc: 2.28\n$SHELL (typically, interactive shell): /bin/bash\n\nAlso reproduced on git 2.20.1 (packaged version on Debian 10)\n\nThe segfault is raised at free_pattern_expr (x=0x0) at grep.c:825 (on\ncommit 670b81a890388c60b7032a4f5b879f2ece8c4558)\n\nRegards,\nMatt\n"},{"id":"428622","messageId":"98171911-ba39-27f1-d068-4d381bcd4804@web.de","threadId":"56019","inReplyTo":"20210628121748.f3yrc72v4mynknl3@debianXPS.lan","subject":"[PATCH] grep: report missing left operand of --and","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-28T18:58:12Z","receivedAt":"2021-06-28T18:58:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Git grep allows combining two patterns with --and.  It checks and\nreports if the second pattern is missing when compiling the expression.\nA missing first pattern, however, is only reported later at match time.\nThus no error is returned if no matching is done, e.g. because no file\nmatches the also given pathspec.\n\nWhen that happens we get an expression tree with an GREP_NODE_AND node\nand a NULL pointer to the missing left child.  free_pattern_expr()\ntries to dereference it during the cleanup at the end, which result in\na segmentation fault.\n\nFix this by verifying the presence of the left operand at expression\ncompilation time.\n\nReported-by: Matthew Hughes <matthewhughes934@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nWhether the check in match_expr_eval() can now be turned into a BUG is\nleft as an exercise for the reader. ;-)\n\n grep.c          |  2 ++\n t/t7810-grep.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex 8f91af1cb0..7d0ea4e956 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -655,6 +655,8 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n \tstruct grep_expr *x, *y, *z;\n\n \tx = compile_pattern_not(list);\n+\tif (!x)\n+\t\tdie(\"Not a valid grep expression\");\n \tp = *list;\n \tif (p && p->token == GREP_AND) {\n \t\tif (!p->next)\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 5830733f3d..c581239674 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n\n . ./test-lib.sh\n\n+test_invalid_grep_expression() {\n+\tparams=\"$@\" &&\n+\ttest_expect_success \"invalid expression: grep $params\" '\n+\t\ttest_must_fail git grep $params -- nonexisting\n+\t'\n+}\n+\n cat >hello.c <<EOF\n #include <assert.h>\n #include <stdio.h>\n@@ -89,6 +96,9 @@ test_expect_success 'grep should not segfault with a bad input' '\n \ttest_must_fail git grep \"(\"\n '\n\n+test_invalid_grep_expression -e A --and\n+test_invalid_grep_expression --and -e A\n+\n for H in HEAD ''\n do\n \tcase \"$H\" in\n--\n2.32.0\n"},{"id":"428623","messageId":"87bl7phl41.fsf@evledraar.gmail.com","threadId":"56019","inReplyTo":"20210628121748.f3yrc72v4mynknl3@debianXPS.lan","subject":"Re: segfault on invalid 'git grep' invocation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-28T18:53:56Z","receivedAt":"2021-06-28T18:58:45Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 28 2021, Matthew Hughes wrote:\n\n> Hi all,\n>\n> It's my first time writing to this mailing list, so apologies in advance\n> for any mistakes/faux pas.\n\nThanks a lot for the report, and welcome.\n\n> Summary\n> =======\n>\n> Segfault seen when running:\n>\n>     $ ./git grep --and -e 'pattern' -- not_a_path\n>\n> Expected Behaviour\n> ==================\n>\n> The above invocation doesn't appear particularly valid to me, I\n> accidentally ran it while editing a command. Not sure if this needs\n> addressing, but perhaps the expected behaviour would be an error message\n> about it not being valid?\n>\n> Compare with, running without '--':\n>\n>     $ ./git grep --and -e 'pattern' not_a_path\n>     fatal: ambiguous argument 'not_a_path': unknown revision or path not in the working tree.\n>     Use '--' to separate paths from revisions, like this:\n>     'git <command> [<revision>...] -- [<file>...]'\n>\n> Running with a valid path:\n>\n>     $ ./git grep --and -e 'pattern' -- common-main.c\n>     fatal: Not a valid grep expression\n>\n>     $ ./git grep --and -e 'pattern' common-main.c\n>     fatal: Not a valid grep expression\n\nThe exact segfault you've found is found at least back to Git version\n2.0.0, so it's been with us for a while.\n\nThe problem is that you're trying to --and two things, but did not\nprovide two things. We apparently never checked for this, and then\nsegfault in the cleanup, thinking an --and must imply that we have two\nsides.\n\nI haven't looked deeply, but think that the reason it segfaults just\nwith not_a_path is that we then proceed to die() in something that\nshould probably be a BUG() instead as we can't grep in the\nexpected-but-not-there pattern at runtime.\n\nGiving it a pathspec that doesn't exist means we never run the \"real\"\ngrep, but we do run the cleanup, hence the segfault.\n"},{"id":"428759","messageId":"87im1wft42.fsf@evledraar.gmail.com","threadId":"56019","inReplyTo":"98171911-ba39-27f1-d068-4d381bcd4804@web.de","subject":"Re: [PATCH] grep: report missing left operand of --and","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-29T17:52:19Z","receivedAt":"2021-06-29T18:01:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 28 2021, René Scharfe wrote:\n\n> Git grep allows combining two patterns with --and.  It checks and\n> reports if the second pattern is missing when compiling the expression.\n> A missing first pattern, however, is only reported later at match time.\n> Thus no error is returned if no matching is done, e.g. because no file\n> matches the also given pathspec.\n>\n> When that happens we get an expression tree with an GREP_NODE_AND node\n> and a NULL pointer to the missing left child.  free_pattern_expr()\n> tries to dereference it during the cleanup at the end, which result in\n> a segmentation fault.\n>\n> Fix this by verifying the presence of the left operand at expression\n> compilation time.\n>\n> Reported-by: Matthew Hughes <matthewhughes934@gmail.com>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> Whether the check in match_expr_eval() can now be turned into a BUG is\n> left as an exercise for the reader. ;-)\n>\n>  grep.c          |  2 ++\n>  t/t7810-grep.sh | 10 ++++++++++\n>  2 files changed, 12 insertions(+)\n>\n> diff --git a/grep.c b/grep.c\n> index 8f91af1cb0..7d0ea4e956 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -655,6 +655,8 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n>  \tstruct grep_expr *x, *y, *z;\n>\n>  \tx = compile_pattern_not(list);\n> +\tif (!x)\n> +\t\tdie(\"Not a valid grep expression\");\n>  \tp = *list;\n>  \tif (p && p->token == GREP_AND) {\n>  \t\tif (!p->next)\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index 5830733f3d..c581239674 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>\n>  . ./test-lib.sh\n>\n> +test_invalid_grep_expression() {\n> +\tparams=\"$@\" &&\n> +\ttest_expect_success \"invalid expression: grep $params\" '\n> +\t\ttest_must_fail git grep $params -- nonexisting\n> +\t'\n> +}\n> +\n>  cat >hello.c <<EOF\n>  #include <assert.h>\n>  #include <stdio.h>\n> @@ -89,6 +96,9 @@ test_expect_success 'grep should not segfault with a bad input' '\n>  \ttest_must_fail git grep \"(\"\n>  '\n>\n> +test_invalid_grep_expression -e A --and\n> +test_invalid_grep_expression --and -e A\n> +\n>  for H in HEAD ''\n>  do\n>  \tcase \"$H\" in\n\nThis seems like an incomplete fix, for the exact same thing with --or we\nsilently return 1, as we would if we exited early in free_pattern_expr\non !x, which aside from the segfault I think we should probably make a\nhabit in our own free()-like functions.\n\nWhatever we're doing about the --and segfault it seems like we should do\nthe same under --or, no?\n\nYour first test also passes before your fix, it's only the latter that\nsegfaults. The first one emits:\n\n    fatal: --and not followed by pattern expression\n\nSo having that in a leading patch to indicate no behavior was changed\nwould be better.\n\nInstead of the \"Not a valid grep expression\" error let's instead say\nsomething like:\n\n    fatal: --[and|or] must follow a pattern expression\n\nThe error (which I know you just copied from elsewhere) is misleading,\nit's not the pattern that's not valid (as to me it implies), but our own\n--and/--or option usage.\n\nAnd the \"excercise for the reader\" is a bit flippant, do we actually hit\nthat condition now? If not and we're sure we won't now seems like the\ntime to add a BUG() there, and to change the \"Not a valid grep\nexpression\" to \"internal error in --and/--or parsing\" or something.\n\nThanks for the patch!\n"},{"id":"428760","messageId":"7c462dc5-f250-0bc3-5e85-cf394fd8fbb9@web.de","threadId":"56019","inReplyTo":"87im1wft42.fsf@evledraar.gmail.com","subject":"Re: [PATCH] grep: report missing left operand of --and","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-29T18:35:47Z","receivedAt":"2021-06-29T18:35:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.06.21 um 19:52 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Mon, Jun 28 2021, René Scharfe wrote:\n>\n>> Git grep allows combining two patterns with --and.  It checks and\n>> reports if the second pattern is missing when compiling the expression.\n>> A missing first pattern, however, is only reported later at match time.\n>> Thus no error is returned if no matching is done, e.g. because no file\n>> matches the also given pathspec.\n>>\n>> When that happens we get an expression tree with an GREP_NODE_AND node\n>> and a NULL pointer to the missing left child.  free_pattern_expr()\n>> tries to dereference it during the cleanup at the end, which result in\n>> a segmentation fault.\n>>\n>> Fix this by verifying the presence of the left operand at expression\n>> compilation time.\n>>\n>> Reported-by: Matthew Hughes <matthewhughes934@gmail.com>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> Whether the check in match_expr_eval() can now be turned into a BUG is\n>> left as an exercise for the reader. ;-)\n>>\n>>  grep.c          |  2 ++\n>>  t/t7810-grep.sh | 10 ++++++++++\n>>  2 files changed, 12 insertions(+)\n>>\n>> diff --git a/grep.c b/grep.c\n>> index 8f91af1cb0..7d0ea4e956 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -655,6 +655,8 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n>>  \tstruct grep_expr *x, *y, *z;\n>>\n>>  \tx = compile_pattern_not(list);\n>> +\tif (!x)\n>> +\t\tdie(\"Not a valid grep expression\");\n>>  \tp = *list;\n>>  \tif (p && p->token == GREP_AND) {\n>>  \t\tif (!p->next)\n>> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n>> index 5830733f3d..c581239674 100755\n>> --- a/t/t7810-grep.sh\n>> +++ b/t/t7810-grep.sh\n>> @@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>>\n>>  . ./test-lib.sh\n>>\n>> +test_invalid_grep_expression() {\n>> +\tparams=\"$@\" &&\n>> +\ttest_expect_success \"invalid expression: grep $params\" '\n>> +\t\ttest_must_fail git grep $params -- nonexisting\n>> +\t'\n>> +}\n>> +\n>>  cat >hello.c <<EOF\n>>  #include <assert.h>\n>>  #include <stdio.h>\n>> @@ -89,6 +96,9 @@ test_expect_success 'grep should not segfault with a bad input' '\n>>  \ttest_must_fail git grep \"(\"\n>>  '\n>>\n>> +test_invalid_grep_expression -e A --and\n>> +test_invalid_grep_expression --and -e A\n>> +\n>>  for H in HEAD ''\n>>  do\n>>  \tcase \"$H\" in\n>\n> This seems like an incomplete fix, for the exact same thing with --or we\n> silently return 1, as we would if we exited early in free_pattern_expr\n> on !x, which aside from the segfault I think we should probably make a\n> habit in our own free()-like functions.\n>\n> Whatever we're doing about the --and segfault it seems like we should do\n> the same under --or, no?\n\nNo, --or is a special case and needs special handling.  Currently it's\nignored.  If we want to berate the user for using it without expressions\nleft and right then we need to start actively handling it.\n\n> Your first test also passes before your fix, it's only the latter that\n> segfaults. The first one emits:\n>\n>     fatal: --and not followed by pattern expression\n>\n> So having that in a leading patch to indicate no behavior was changed\n> would be better.\n\nTrue, the first test is just nice to have.  I can remove it to reduce\nconfusion.\n\n> Instead of the \"Not a valid grep expression\" error let's instead say\n> something like:\n>\n>     fatal: --[and|or] must follow a pattern expression\n\nGood point.\n\n> The error (which I know you just copied from elsewhere) is misleading,\n> it's not the pattern that's not valid (as to me it implies), but our own\n> --and/--or option usage.\n\nThat's what's meant with extended pattern, I think.\n\n> And the \"excercise for the reader\" is a bit flippant, do we actually hit\n> that condition now? If not and we're sure we won't now seems like the\n> time to add a BUG() there, and to change the \"Not a valid grep\n> expression\" to \"internal error in --and/--or parsing\" or something.\n\nI don't know.\n\nRené\n"},{"id":"428826","messageId":"1eeb34cf-3229-21f8-23ca-ab3c6d4dff2e@web.de","threadId":"56019","inReplyTo":"20210628121748.f3yrc72v4mynknl3@debianXPS.lan","subject":"[PATCH v2] grep: report missing left operand of --and","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-06-30T16:12:43Z","receivedAt":"2021-06-30T16:12:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Git grep allows combining two patterns with --and.  It checks and\nreports if the second pattern is missing when compiling the expression.\nA missing first pattern, however, is only reported later at match time.\nThus no error is returned if no matching is done, e.g. because no file\nmatches the also given pathspec.\n\nWhen that happens we get an expression tree with an GREP_NODE_AND node\nand a NULL pointer to the missing left child.  free_pattern_expr()\ntries to dereference it during the cleanup at the end, which results\nin a segmentation fault.\n\nFix this by verifying the presence of the left operand at expression\ncompilation time.\n\nReported-by: Matthew Hughes <matthewhughes934@gmail.com>\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges since v2:\n- more specific error message,\n- grammar error fix in commit message,\n- no test of already working behavior.\n\n grep.c          | 2 ++\n t/t7810-grep.sh | 9 +++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/grep.c b/grep.c\nindex 8f91af1cb0..424a39591b 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -657,6 +657,8 @@ static struct grep_expr *compile_pattern_and(struct grep_pat **list)\n \tx = compile_pattern_not(list);\n \tp = *list;\n \tif (p && p->token == GREP_AND) {\n+\t\tif (!x)\n+\t\t\tdie(\"--and not preceded by pattern expression\");\n \t\tif (!p->next)\n \t\t\tdie(\"--and not followed by pattern expression\");\n \t\t*list = p->next;\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 5830733f3d..6b6423a07c 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -11,6 +11,13 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n\n . ./test-lib.sh\n\n+test_invalid_grep_expression() {\n+\tparams=\"$@\" &&\n+\ttest_expect_success \"invalid expression: grep $params\" '\n+\t\ttest_must_fail git grep $params -- nonexisting\n+\t'\n+}\n+\n cat >hello.c <<EOF\n #include <assert.h>\n #include <stdio.h>\n@@ -89,6 +96,8 @@ test_expect_success 'grep should not segfault with a bad input' '\n \ttest_must_fail git grep \"(\"\n '\n\n+test_invalid_grep_expression --and -e A\n+\n for H in HEAD ''\n do\n \tcase \"$H\" in\n--\n2.32.0\n"}]}