[PATCH] grep: correctly initialize help-all option

[PATCH] grep: correctly initialize help-all option

[Patrick Steinhardt]

The "help-all" option is being initialized with a wrong value.
While being semantically wrong this can also cause a gcc
segmentation fault on ARMv7 hardfloat platforms with a hardened
toolchain. Fix this by initializing with the correct value.
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index abc4400..c0bf005 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
 		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
 			 N_("allow calling of grep(1) (ignored by this build)")),
-		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
+		{ OPTION_CALLBACK, 0, "help-all", &opt, NULL, N_("show usage"),
 		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
 		OPT_END()
 	};
-- 
2.3.5

Re: [PATCH] grep: correctly initialize help-all option

[Eric Sunshine]

On Thu, Apr 9, 2015 at 9:41 AM, Patrick Steinhardt <ps@pks.im> wrote:
> The "help-all" option is being initialized with a wrong value.
> While being semantically wrong this can also cause a gcc
> segmentation fault on ARMv7 hardfloat platforms with a hardened
> toolchain. Fix this by initializing with the correct value.

Missing sign-off.

> ---
> diff --git a/builtin/grep.c b/builtin/grep.c
> index abc4400..c0bf005 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>                         PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
>                 OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
>                          N_("allow calling of grep(1) (ignored by this build)")),
> -               { OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
> +               { OPTION_CALLBACK, 0, "help-all", &opt, NULL, N_("show usage"),

This dates back to 3e230fa1 (grep: use parseopt; 2009-05-07).

builtin/show-ref.c just passes NULL for that argument in the same situation.

>                   PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>                 OPT_END()
>         };
> --
> 2.3.5

[PATCH v2] grep: correctly initialize help-all option

[Patrick Steinhardt]

The "help-all" option is being initialized with a wrong value.
While being semantically wrong this can also cause a segmentation
fault in gcc on ARMv7 hardfloat platforms with a hardened
toolchain. Fix this by initializing with a NULL value.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

Added missing sign-off and changed initialization with &opts to
initialization with NULL according to Eric's comment as the value
is not being used anyway.

 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index abc4400..d04f440 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
 		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
 			 N_("allow calling of grep(1) (ignored by this build)")),
-		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
+		{ OPTION_CALLBACK, 0, "help-all", NULL, NULL, N_("show usage"),
 		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
 		OPT_END()
 	};
-- 
2.3.5

Re: [PATCH v2] grep: correctly initialize help-all option

[René Scharfe]

Am 09.04.2015 um 21:59 schrieb Patrick Steinhardt:
> The "help-all" option is being initialized with a wrong value.
> While being semantically wrong this can also cause a segmentation
> fault in gcc on ARMv7 hardfloat platforms with a hardened
> toolchain. Fix this by initializing with a NULL value.

Because the pointer is not used it can't exactly be "wrong".  The 
callback function could call usage_with_options itself using that 
pointer, for example.  I suspect that's what an earlier version did (I 
don't remember).

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> Added missing sign-off and changed initialization with &opts to
> initialization with NULL according to Eric's comment as the value
> is not being used anyway.

Thanks, it looks good.

>
>   builtin/grep.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index abc4400..d04f440 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>   			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
>   		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
>   			 N_("allow calling of grep(1) (ignored by this build)")),
> -		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
> +		{ OPTION_CALLBACK, 0, "help-all", NULL, NULL, N_("show usage"),
>   		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>   		OPT_END()
>   	};
>

Re: [PATCH] grep: correctly initialize help-all option

[René Scharfe]

Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:
> The "help-all" option is being initialized with a wrong value.
> While being semantically wrong this can also cause a gcc
> segmentation fault on ARMv7 hardfloat platforms with a hardened
> toolchain. Fix this by initializing with the correct value.

Thanks for your report and patch.  A few comments:

Sign-off?  (See "Sign your work" in Documentation/SubmittingPatches)

> ---
>   builtin/grep.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index abc4400..c0bf005 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>   			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
>   		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
>   			 N_("allow calling of grep(1) (ignored by this build)")),
> -		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
> +		{ OPTION_CALLBACK, 0, "help-all", &opt, NULL, N_("show usage"),
>   		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>   		OPT_END()
>   	};

help_callback() returns -1 immediately, IOW the value pointer is never 
used anyway.  So why does your change make a difference?  *puzzled*

We could pass NULL instead, as in builtin/show-ref.c, which would make 
it clear that the pointer is just a dummy.

Q: Why does the callback exist in the first place?  A: parse_options() 
handles -h and --help-all automatically by showing the list of available 
options.  This is good for most commands for consistency's sake, but bad 
for commands that want to use -h for something else.  That's why it can 
be turned off with the flag PARSE_OPT_NO_INTERNAL_HELP, but that flag 
disables the long option as well.  It's added back by grep and show-ref 
explicitly, to at least provide --help-all consistently across all commands.

We could solve this problem centrally by checking for -h and --help-all 
only after looking through the struct option list supplied to 
parse_options_step() instead of before and getting rid of the then 
unneeded callbacks for --help-all in grep and show-ref.  Are there any 
downsides to that approach?

René

Re: [PATCH] grep: correctly initialize help-all option

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2961 bytes --]

On Thu, Apr 09, 2015 at 11:55:01PM +0200, René Scharfe wrote:
> Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:
> > The "help-all" option is being initialized with a wrong value.
> > While being semantically wrong this can also cause a gcc
> > segmentation fault on ARMv7 hardfloat platforms with a hardened
> > toolchain. Fix this by initializing with the correct value.
> 
> Thanks for your report and patch.  A few comments:
> 
> Sign-off?  (See "Sign your work" in Documentation/SubmittingPatches)

Fixed in v2 of the patch already.

> 
> > ---
> >   builtin/grep.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/grep.c b/builtin/grep.c
> > index abc4400..c0bf005 100644
> > --- a/builtin/grep.c
> > +++ b/builtin/grep.c
> > @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >   			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
> >   		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
> >   			 N_("allow calling of grep(1) (ignored by this build)")),
> > -		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
> > +		{ OPTION_CALLBACK, 0, "help-all", &opt, NULL, N_("show usage"),
> >   		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
> >   		OPT_END()
> >   	};
> 
> help_callback() returns -1 immediately, IOW the value pointer is never 
> used anyway.  So why does your change make a difference?  *puzzled*
> 
> We could pass NULL instead, as in builtin/show-ref.c, which would make 
> it clear that the pointer is just a dummy.

Changed in v2, as well.

In general the change won't make any difference when running the
command. But as said in the commit message it caused gcc (gcc
version 4.8.3 (Gentoo Hardened 4.8.3 p1.1, pie-0.5.9), ARMv7 HF)
to segfault when &options was passed in as value. Even though
this is probably an error in gcc we can easily work around it by
doing the Right Thing here.

> Q: Why does the callback exist in the first place?  A: parse_options() 
> handles -h and --help-all automatically by showing the list of available 
> options.  This is good for most commands for consistency's sake, but bad 
> for commands that want to use -h for something else.  That's why it can 
> be turned off with the flag PARSE_OPT_NO_INTERNAL_HELP, but that flag 
> disables the long option as well.  It's added back by grep and show-ref 
> explicitly, to at least provide --help-all consistently across all commands.
> 
> We could solve this problem centrally by checking for -h and --help-all 
> only after looking through the struct option list supplied to 
> parse_options_step() instead of before and getting rid of the then 
> unneeded callbacks for --help-all in grep and show-ref.  Are there any 
> downsides to that approach?

I'll leave that question to the folks more experienced with the
code base.

> René

Thanks for your feedback.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] grep: correctly initialize help-all option

[René Scharfe]

Am 10.04.2015 um 07:22 schrieb Patrick Steinhardt:
> On Thu, Apr 09, 2015 at 11:55:01PM +0200, René Scharfe wrote:
>> Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:
>>> ---
>>>    builtin/grep.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/builtin/grep.c b/builtin/grep.c
>>> index abc4400..c0bf005 100644
>>> --- a/builtin/grep.c
>>> +++ b/builtin/grep.c
>>> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>>>    			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
>>>    		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
>>>    			 N_("allow calling of grep(1) (ignored by this build)")),
>>> -		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
>>> +		{ OPTION_CALLBACK, 0, "help-all", &opt, NULL, N_("show usage"),
>>>    		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
>>>    		OPT_END()
>>>    	};
>>
>> help_callback() returns -1 immediately, IOW the value pointer is never
>> used anyway.  So why does your change make a difference?  *puzzled*
>>
>> We could pass NULL instead, as in builtin/show-ref.c, which would make
>> it clear that the pointer is just a dummy.
>
> Changed in v2, as well.

Thank you.  I should really re-fetch from Gmane before finishing an 
interrupted reply..

> In general the change won't make any difference when running the
> command. But as said in the commit message it caused gcc (gcc
> version 4.8.3 (Gentoo Hardened 4.8.3 p1.1, pie-0.5.9), ARMv7 HF)
> to segfault when &options was passed in as value. Even though
> this is probably an error in gcc we can easily work around it by
> doing the Right Thing here.

OK, so does it crash on this one-liner as well?

	struct t {void *p;} s = {&s};

Or on this?

	void *p = &p;

If yes then the author of the hardening feature might be interested in 
this fact.

René

Re: [PATCH] grep: correctly initialize help-all option

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]

On Fri, Apr 10, 2015 at 06:34:29PM +0200, René Scharfe wrote:
> Am 10.04.2015 um 07:22 schrieb Patrick Steinhardt:
> > On Thu, Apr 09, 2015 at 11:55:01PM +0200, René Scharfe wrote:
> >> Am 09.04.2015 um 15:41 schrieb Patrick Steinhardt:
> >>> ---
> >>>    builtin/grep.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/builtin/grep.c b/builtin/grep.c
> >>> index abc4400..c0bf005 100644
> >>> --- a/builtin/grep.c
> >>> +++ b/builtin/grep.c
> >>> @@ -738,7 +738,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
> >>>    			PARSE_OPT_OPTARG, NULL, (intptr_t)default_pager },
> >>>    		OPT_BOOL(0, "ext-grep", &external_grep_allowed__ignored,
> >>>    			 N_("allow calling of grep(1) (ignored by this build)")),
> >>> -		{ OPTION_CALLBACK, 0, "help-all", &options, NULL, N_("show usage"),
> >>> +		{ OPTION_CALLBACK, 0, "help-all", &opt, NULL, N_("show usage"),
> >>>    		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, help_callback },
> >>>    		OPT_END()
> >>>    	};
> >>
> >> help_callback() returns -1 immediately, IOW the value pointer is never
> >> used anyway.  So why does your change make a difference?  *puzzled*
> >>
> >> We could pass NULL instead, as in builtin/show-ref.c, which would make
> >> it clear that the pointer is just a dummy.
> >
> > Changed in v2, as well.
> 
> Thank you.  I should really re-fetch from Gmane before finishing an 
> interrupted reply..
> 
> > In general the change won't make any difference when running the
> > command. But as said in the commit message it caused gcc (gcc
> > version 4.8.3 (Gentoo Hardened 4.8.3 p1.1, pie-0.5.9), ARMv7 HF)
> > to segfault when &options was passed in as value. Even though
> > this is probably an error in gcc we can easily work around it by
> > doing the Right Thing here.
> 
> OK, so does it crash on this one-liner as well?
> 
> 	struct t {void *p;} s = {&s};
> 
> Or on this?
> 
> 	void *p = &p;
> 
> If yes then the author of the hardening feature might be interested in 
> this fact.
> 
> René

No, the segfault is not triggered by this. I've also tried to use
the exact arguments to gcc that are used when compiling grep.c,
but to no avail. I'll maybe try to reproduce this with a minimal
testcase next week.

Regards
Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]