This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 01/18] Configury support for --enable-stack-protector.
- From: Nix <nix at esperi dot org dot uk>
- To: Mike Frysinger <vapier at gentoo dot org>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 09 Mar 2016 23:29:39 +0000
- Subject: Re: [PATCH 01/18] Configury support for --enable-stack-protector.
- Authentication-results: sourceware.org; auth=none
- References: <1457445064-7107-1-git-send-email-nix at esperi dot org dot uk> <1457445064-7107-2-git-send-email-nix at esperi dot org dot uk> <20160309223649 dot GB6588 at vapier dot lan>
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)