Remove debug/stack_chk_fail_local.c [BZ #21740]

  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
  

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.
  

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