diff mbox

Remove debug/stack_chk_fail_local.c [BZ #21740]

Message ID CAMe9rOrcNvQJiivdRUoKx4UyAKvs1g9amVP0BMP8sQU1OKMO_w@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu July 18, 2017, 4:23 p.m. UTC 
On Sun, Jul 16, 2017 at 12:22 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jul 10, 2017 at 10:34 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Jul 10, 2017 at 6:29 AM, Nick Alcock <nix@esperi.org.uk> wrote:
>>> On 10 Jul 2017, H. J. Lu said:
>>>> On Mon, Jul 10, 2017 at 11:50:02AM +0100, Nick Alcock wrote:
>>>>> If it passes a test build with --enable-stack-protector=all without
>>>>> pulling junk into ld.so and exploding at ld.so link time, sure. (That's
>>>>> what happened every time I tried to remove this stuff before, but I may
>>>>> have failed to notice that this may not be necessary any more.)
>>>>
>>>> Here is the updated patch.  tst-_dl_addr_inside_object should be
>>>> linked with $(dummy-stack-chk-fail).  Otherwise, __stack_chk_fail is
>>>> undefined.  OK for master branch?
>>>
>>> I presume this is because it's in $(all-nonlib)? Are other all-nonlib
>>> things similarly affected? (If they are, is the code in Makerules
>>> perhaps a better place to adjust this?)
>>>
>>> I guess the only affected nonlib things would be things that directly
>>> link against objects that will otherwise end up in the shared libc or
>>> ld.so, which means this is the only affected test (since only those
>>> things reference the usually-hidden __stack_chk_fail). If so, I have no
>>> objection to this patch, but maybe a comment explaining this would be a
>>> good idea so that if more such tests turn up in future this unusual
>>> piece of linking can be generalized a bit.
>>>
>>> Modulo that, I have no objections, but of course I also have no actual
>>> right to ack anything whatsoever :)
>>>
>>>>> > -/* On some architectures, this helps needless PIC pointer setup
>>>>> > -   that would be needed just for the __stack_chk_fail call.  */
>>>>>
>>>>> Does anyone know what architectures these might be? Presumably x86-32...
>>>>>
>>>>
>>>> config/i386/i386.c:   __stack_chk_fail_local hidden function instead of calling
>>>
>>> I presume you tested a build on x86-32 :) I guess that will suffice...
>>
>> We must keep debug/stack_chk_fail_local.c for libc_nonshared.a.
>> Here is the updated patch to add __stack_chk_fail_local alias only
>> to libc.so.
>>
>> Tested on i686 and x86-64 with and without --enable-stack-protector=all.
>> OK for master?
>>
>
> Any objections?
>
> https://sourceware.org/ml/libc-alpha/2017-07/msg00406.html
>

Here is the updated patch.   I will check it tomorrow if there are no
objections.

Comments

Nix July 18, 2017, 5:11 p.m. UTC | #1 
On 18 Jul 2017, H. J. Lu uttered the following:
> From 47d95763a35fbc83ac871b10fc59f9eca1ef0a40 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Sun, 9 Jul 2017 08:39:17 -0700
> Subject: [PATCH] Don't add stack_chk_fail_local.o to libc.a [BZ #21740]
>
> commit 524a8ef2ad76af8ac049293d993a1856b0d888fb
> Author: Nick Alcock <nick.alcock@oracle.com>
> Date:   Mon Dec 26 10:08:57 2016 +0100
>
>     PLT avoidance for __stack_chk_fail [BZ #7065]
>
>     Add a hidden __stack_chk_fail_local alias to libc.so,
>     and make sure that on targets which use __stack_chk_fail,
>     this does not introduce a local PLT reference into libc.so.
>
> which unconditionally added
>
> strong_alias (__stack_chk_fail, __stack_chk_fail_local)
>
> defines __stack_chk_fail_local as an alias of __stack_chk_fail in libc.a.
> There is no need to add stack_chk_fail_local.o to libc.a.  We only need
> to add stack_chk_fail_local.oS to libc_nonshared.a.

Completely agreed -- we only need the _local alias because of internal
calls within the *shared* libc and related objects (in all of which the
symbol is hidden). The nonshared libc just calls __stack_chk_fail
directly and should not reference stack_chk_fail_local at all, and
nothing else should have undefined references to it because the only
objects that presently reference it should also contain a definition of
it.
H.J. Lu July 19, 2017, 3:19 p.m. UTC | #2 
On Tue, Jul 18, 2017 at 10:11 AM, Nick Alcock <nix@esperi.org.uk> wrote:
> On 18 Jul 2017, H. J. Lu uttered the following:
>> From 47d95763a35fbc83ac871b10fc59f9eca1ef0a40 Mon Sep 17 00:00:00 2001
>> From: "H.J. Lu" <hjl.tools@gmail.com>
>> Date: Sun, 9 Jul 2017 08:39:17 -0700
>> Subject: [PATCH] Don't add stack_chk_fail_local.o to libc.a [BZ #21740]
>>
>> commit 524a8ef2ad76af8ac049293d993a1856b0d888fb
>> Author: Nick Alcock <nick.alcock@oracle.com>
>> Date:   Mon Dec 26 10:08:57 2016 +0100
>>
>>     PLT avoidance for __stack_chk_fail [BZ #7065]
>>
>>     Add a hidden __stack_chk_fail_local alias to libc.so,
>>     and make sure that on targets which use __stack_chk_fail,
>>     this does not introduce a local PLT reference into libc.so.
>>
>> which unconditionally added
>>
>> strong_alias (__stack_chk_fail, __stack_chk_fail_local)
>>
>> defines __stack_chk_fail_local as an alias of __stack_chk_fail in libc.a.
>> There is no need to add stack_chk_fail_local.o to libc.a.  We only need
>> to add stack_chk_fail_local.oS to libc_nonshared.a.
>
> Completely agreed -- we only need the _local alias because of internal
> calls within the *shared* libc and related objects (in all of which the
> symbol is hidden). The nonshared libc just calls __stack_chk_fail
> directly and should not reference stack_chk_fail_local at all, and
> nothing else should have undefined references to it because the only
> objects that presently reference it should also contain a definition of
> it.

I am checking it in.
diff mbox

Patch

From 47d95763a35fbc83ac871b10fc59f9eca1ef0a40 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 9 Jul 2017 08:39:17 -0700
Subject: [PATCH] Don't add stack_chk_fail_local.o to libc.a [BZ #21740]

commit 524a8ef2ad76af8ac049293d993a1856b0d888fb
Author: Nick Alcock <nick.alcock@oracle.com>
Date:   Mon Dec 26 10:08:57 2016 +0100

    PLT avoidance for __stack_chk_fail [BZ #7065]

    Add a hidden __stack_chk_fail_local alias to libc.so,
    and make sure that on targets which use __stack_chk_fail,
    this does not introduce a local PLT reference into libc.so.

which unconditionally added

strong_alias (__stack_chk_fail, __stack_chk_fail_local)

defines __stack_chk_fail_local as an alias of __stack_chk_fail in libc.a.
There is no need to add stack_chk_fail_local.o to libc.a.  We only need
to add stack_chk_fail_local.oS to libc_nonshared.a.

Tested on x86-64:

[hjl@gnu-skl-1 build-x86_64-linux]$ nm libc.a | grep __stack_chk_fail
0000000000000000 T __stack_chk_fail
0000000000000000 T __stack_chk_fail_local
[hjl@gnu-skl-1 build-x86_64-linux]$ nm libc_nonshared.a | grep __stack_chk_fail_local
0000000000000000 T __stack_chk_fail_local
[hjl@gnu-skl-1 build-x86_64-linux]$

	[BZ #21740]
	* debug/Makefile (elide-routines.o): New.
---
 debug/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/debug/Makefile b/debug/Makefile
index ce5fa8801f..504bf875fe 100644
--- a/debug/Makefile
+++ b/debug/Makefile
@@ -53,6 +53,10 @@  routines  = backtrace backtracesyms backtracesymsfd noophooks \
 	    $(static-only-routines)
 static-only-routines := warning-nop stack_chk_fail_local
 
+# Don't add stack_chk_fail_local.o to libc.a since __stack_chk_fail_local
+# is an alias of __stack_chk_fail in stack_chk_fail.o.
+elide-routines.o := stack_chk_fail_local
+
 # Building the stack-protector failure routines with stack protection
 # makes no sense.
 
-- 
2.13.3