Re: [patch] Fix for BZ 16381 -- explicit loader invocation "ld.so ./a.out" on a PIE binary calls global ctors twice

["Carlos O'Donell" <carlos at redhat dot com>]

On 01/11/2014 10:45 PM, Paul Pluzhnikov wrote:
> Greetings,
> 
> This patch fixes BZ 16381 -- explicit loader invocation "ld.so ./a.out" on a PIE binary calls global ctors twice.

That's terrible. Thanks for the patch and regression test.

However, your submission lacks a complete description of
why your change is correct and why it fixes the problem.
Please repost with a detailed description of "why" the
patch is correct. Please also outline the location where
the conditional triggers a double run of the constructors.

> Thanks,
> 
> 2014-01-11  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 

Add BZ marker. Don't forget to update NEWS please.

>     * elf/Makefile: Add tst-pie2
>     * elf/tst-pie2.c: New file.
>     * elf/rtld.c (dl_main): Mark main executable as such.
> 
> 
> glibc-PR16381-2014011.txt
> 
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 4c58fc9..055ea38 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -214,8 +214,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-array5dep tst-null-argv-lib
>  ifeq (yesyes,$(have-fpie)$(build-shared))
>  modules-names += tst-piemod1
> -tests += tst-pie1
> -tests-pie += tst-pie1
> +tests += tst-pie1 tst-pie2
> +tests-pie += tst-pie1 tst-pie2

OK.

>  endif
>  modules-execstack-yes = tst-execstack-mod
>  extra-test-objs += $(addsuffix .os,$(strip $(modules-names)))
> @@ -882,6 +882,7 @@ $(objpfx)tst-array5-static.out: tst-array5-static.exp \
>  	cmp $@ tst-array5-static.exp > /dev/null
>  
>  CFLAGS-tst-pie1.c += $(pie-ccflag)
> +CFLAGS-tst-pie2.c += $(pie-ccflag)

OK.

>  
>  $(objpfx)tst-pie1: $(objpfx)tst-piemod1.so
>  
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 6dcbabc..3e0bae4 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1101,6 +1101,9 @@ of this helper program; chances are you did not intend to run this program.\n\
>  	 implementations which has no real free() function it does not
>  	 makes sense to free the old string first.  */
>        main_map->l_name = (char *) "";
> +      /* l_type is already lt_executable for ET_EXEC, but not for a PIE
> +         binary, which is ET_DYN.  */
> +      main_map->l_type = lt_executable;

What is it for ET_DYN? Unset? 0?

I agree that this looks like the correct fix, but I'd like to know
what conditionals are triggered by this, since it looks like you've
already done the work of finding them.

>        *user_entry = main_map->l_entry;
>  
>  #ifdef HAVE_AUX_VECTOR
> diff --git a/elf/tst-pie2.c b/elf/tst-pie2.c
> new file mode 100644
> index 0000000..14cbf93
> --- /dev/null
> +++ b/elf/tst-pie2.c
> @@ -0,0 +1,37 @@

First line in the test case should say what it is.

> +/* Copyright (C) 2014 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.
> +
> +   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/>.  */
> +
> +/* Test case for BZ 16381  */

Move this to the first line please.

> +
> +#include <assert.h>
> +
> +static int g;
> +
> +void init_g (void) __attribute__((constructor));
> +
> +void
> +init_g (void)
> +{
> +  assert (g == 0);
> +  g += 1;
> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  return 0;
> +}

Cheers,
Carlos.