{"thread":{"id":"39042","subject":"[PATCH] grep: correctly initialize help-all option","startedAt":"2015-04-09T13:41:56Z","lastAt":"2015-04-11T00:34:21Z","messageCount":8,"participants":["Patrick Steinhardt","Eric Sunshine","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"259207","messageId":"1428586916-22679-1-git-send-email-ps@pks.im","threadId":"39042","inReplyTo":null,"subject":"[PATCH] grep: correctly initialize help-all option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-04-09T13:41:56Z","receivedAt":"2015-04-09T13:41:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \"help-all\" option is being initialized with a wrong value.\nWhile being semantically wrong this can also cause a gcc\nsegmentation fault on ARMv7 hardfloat platforms with a hardened\ntoolchain. Fix this by initializing with the correct value.\n---\n builtin/grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex abc4400..c0bf005 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n-\t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n+\t\t{ OPTION_CALLBACK, 0, \"help-all\", &opt, NULL, N_(\"show usage\"),\n \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n \t\tOPT_END()\n \t};\n-- \n2.3.5\n"},{"id":"259213","messageId":"CAPig+cQQVO0RqZrAkLxS3v-iPf4LFsDOVz_fPDhofvLvs7tFyA@mail.gmail.com","threadId":"39042","inReplyTo":"1428586916-22679-1-git-send-email-ps@pks.im","subject":"Re: [PATCH] grep: correctly initialize help-all option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-04-09T19:45:29Z","receivedAt":"2015-04-09T19:45:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 9, 2015 at 9:41 AM, Patrick Steinhardt <ps@pks.im> wrote:\n> The \"help-all\" option is being initialized with a wrong value.\n> While being semantically wrong this can also cause a gcc\n> segmentation fault on ARMv7 hardfloat platforms with a hardened\n> toolchain. Fix this by initializing with the correct value.\n\nMissing sign-off.\n\n> ---\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index abc4400..c0bf005 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>                         PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n>                 OPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n>                          N_(\"allow calling of grep(1) (ignored by this build)\")),\n> -               { OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n> +               { OPTION_CALLBACK, 0, \"help-all\", &opt, NULL, N_(\"show usage\"),\n\nThis dates back to 3e230fa1 (grep: use parseopt; 2009-05-07).\n\nbuiltin/show-ref.c just passes NULL for that argument in the same situation.\n\n>                   PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n>                 OPT_END()\n>         };\n> --\n> 2.3.5\n"},{"id":"259214","messageId":"1428609546-3535-1-git-send-email-ps@pks.im","threadId":"39042","inReplyTo":"1428586916-22679-1-git-send-email-ps@pks.im","subject":"[PATCH v2] grep: correctly initialize help-all option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-04-09T19:59:06Z","receivedAt":"2015-04-09T19:59:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The \"help-all\" option is being initialized with a wrong value.\nWhile being semantically wrong this can also cause a segmentation\nfault in gcc on ARMv7 hardfloat platforms with a hardened\ntoolchain. Fix this by initializing with a NULL value.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n\nAdded missing sign-off and changed initialization with &opts to\ninitialization with NULL according to Eric's comment as the value\nis not being used anyway.\n\n builtin/grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex abc4400..d04f440 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n-\t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n+\t\t{ OPTION_CALLBACK, 0, \"help-all\", NULL, NULL, N_(\"show usage\"),\n \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n \t\tOPT_END()\n \t};\n-- \n2.3.5\n"},{"id":"259215","messageId":"5526F535.4020407@web.de","threadId":"39042","inReplyTo":"1428586916-22679-1-git-send-email-ps@pks.im","subject":"Re: [PATCH] grep: correctly initialize help-all option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-04-09T21:55:01Z","receivedAt":"2015-04-09T21:55:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:\n> The \"help-all\" option is being initialized with a wrong value.\n> While being semantically wrong this can also cause a gcc\n> segmentation fault on ARMv7 hardfloat platforms with a hardened\n> toolchain. Fix this by initializing with the correct value.\n\nThanks for your report and patch.  A few comments:\n\nSign-off?  (See \"Sign your work\" in Documentation/SubmittingPatches)\n\n> ---\n>   builtin/grep.c | 2 +-\n>   1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index abc4400..c0bf005 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>   \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n>   \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n>   \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n> -\t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n> +\t\t{ OPTION_CALLBACK, 0, \"help-all\", &opt, NULL, N_(\"show usage\"),\n>   \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n>   \t\tOPT_END()\n>   \t};\n\nhelp_callback() returns -1 immediately, IOW the value pointer is never \nused anyway.  So why does your change make a difference?  *puzzled*\n\nWe could pass NULL instead, as in builtin/show-ref.c, which would make \nit clear that the pointer is just a dummy.\n\nQ: Why does the callback exist in the first place?  A: parse_options() \nhandles -h and --help-all automatically by showing the list of available \noptions.  This is good for most commands for consistency's sake, but bad \nfor commands that want to use -h for something else.  That's why it can \nbe turned off with the flag PARSE_OPT_NO_INTERNAL_HELP, but that flag \ndisables the long option as well.  It's added back by grep and show-ref \nexplicitly, to at least provide --help-all consistently across all commands.\n\nWe could solve this problem centrally by checking for -h and --help-all \nonly after looking through the struct option list supplied to \nparse_options_step() instead of before and getting rid of the then \nunneeded callbacks for --help-all in grep and show-ref.  Are there any \ndownsides to that approach?\n\nRené\n"},{"id":"259219","messageId":"20150410052250.GA372@pks-pc.localdomain","threadId":"39042","inReplyTo":"5526F535.4020407@web.de","subject":"Re: [PATCH] grep: correctly initialize help-all option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-04-10T05:22:50Z","receivedAt":"2015-04-10T05:22:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 09, 2015 at 11:55:01PM +0200, René Scharfe wrote:\n> Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:\n> > The \"help-all\" option is being initialized with a wrong value.\n> > While being semantically wrong this can also cause a gcc\n> > segmentation fault on ARMv7 hardfloat platforms with a hardened\n> > toolchain. Fix this by initializing with the correct value.\n> \n> Thanks for your report and patch.  A few comments:\n> \n> Sign-off?  (See \"Sign your work\" in Documentation/SubmittingPatches)\n\nFixed in v2 of the patch already.\n\n> \n> > ---\n> >   builtin/grep.c | 2 +-\n> >   1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/builtin/grep.c b/builtin/grep.c\n> > index abc4400..c0bf005 100644\n> > --- a/builtin/grep.c\n> > +++ b/builtin/grep.c\n> > @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> >   \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n> >   \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n> >   \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n> > -\t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n> > +\t\t{ OPTION_CALLBACK, 0, \"help-all\", &opt, NULL, N_(\"show usage\"),\n> >   \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n> >   \t\tOPT_END()\n> >   \t};\n> \n> help_callback() returns -1 immediately, IOW the value pointer is never \n> used anyway.  So why does your change make a difference?  *puzzled*\n> \n> We could pass NULL instead, as in builtin/show-ref.c, which would make \n> it clear that the pointer is just a dummy.\n\nChanged in v2, as well.\n\nIn general the change won't make any difference when running the\ncommand. But as said in the commit message it caused gcc (gcc\nversion 4.8.3 (Gentoo Hardened 4.8.3 p1.1, pie-0.5.9), ARMv7 HF)\nto segfault when &options was passed in as value. Even though\nthis is probably an error in gcc we can easily work around it by\ndoing the Right Thing here.\n\n> Q: Why does the callback exist in the first place?  A: parse_options() \n> handles -h and --help-all automatically by showing the list of available \n> options.  This is good for most commands for consistency's sake, but bad \n> for commands that want to use -h for something else.  That's why it can \n> be turned off with the flag PARSE_OPT_NO_INTERNAL_HELP, but that flag \n> disables the long option as well.  It's added back by grep and show-ref \n> explicitly, to at least provide --help-all consistently across all commands.\n> \n> We could solve this problem centrally by checking for -h and --help-all \n> only after looking through the struct option list supplied to \n> parse_options_step() instead of before and getting rid of the then \n> unneeded callbacks for --help-all in grep and show-ref.  Are there any \n> downsides to that approach?\n\nI'll leave that question to the folks more experienced with the\ncode base.\n\n> René\n\nThanks for your feedback.\n\nPatrick\n"},{"id":"259227","messageId":"5527FB95.5010806@web.de","threadId":"39042","inReplyTo":"20150410052250.GA372@pks-pc.localdomain","subject":"Re: [PATCH] grep: correctly initialize help-all option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-04-10T16:34:29Z","receivedAt":"2015-04-10T16:34:29Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.04.2015 um 07:22 schrieb Patrick Steinhardt:\n> On Thu, Apr 09, 2015 at 11:55:01PM +0200, René Scharfe wrote:\n>> Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:\n>>> ---\n>>>    builtin/grep.c | 2 +-\n>>>    1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/builtin/grep.c b/builtin/grep.c\n>>> index abc4400..c0bf005 100644\n>>> --- a/builtin/grep.c\n>>> +++ b/builtin/grep.c\n>>> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>>    \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n>>>    \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n>>>    \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n>>> -\t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n>>> +\t\t{ OPTION_CALLBACK, 0, \"help-all\", &opt, NULL, N_(\"show usage\"),\n>>>    \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n>>>    \t\tOPT_END()\n>>>    \t};\n>>\n>> help_callback() returns -1 immediately, IOW the value pointer is never\n>> used anyway.  So why does your change make a difference?  *puzzled*\n>>\n>> We could pass NULL instead, as in builtin/show-ref.c, which would make\n>> it clear that the pointer is just a dummy.\n>\n> Changed in v2, as well.\n\nThank you.  I should really re-fetch from Gmane before finishing an \ninterrupted reply..\n\n> In general the change won't make any difference when running the\n> command. But as said in the commit message it caused gcc (gcc\n> version 4.8.3 (Gentoo Hardened 4.8.3 p1.1, pie-0.5.9), ARMv7 HF)\n> to segfault when &options was passed in as value. Even though\n> this is probably an error in gcc we can easily work around it by\n> doing the Right Thing here.\n\nOK, so does it crash on this one-liner as well?\n\n\tstruct t {void *p;} s = {&s};\n\nOr on this?\n\n\tvoid *p = &p;\n\nIf yes then the author of the hardening feature might be interested in \nthis fact.\n\nRené\n"},{"id":"259228","messageId":"5527FBD3.4000103@web.de","threadId":"39042","inReplyTo":"1428609546-3535-1-git-send-email-ps@pks.im","subject":"Re: [PATCH v2] grep: correctly initialize help-all option","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-04-10T16:35:31Z","receivedAt":"2015-04-10T16:35:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.04.2015 um 21:59 schrieb Patrick Steinhardt:\n> The \"help-all\" option is being initialized with a wrong value.\n> While being semantically wrong this can also cause a segmentation\n> fault in gcc on ARMv7 hardfloat platforms with a hardened\n> toolchain. Fix this by initializing with a NULL value.\n\nBecause the pointer is not used it can't exactly be \"wrong\".  The \ncallback function could call usage_with_options itself using that \npointer, for example.  I suspect that's what an earlier version did (I \ndon't remember).\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>\n> Added missing sign-off and changed initialization with &opts to\n> initialization with NULL according to Eric's comment as the value\n> is not being used anyway.\n\nThanks, it looks good.\n\n>\n>   builtin/grep.c | 2 +-\n>   1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index abc4400..d04f440 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>   \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n>   \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n>   \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n> -\t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n> +\t\t{ OPTION_CALLBACK, 0, \"help-all\", NULL, NULL, N_(\"show usage\"),\n>   \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n>   \t\tOPT_END()\n>   \t};\n>\n"},{"id":"259232","messageId":"20150411003421.GA2111@pks-mb","threadId":"39042","inReplyTo":"5527FB95.5010806@web.de","subject":"Re: [PATCH] grep: correctly initialize help-all option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2015-04-11T00:34:21Z","receivedAt":"2015-04-11T00:34:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Apr 10, 2015 at 06:34:29PM +0200, René Scharfe wrote:\n> Am 10.04.2015 um 07:22 schrieb Patrick Steinhardt:\n> > On Thu, Apr 09, 2015 at 11:55:01PM +0200, René Scharfe wrote:\n> >> Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:\n> >>> ---\n> >>>    builtin/grep.c | 2 +-\n> >>>    1 file changed, 1 insertion(+), 1 deletion(-)\n> >>>\n> >>> diff --git a/builtin/grep.c b/builtin/grep.c\n> >>> index abc4400..c0bf005 100644\n> >>> --- a/builtin/grep.c\n> >>> +++ b/builtin/grep.c\n> >>> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> >>>    \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },\n> >>>    \t\tOPT_BOOL(0, \"ext-grep\", &external_grep_allowed__ignored,\n> >>>    \t\t\t N_(\"allow calling of grep(1) (ignored by this build)\")),\n> >>> -\t\t{ OPTION_CALLBACK, 0, \"help-all\", &options, NULL, N_(\"show usage\"),\n> >>> +\t\t{ OPTION_CALLBACK, 0, \"help-all\", &opt, NULL, N_(\"show usage\"),\n> >>>    \t\t  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },\n> >>>    \t\tOPT_END()\n> >>>    \t};\n> >>\n> >> help_callback() returns -1 immediately, IOW the value pointer is never\n> >> used anyway.  So why does your change make a difference?  *puzzled*\n> >>\n> >> We could pass NULL instead, as in builtin/show-ref.c, which would make\n> >> it clear that the pointer is just a dummy.\n> >\n> > Changed in v2, as well.\n> \n> Thank you.  I should really re-fetch from Gmane before finishing an \n> interrupted reply..\n> \n> > In general the change won't make any difference when running the\n> > command. But as said in the commit message it caused gcc (gcc\n> > version 4.8.3 (Gentoo Hardened 4.8.3 p1.1, pie-0.5.9), ARMv7 HF)\n> > to segfault when &options was passed in as value. Even though\n> > this is probably an error in gcc we can easily work around it by\n> > doing the Right Thing here.\n> \n> OK, so does it crash on this one-liner as well?\n> \n> \tstruct t {void *p;} s = {&s};\n> \n> Or on this?\n> \n> \tvoid *p = &p;\n> \n> If yes then the author of the hardening feature might be interested in \n> this fact.\n> \n> René\n\nNo, the segfault is not triggered by this. I've also tried to use\nthe exact arguments to gcc that are used when compiling grep.c,\nbut to no avail. I'll maybe try to reproduce this with a minimal\ntestcase next week.\n\nRegards\nPatrick\n"}]}