Re: [PATCH] Remove debug/stack_chk_fail_local.c [BZ #21740]

["H.J. Lu" <hjl.tools@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

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?


-- 
H.J.

[-- Attachment #2: 0001-Add-__stack_chk_fail_local-alias-only-to-libc.so-BZ-.patch --]
[-- Type: text/x-patch, Size: 1598 bytes --]

From 217275043ad73f922d07f42e5fc1a4be70183209 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] Add __stack_chk_fail_local alias only to libc.so [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.

unconditionally added

strong_alias (__stack_chk_fail, __stack_chk_fail_local)

Since libc.a and libc_nonshared.a has debug/stack_chk_fail_local.c,
__stack_chk_fail_local alias should be limited to libc.so.

Tested on x86-64 with --enable-stack-protector=all and got

FAIL: elf/tst-env-setuid
FAIL: elf/tst-env-setuid-tunables
FAIL: stdlib/tst-secure-getenv

which are the same as without this patch.

	[BZ #21740]
	* debug/stack_chk_fail.c (__stack_chk_fail_local): Add the
	alias only if SHARED is defined.
---
 debug/stack_chk_fail.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/debug/stack_chk_fail.c b/debug/stack_chk_fail.c
index 4f73464..120d269 100644
--- a/debug/stack_chk_fail.c
+++ b/debug/stack_chk_fail.c
@@ -28,4 +28,8 @@ __stack_chk_fail (void)
   __fortify_fail ("stack smashing detected");
 }
 
+#ifdef SHARED
+/* Some targets call __stack_chk_fail_local as a hidden function within
+   libc.so.  */
 strong_alias (__stack_chk_fail, __stack_chk_fail_local)
+#endif
-- 
2.9.4