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

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

[H.J. Lu]

Since

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.

added

strong_alias (__stack_chk_fail, __stack_chk_fail_local)

to debug/stack_chk_fail.c, debug/stack_chk_fail_local.c should be
removed.

OK for master?

H.J.
---
	* [BZ #21740]
	* debug/Makefile (static-only-routines): Remove
	stack_chk_fail_local.
	* debug/stack_chk_fail_local.c: Removed.
---
 debug/Makefile               |  2 +-
 debug/stack_chk_fail_local.c | 46 --------------------------------------------
 2 files changed, 1 insertion(+), 47 deletions(-)
 delete mode 100644 debug/stack_chk_fail_local.c

diff --git a/debug/Makefile b/debug/Makefile
index cd4975c..136c9a1 100644
--- a/debug/Makefile
+++ b/debug/Makefile
@@ -51,7 +51,7 @@ routines  = backtrace backtracesyms backtracesymsfd noophooks \
 	    explicit_bzero_chk \
 	    stack_chk_fail fortify_fail \
 	    $(static-only-routines)
-static-only-routines := warning-nop stack_chk_fail_local
+static-only-routines := warning-nop
 
 # Building the stack-protector failure routines with stack protection
 # makes no sense.
diff --git a/debug/stack_chk_fail_local.c b/debug/stack_chk_fail_local.c
deleted file mode 100644
index eb0a759..0000000
--- a/debug/stack_chk_fail_local.c
+++ /dev/null
@@ -1,46 +0,0 @@
-/* Copyright (C) 2005-2017 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   In addition to the permissions in the GNU Lesser General Public
-   License, the Free Software Foundation gives you unlimited
-   permission to link the compiled version of this file with other
-   programs, and to distribute those programs without any restriction
-   coming from the use of this file. (The GNU Lesser General Public
-   License restrictions do apply in other respects; for example, they
-   cover modification of the file, and distribution when not linked
-   into another program.)
-
-   Note that people who make modified versions of this file are not
-   obligated to grant this special exception for their modified
-   versions; it is their choice whether to do so. The GNU Lesser
-   General Public License gives permission to release a modified
-   version without this exception; this exception also makes it
-   possible to release a modified version which carries forward this
-   exception.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <sys/cdefs.h>
-
-extern void __stack_chk_fail (void) __attribute__ ((noreturn));
-
-/* On some architectures, this helps needless PIC pointer setup
-   that would be needed just for the __stack_chk_fail call.  */
-
-void __attribute__ ((noreturn)) attribute_hidden
-__stack_chk_fail_local (void)
-{
-  __stack_chk_fail ();
-}
-- 
2.9.4

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

[Nick Alcock]

On 9 Jul 2017, H. J. Lu verbalised:

> Since
>
> 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.
>
> added
>
> strong_alias (__stack_chk_fail, __stack_chk_fail_local)
>
> to debug/stack_chk_fail.c, debug/stack_chk_fail_local.c should be
> removed.
>
> OK for master?

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.)

> -/* 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...

-- 
NULL && (void)

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

[H.J. Lu]

On Mon, Jul 10, 2017 at 11:50:02AM +0100, Nick Alcock wrote:
> On 9 Jul 2017, H. J. Lu verbalised:
> 
> > Since
> >
> > 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.
> >
> > added
> >
> > strong_alias (__stack_chk_fail, __stack_chk_fail_local)
> >
> > to debug/stack_chk_fail.c, debug/stack_chk_fail_local.c should be
> > removed.
> >
> > OK for master?
> 
> 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?

> 
> > -/* 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
config/powerpcspe/powerpcspe.c:   setup by using __stack_chk_fail_local hidden function instead of
config/rs6000/rs6000.c:   setup by using __stack_chk_fail_local hidden function instead of


H.J.
--
From 712e70de9743a61618001b4c6372a0e3d4fc1d90 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] Remove debug/stack_chk_fail_local.c [BZ #21740]

Since

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.

added

strong_alias (__stack_chk_fail, __stack_chk_fail_local)

to debug/stack_chk_fail.c, debug/stack_chk_fail_local.c should be
removed.  Since dummy __stack_chk_fail and __stack_chk_fail_local
symbols are used in ld.so, tst-_dl_addr_inside_object should be
linked with $(dummy-stack-chk-fail).   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/Makefile (static-only-routines): Remove
	stack_chk_fail_local.
	* debug/stack_chk_fail_local.c: Removed.
	* elf/Makefile (LDFLAGS-tst-_dl_addr_inside_object): New.
---
 debug/Makefile               |  2 +-
 debug/stack_chk_fail_local.c | 46 --------------------------------------------
 elf/Makefile                 |  1 +
 3 files changed, 2 insertions(+), 47 deletions(-)
 delete mode 100644 debug/stack_chk_fail_local.c

diff --git a/debug/Makefile b/debug/Makefile
index cd4975c..136c9a1 100644
--- a/debug/Makefile
+++ b/debug/Makefile
@@ -51,7 +51,7 @@ routines  = backtrace backtracesyms backtracesymsfd noophooks \
 	    explicit_bzero_chk \
 	    stack_chk_fail fortify_fail \
 	    $(static-only-routines)
-static-only-routines := warning-nop stack_chk_fail_local
+static-only-routines := warning-nop
 
 # Building the stack-protector failure routines with stack protection
 # makes no sense.
diff --git a/debug/stack_chk_fail_local.c b/debug/stack_chk_fail_local.c
deleted file mode 100644
index eb0a759..0000000
--- a/debug/stack_chk_fail_local.c
+++ /dev/null
@@ -1,46 +0,0 @@
-/* Copyright (C) 2005-2017 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   In addition to the permissions in the GNU Lesser General Public
-   License, the Free Software Foundation gives you unlimited
-   permission to link the compiled version of this file with other
-   programs, and to distribute those programs without any restriction
-   coming from the use of this file. (The GNU Lesser General Public
-   License restrictions do apply in other respects; for example, they
-   cover modification of the file, and distribution when not linked
-   into another program.)
-
-   Note that people who make modified versions of this file are not
-   obligated to grant this special exception for their modified
-   versions; it is their choice whether to do so. The GNU Lesser
-   General Public License gives permission to release a modified
-   version without this exception; this exception also makes it
-   possible to release a modified version which carries forward this
-   exception.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <sys/cdefs.h>
-
-extern void __stack_chk_fail (void) __attribute__ ((noreturn));
-
-/* On some architectures, this helps needless PIC pointer setup
-   that would be needed just for the __stack_chk_fail call.  */
-
-void __attribute__ ((noreturn)) attribute_hidden
-__stack_chk_fail_local (void)
-{
-  __stack_chk_fail ();
-}
diff --git a/elf/Makefile b/elf/Makefile
index e758a4c..fb2d855 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -369,6 +369,7 @@ tests-internal += tst-_dl_addr_inside_object
 tests-pie += tst-_dl_addr_inside_object
 $(objpfx)tst-_dl_addr_inside_object: $(objpfx)dl-addr-obj.os
 CFLAGS-tst-_dl_addr_inside_object.c += $(PIE-ccflag)
+LDFLAGS-tst-_dl_addr_inside_object = $(dummy-stack-chk-fail)
 endif
 
 # By default tst-linkall-static should try to use crypt routines to test
-- 
2.9.4

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

[H.J. Lu]

On Mon, Jul 10, 2017 at 6:14 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Jul 10, 2017 at 11:50:02AM +0100, Nick Alcock wrote:
>> On 9 Jul 2017, H. J. Lu verbalised:
>>
>> > Since
>> >
>> > 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.
>> >
>> > added
>> >
>> > strong_alias (__stack_chk_fail, __stack_chk_fail_local)
>> >
>> > to debug/stack_chk_fail.c, debug/stack_chk_fail_local.c should be
>> > removed.
>> >
>> > OK for master?
>>
>> 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?
>
>>
>> > -/* 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
> config/powerpcspe/powerpcspe.c:   setup by using __stack_chk_fail_local hidden function instead of
> config/rs6000/rs6000.c:   setup by using __stack_chk_fail_local hidden function instead of
>
>
> H.J.
> --
> From 712e70de9743a61618001b4c6372a0e3d4fc1d90 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] Remove debug/stack_chk_fail_local.c [BZ #21740]
>
> Since
>
> 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.
>
> added
>
> strong_alias (__stack_chk_fail, __stack_chk_fail_local)
>
> to debug/stack_chk_fail.c, debug/stack_chk_fail_local.c should be
> removed.  Since dummy __stack_chk_fail and __stack_chk_fail_local
> symbols are used in ld.so, tst-_dl_addr_inside_object should be
> linked with $(dummy-stack-chk-fail).   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/Makefile (static-only-routines): Remove
>         stack_chk_fail_local.
>         * debug/stack_chk_fail_local.c: Removed.
>         * elf/Makefile (LDFLAGS-tst-_dl_addr_inside_object): New.
> ---
>  debug/Makefile               |  2 +-
>  debug/stack_chk_fail_local.c | 46 --------------------------------------------
>  elf/Makefile                 |  1 +
>  3 files changed, 2 insertions(+), 47 deletions(-)
>  delete mode 100644 debug/stack_chk_fail_local.c
>
> diff --git a/debug/Makefile b/debug/Makefile
> index cd4975c..136c9a1 100644
> --- a/debug/Makefile
> +++ b/debug/Makefile
> @@ -51,7 +51,7 @@ routines  = backtrace backtracesyms backtracesymsfd noophooks \
>             explicit_bzero_chk \
>             stack_chk_fail fortify_fail \
>             $(static-only-routines)
> -static-only-routines := warning-nop stack_chk_fail_local
> +static-only-routines := warning-nop
>
>  # Building the stack-protector failure routines with stack protection
>  # makes no sense.
> diff --git a/debug/stack_chk_fail_local.c b/debug/stack_chk_fail_local.c
> deleted file mode 100644
> index eb0a759..0000000
> --- a/debug/stack_chk_fail_local.c
> +++ /dev/null
> @@ -1,46 +0,0 @@
> -/* Copyright (C) 2005-2017 Free Software Foundation, Inc.
> -   This file is part of the GNU C Library.
> -
> -   The GNU C Library is free software; you can redistribute it and/or
> -   modify it under the terms of the GNU Lesser General Public
> -   License as published by the Free Software Foundation; either
> -   version 2.1 of the License, or (at your option) any later version.
> -
> -   In addition to the permissions in the GNU Lesser General Public
> -   License, the Free Software Foundation gives you unlimited
> -   permission to link the compiled version of this file with other
> -   programs, and to distribute those programs without any restriction
> -   coming from the use of this file. (The GNU Lesser General Public
> -   License restrictions do apply in other respects; for example, they
> -   cover modification of the file, and distribution when not linked
> -   into another program.)
> -
> -   Note that people who make modified versions of this file are not
> -   obligated to grant this special exception for their modified
> -   versions; it is their choice whether to do so. The GNU Lesser
> -   General Public License gives permission to release a modified
> -   version without this exception; this exception also makes it
> -   possible to release a modified version which carries forward this
> -   exception.
> -
> -   The GNU C Library is distributed in the hope that it will be useful,
> -   but WITHOUT ANY WARRANTY; without even the implied warranty of
> -   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -   Lesser General Public License for more details.
> -
> -   You should have received a copy of the GNU Lesser General Public
> -   License along with the GNU C Library; if not, see
> -   <http://www.gnu.org/licenses/>.  */
> -
> -#include <sys/cdefs.h>
> -
> -extern void __stack_chk_fail (void) __attribute__ ((noreturn));
> -
> -/* On some architectures, this helps needless PIC pointer setup
> -   that would be needed just for the __stack_chk_fail call.  */
> -
> -void __attribute__ ((noreturn)) attribute_hidden
> -__stack_chk_fail_local (void)
> -{
> -  __stack_chk_fail ();
> -}
> diff --git a/elf/Makefile b/elf/Makefile
> index e758a4c..fb2d855 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -369,6 +369,7 @@ tests-internal += tst-_dl_addr_inside_object
>  tests-pie += tst-_dl_addr_inside_object
>  $(objpfx)tst-_dl_addr_inside_object: $(objpfx)dl-addr-obj.os
>  CFLAGS-tst-_dl_addr_inside_object.c += $(PIE-ccflag)
> +LDFLAGS-tst-_dl_addr_inside_object = $(dummy-stack-chk-fail)
>  endif
>
>  # By default tst-linkall-static should try to use crypt routines to test
> --
> 2.9.4
>

This patch is incorrect.  A testcase is missing to catch the error.


-- 
H.J.

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

[Nick Alcock]

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...

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

[H.J. Lu]

[-- 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

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

[H.J. Lu]

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

-- 
H.J.

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

[H.J. Lu]

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

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.


-- 
H.J.

[-- Attachment #2: 0001-Don-t-add-stack_chk_fail_local.o-to-libc.a-BZ-21740.patch --]
[-- Type: text/x-patch, Size: 1899 bytes --]

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

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

[Nick Alcock]

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.

-- 
NULL && (void)

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

[H.J. Lu]

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.


-- 
H.J.