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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]