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

[Nix <nix at esperi dot org dot uk>]

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.

-- 
NULL && (void)