[08/12] De-PLTize __stack_chk_fail internal calls within libc.so.

  On 15 Dec 2016, Florian Weimer uttered the following:

> On 12/15/2016 03:29 PM, Nix wrote:
>> On 15 Dec 2016, Florian Weimer told this:
>>> I wonder if it's better to add something to $(no-stack-protector) and use that in the conditional.
>>
>> That was my other option, but the total absence of anything in
>> configure.ac passing -D made me think twice.
>>
>> Something like this? (even more untested than the last one, if
>> possible -- but adds a new possibility: we can now differentiate between
>> "glibc built without stack protector" and "glibc built with stack
>> protector but this file doesn't have it" without relying on GCC
>> predefined macros. The __WITH_ naming scheme is completely arbitrary
>> and I can change it to anything you prefer.)
>
> WITH_STACK_PROTECTOR (without the leading underscores) looks okay to
> me because it's only used at build time. Or you could call it
> STACK_PROTECTOR_LEVEL, to match the other variable.

That's definitely the best choice, because it means we don't need
to change configure.ac as much, and we don't have a confusing mix
of nonzero STACK_PROTECTOR_LEVEL and zero WITH_STACK_PROTECTOR (or
whatever) in some files.  It needs a tiny adjustment of config.h.in
to stop it being overridden though.

Something like this:
  

On 15 Dec 2016, nix@esperi.org.uk verbalised:

> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
> index 36908b5..15ff56a 100644
> --- a/sysdeps/generic/symbol-hacks.h
> +++ b/sysdeps/generic/symbol-hacks.h
> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>  
>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>     similar adjustments to avoid going through the PLT.  */
> +# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>  asm ("__stack_chk_fail = __stack_chk_fail_local");
> +# endif
>  #endif

This causes (minor) problems on SPARC:

Extra PLT reference: libc.so: __stack_chk_fail

Whether we can disregard this, I don't know, but it does feel wrong.
  

On 12/15/2016 06:24 PM, Nix wrote:
> On 15 Dec 2016, nix@esperi.org.uk verbalised:
>
>> diff --git a/sysdeps/generic/symbol-hacks.h b/sysdeps/generic/symbol-hacks.h
>> index 36908b5..15ff56a 100644
>> --- a/sysdeps/generic/symbol-hacks.h
>> +++ b/sysdeps/generic/symbol-hacks.h
>> @@ -7,5 +7,7 @@ asm ("memcpy = __GI_memcpy");
>>
>>  /* -fstack-protector generates calls to __stack_chk_fail, which need
>>     similar adjustments to avoid going through the PLT.  */
>> +# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>>  asm ("__stack_chk_fail = __stack_chk_fail_local");
>> +# endif
>>  #endif
>
> This causes (minor) problems on SPARC:
>
> Extra PLT reference: libc.so: __stack_chk_fail
>
> Whether we can disregard this, I don't know, but it does feel wrong.

Well, avoiding this is the point of __stack_chk_fail_local, isn't it? 
So we surely can't ignore it.

I think what is going on is that once a symbol has a hidden anywhere in 
a static link, all references to it are turned hidden.  Previously, this 
hidden reference occurred in the file which *defined* 
__stack_chk_fail_local, but is now gone from there.  (At the assembler 
level, the .hidden directive is markedly different from the GCC 
visibility attribute.)

Could you try this?

# if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
asm (".hidden __stack_chk_fail_local");
asm ("__stack_chk_fail = __stack_chk_fail_local");
# endif

Thanks,
Florian
  

On 15 Dec 2016, Florian Weimer verbalised:

> On 12/15/2016 06:24 PM, Nix wrote:
>> This causes (minor) problems on SPARC:
>>
>> Extra PLT reference: libc.so: __stack_chk_fail
>>
>> Whether we can disregard this, I don't know, but it does feel wrong.
>
> Well, avoiding this is the point of __stack_chk_fail_local, isn't it?
> So we surely can't ignore it.

Agreed. (I just wasn't sure I could remember what this was added for...)

> I think what is going on is that once a symbol has a hidden anywhere
> in a static link, all references to it are turned hidden. Previously,
> this hidden reference occurred in the file which *defined*
> __stack_chk_fail_local, but is now gone from there. (At the assembler

Oof. And with no such reference, it ends up visible again...

> Could you try this?
>
> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
> asm (".hidden __stack_chk_fail_local");
> asm ("__stack_chk_fail = __stack_chk_fail_local");
> # endif

No change :( the only reference to __stack_chk_fail is still inside
stack_chk_fail_local:

Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:

Name                        Value            Class  Type     Size             Line Section

__GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
__GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
__GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
__stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
__stack_chk_fail_local     |0000000000000000|GLOBAL|FUNC    |0000000000000010|    |.text
libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE    |0000000000000000|    |ABS

(And, of course, this code is not affected by your suggestion, because
it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)
  

On 12/15/2016 09:00 PM, Nix wrote:

>> Could you try this?
>>
>> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>> asm (".hidden __stack_chk_fail_local");
>> asm ("__stack_chk_fail = __stack_chk_fail_local");
>> # endif
>
> No change :( the only reference to __stack_chk_fail is still inside
> stack_chk_fail_local:
>
> Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:
>
> Name                        Value            Class  Type     Size             Line Section
>
> __GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
> __GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
> __GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
> __stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
> __stack_chk_fail_local     |0000000000000000|GLOBAL|FUNC    |0000000000000010|    |.text
> libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE    |0000000000000000|    |ABS
>
> (And, of course, this code is not affected by your suggestion, because
> it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)

I think this attempt at PLT avoidance within libc.so itself is subtly 
wrong.  We need to mirror more closely what 
libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this 
from the __stack_chk_fail_local definition used in other DSOs.

I think this means removing any definition of a C function definition 
called __stack_chk_fail_local from libc.so, and instead use a strong 
alias from __stack_chk_fail to __stack_chk_fail_local to define the 
symbol.  The alias will not incorporate a PLT reference.  If you look at 
include/libc-symbols.h, strong_alias and hidden_def are quite similar.

It's too late for me to try this today. :-/

Florian
  

* Florian Weimer:

> On 12/15/2016 09:00 PM, Nix wrote:
>
>>> Could you try this?
>>>
>>> # if defined STACK_PROTECTOR_LEVEL && STACK_PROTECTOR_LEVEL > 0
>>> asm (".hidden __stack_chk_fail_local");
>>> asm ("__stack_chk_fail = __stack_chk_fail_local");
>>> # endif
>>
>> No change :( the only reference to __stack_chk_fail is still inside
>> stack_chk_fail_local:
>>
>> Symbols from libc_pic.a[libc-stack_chk_fail_local.os]:
>>
>> Name Value Class Type Size Line Section
>>
>> __GI_memcpy                ||GLOBAL|NOTYPE  ||    |UNDEF
>> __GI_memmove               ||GLOBAL|NOTYPE  ||    |UNDEF
>> __GI_memset                ||GLOBAL|NOTYPE  ||    |UNDEF
>> __stack_chk_fail           ||GLOBAL|NOTYPE  ||    |UNDEF
>> __stack_chk_fail_local |0000000000000000|GLOBAL|FUNC
>> |0000000000000010| |.text
>> libc-stack_chk_fail_local.c|0000000000000000|LOCAL |FILE
>> |0000000000000000| |ABS
>>
>> (And, of course, this code is not affected by your suggestion, because
>> it's compiled with -fno-stack-protector -DSTACK_PROTECTOR_LEVEL=0.)
>
> I think this attempt at PLT avoidance within libc.so itself is subtly 
> wrong.  We need to mirror more closely what 
> libc_hidden_proto/libc_hidden_def does, and perhaps disentangle this 
> from the __stack_chk_fail_local definition used in other DSOs.
>
> I think this means removing any definition of a C function definition 
> called __stack_chk_fail_local from libc.so, and instead use a strong 
> alias from __stack_chk_fail to __stack_chk_fail_local to define the 
> symbol.  The alias will not incorporate a PLT reference.  If you look at 
> include/libc-symbols.h, strong_alias and hidden_def are quite similar.

It may also be a good idea to switch to a different symbol for
__stack_chk_fail_local because this collides with the name GCC uses on
some architectures for a similar purpose.  Or is this the intent here?
  

On 15 Dec 2016, Florian Weimer spake thusly:

> * Florian Weimer:
>
>> I think this means removing any definition of a C function definition 
>> called __stack_chk_fail_local from libc.so, and instead use a strong 
>> alias from __stack_chk_fail to __stack_chk_fail_local to define the 
>> symbol.  The alias will not incorporate a PLT reference.  If you look at 
>> include/libc-symbols.h, strong_alias and hidden_def are quite similar.
>
> It may also be a good idea to switch to a different symbol for
> __stack_chk_fail_local because this collides with the name GCC uses on
> some architectures for a similar purpose.  Or is this the intent here?

That's the point. On targets where __stack_chk_fail_local is called
(e.g. x86-32), we're not generating the calls to this thing: GCC is.
We cannot pick a different name.
  

> On 15 Dec 2016, Florian Weimer spake thusly:
>
>> * Florian Weimer:
>>
>>> I think this means removing any definition of a C function definition 
>>> called __stack_chk_fail_local from libc.so, and instead use a strong 
>>> alias from __stack_chk_fail to __stack_chk_fail_local to define the 
>>> symbol.  The alias will not incorporate a PLT reference.  If you look at 
>>> include/libc-symbols.h, strong_alias and hidden_def are quite similar.
>>
>> It may also be a good idea to switch to a different symbol for
>> __stack_chk_fail_local because this collides with the name GCC uses on
>> some architectures for a similar purpose.  Or is this the intent here?
>
> That's the point. On targets where __stack_chk_fail_local is called
> (e.g. x86-32), we're not generating the calls to this thing: GCC is.
> We cannot pick a different name.

Ahh, and GCC does not synthesize a local definition, so there is no
actual collision.

Then the strong_alias approach should work for libc.so.
libc_nonshared.a still needs a hidden function definition, of course.