[01/18] Configury support for --enable-stack-protector.

  From: Nick Alcock <nick.alcock@oracle.com>

This adds =all and =strong, with obvious semantics, and with a rather
arbitrarily-chosen default off, which we might well want to change to
something stronger once this patch has been tested by people other than
me.

We don't validate the value of the option yet: that's in a later patch.
Nor do we use it for anything at this stage.

We differentiate between 'the compiler understands -fstack-protector'
and 'the user wanted -fstack-protector' so that we can pass
-fno-stack-protector in appropriate places even if the user didn't want
to turn on -fstack-protector for other parts.  (This helps us overcome
another existing limitation, that glibc doesn't work with GCCs hacked
to pass in -fstack-protector by default.)

We might want to add another configuration option to turn on
-fstack-protector for nscd and other network-facing operations by
default, but for now I've stuck with one option to control everything.

v2: documentation in install.texi; better description of the option.
    INSTALL regenerated.
v3: Substitute in no_stack_protector.

	[BZ #7065]
	* configure.ac (libc_cv_ssp): Move up.
	(libc_cv_ssp_strong): Likewise.
	(libc_cv_ssp_all): New.
	(stack_protector): Augment, adding -fstack-protector-all.
	(no_stack_protector): New.
	(AC_ARG_ENABLE(stack-protector)): New configure flag.
	* manual/install.texi (--enable-stack-protector): Document it.
	* INSTALL: Regenerate.
---
 INSTALL             | 39 ++++++++++++++++++++-----------
 configure.ac        | 67 +++++++++++++++++++++++++++++++++++++----------------
 manual/install.texi | 12 ++++++++++
 3 files changed, 84 insertions(+), 34 deletions(-)
  

On 08 Mar 2016 13:50, Nix wrote:
> +AC_ARG_ENABLE([stack-protector],
> +            AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@],

8 spaces -> tabs, and re-align the 2nd arg

> +                           [Detect stack overflows in glibc functions, either with local buffers (yes), or with those plus arrays (strong), or all functions (all)]),

is this really necessary ?  can't we just say something like "pass
-fstack-protector[-all|-strong]" and leave the rest to the gcc manual ?

> +            [enable_stack_protector=no])

should we default this to auto-detecting strong ?
or at least make that a follow up patch in this series ?

> +case x"$enable_stack_protector" in
> +    xall|xyes|xno|xstrong) ;;

don't need the x prefix.  that historical wart is related to empty
string compares with the test command, not the case command.

> +    *) AC_MSG_ERROR([Not a valid argument for --enable-stack-protector]);;

don't indent case statements

> +if test x$libc_cv_ssp = xyes; then

quote the LHS -- cache vars can be set via the env or config.site files

> +    no_stack_protector=-fno-stack-protector

indent with two spaces

> +if test x$enable_stack_protector = xyes && test $libc_cv_ssp = yes; then
> +  stack_protector=-fstack-protector
> +elif test x$enable_stack_protector = xall && test $libc_cv_ssp_all = yes; then
> +  stack_protector=-fstack-protector-all
> +elif test x$enable_stack_protector = xstrong && test $libc_cv_ssp_strong = yes; then
> +  stack_protector=-fstack-protector-strong
> +fi

all of these strings should be quoted imo.  they were before you changed
things too.
-mike
  

On 9 Mar 2016, Mike Frysinger said:

> On 08 Mar 2016 13:50, Nix wrote:
>> +AC_ARG_ENABLE([stack-protector],
>> +            AC_HELP_STRING([--enable-stack-protector=@<:@yes|no|all|strong@:>@],
>
> 8 spaces -> tabs, and re-align the 2nd arg

Fixed.

Second arg looks perfectly alright to me, though: it's aligned in my
tree, and in the patch I emailed out. Looks like your MUA has done
something to it.  (Though it was aligned with spaces.)

>> +                           [Detect stack overflows in glibc functions, either with local buffers (yes), or with those plus arrays (strong), or all functions (all)]),
>
> is this really necessary ?  can't we just say something like "pass
> -fstack-protector[-all|-strong]" and leave the rest to the gcc manual ?

Er, yes, and Andreas even asked me to do that, and I thought oh yes
that's a good idea and then didn't do it. Um. It's done now.

How about

[Use -fstack-protector[-all|-strong] to detect glibc stack overflows]

so that at least it becomes clear what it means to 'protect' the stack
without having to dive into the manual? I'd like to say a tiny bit more
than just say "pass": to what? why?

(but maybe the last five words are redundant after all. I dunno. Still,
even if you take them off it doesn't fit on one line in configure --help
output in my tests, so the extra words don't actually *cost* anything.)

>> +            [enable_stack_protector=no])
>
> should we default this to auto-detecting strong ?
> or at least make that a follow up patch in this series ?

I thought that might be too contentious! but if people want to flip the
default, it's fine by me. (Just don't flip it to -all: that's probably
best left to hardened distros, since it probably does have a real
performance cost...)

>> +case x"$enable_stack_protector" in
>> +    xall|xyes|xno|xstrong) ;;
>
> don't need the x prefix.  that historical wart is related to empty
> string compares with the test command, not the case command.

It's historical terror. (And I have distinct memories of seeing problems
with case on ancient HP-UX boxes, HP-UX 9 or thereabouts. This is
obviously relevant since people build glibc on HP-UX 9 boxes *all the
time*.)

Fixed. :)

>> +    *) AC_MSG_ERROR([Not a valid argument for --enable-stack-protector]);;
>
> don't indent case statements

Ew, really?

... gods, you're right, none of them are indented. I never noticed that.
Fixed.

>> +if test x$libc_cv_ssp = xyes; then
>
> quote the LHS -- cache vars can be set via the env or config.site files

Fixed. (And if I'm quoting it I don't need to x it: fixed that, too.)

>> +    no_stack_protector=-fno-stack-protector
>
> indent with two spaces

Oops, yes. (Also fixed a couple of places in patch 9 that made the same
mistake.)

>> +if test x$enable_stack_protector = xyes && test $libc_cv_ssp = yes; then
>> +  stack_protector=-fstack-protector
>> +elif test x$enable_stack_protector = xall && test $libc_cv_ssp_all = yes; then
>> +  stack_protector=-fstack-protector-all
>> +elif test x$enable_stack_protector = xstrong && test $libc_cv_ssp_strong = yes; then
>> +  stack_protector=-fstack-protector-strong
>> +fi
>
> all of these strings should be quoted imo.  they were before you changed
> things too.

Yeah, I removed the quotation because it was useless -- the value of
$enable_stack_protector is constrained by the check further up, and you
don't need to quote a single word with no shell metacharacters in it
(and - is not a shell metacharacter). The former argument is debatable
-- someone might remove the check further up in later maintenance
without even noticing this code exists -- but the latter is not, really:
any shell in which the string '-fstack-protector' needed quoting would
not be a shell that could run configure at all.

So, quoted all the $libc_cv_ssps and $enable_stack_protectors, de-x'ed
the latter, but not yet quoted the assignments, because... what for?
Also quoted and de-x'ed the stuff in the checks added in patch 9, for
the same reason.
  

On 9 Mar 2016, nix@esperi.org.uk uttered the following:

> [Use -fstack-protector[-all|-strong] to detect glibc stack overflows]

Obviously this was stupid. That's not what -fstack-protector does!
What was I thinking...

This is better:

[Use -fstack-protector[-all|-strong] to detect glibc buffer overflows]