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/17/2014 06:36 PM, Paul Pluzhnikov wrote:
> On Thu, Jan 16, 2014 at 8:59 AM, Carlos O'Donell <carlos@redhat.com> wrote:
> 
> Attached is a second version of the patch, modified to address Carlos'
> comments.
> 
> This is a fix for PR 16381 -- "ld.so ./a.out" on PIE binary calls global
> constructors twice.
> 
> The reason for the bug is that a.out was loaded as lt_library when using
> "ld.so a.out", but as lt_executable when using "a.out" directly (contrast
> the call to _dl_new_object() from dl_main in rtld.c for "normal" invocation
> (the kernel has already mapped a.out in) with the call to _dl_map_object()
> for the loader-prefixed invocation.
> 
> When lt_executable is used, the code in call_init() correctly skips
> initialization, since the crt0.o will perform it (again).
> 
> The reason this worked for non-PIE binaries is that _dl_map_object_from_fd
> re-set l_type to lt_executable when it discovered that it just loaded
> ET_EXEC.
> 
> This patch simply sets correct type for loading of the executable from
> the start.
> 
> Tested on Linux / x86_64 with no regressions.

OK to checkin if you fix the minor nit and explain my
question in the middle of the patch.

> glibc-PR16381-2014017.txt

>2014-01-17  Paul Pluzhnikov  <ppluzhnikov@google.com>
>
>	* elf/Makefile: Add tst-pie2

Slightly more detailed please:
	* elf/Makefile (tests): Add tst-pie2.
	(tests-pie): Add tst-pie2.


>	* elf/tst-pie2.c: New file.
>	* elf/dl-load.c (_dl_map_object_from_fd): Assert correct l_type
>	for ET_EXEC.
>	* elf/rtld.c (map_doit): Load executable as lt_executable.
>	(dl_main): Likewise.
 
OK.
 
> diff --git a/NEWS b/NEWS
> index da86ee5..93dfff5 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -24,8 +24,8 @@ Version 2.19
>    16074, 16077, 16078, 16103, 16112, 16133, 16143, 16144, 16146, 16150,
>    16151, 16153, 16167, 16172, 16195, 16214, 16245, 16271, 16274, 16283,
>    16289, 16293, 16314, 16316, 16330, 16337, 16338, 16356, 16365, 16366,
> -  16369, 16372, 16375, 16379, 16384, 16385, 16386, 16387, 16390, 16394,
> -  16400, 16407, 16408, 16414, 16430, 16453.
> +  16369, 16372, 16375, 16379, 16381, 16384, 16385, 16386, 16387, 16390,
> +  16394, 16400, 16407, 16408, 16414, 16430, 16453.

OK.

>  
>  * Slovenian translations for glibc messages have been contributed by the
>    Translation Project's Slovenian team of translators.
> 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/dl-load.c b/elf/dl-load.c
> index ee12f32..6f6b344 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1534,8 +1534,8 @@ cannot enable executable stack as shared object requires");
>    /* Signal that we closed the file.  */
>    fd = -1;
>  
> -  if (l->l_type == lt_library && type == ET_EXEC)
> -    l->l_type = lt_executable;
> +  /* If this is ET_EXEC, we should have loaded it as lt_executable.  */
> +  assert (type != ET_EXEC || l->l_type == lt_executable);

OK.

Perfect. This is exactly the code I was looking at in _dl_map_object_from_fd
when I first reviewed this.

I wondered why there were these two nonsense lines that fixed up the l_type.
The assert is good.

>  
>    l->l_entry += l->l_addr;
>  
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 6dcbabc..021b72e 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -623,7 +623,8 @@ static void
>  map_doit (void *a)
>  {
>    struct map_args *args = (struct map_args *) a;
> -  args->map = _dl_map_object (args->loader, args->str, lt_library, 0,
> +  int type = (args->mode == __RTLD_OPENEXEC) ? lt_executable : lt_library;

OK.

This took me a while to review as correct, but I agree that lt_library
is wrong if the mode is known to be __RTLD_OPENEXEC.

> +  args->map = _dl_map_object (args->loader, args->str, type, 0,
>  			      args->mode, LM_ID_BASE);

OK.

>  }
>  
> @@ -1075,7 +1076,7 @@ of this helper program; chances are you did not intend to run this program.\n\
>        else
>  	{
>  	  HP_TIMING_NOW (start);
> -	  _dl_map_object (NULL, rtld_progname, lt_library, 0,
> +	  _dl_map_object (NULL, rtld_progname, lt_executable, 0,

OK, but I'd like you to tell me you verified that the lt_library here
was previously ignored and set to lt_executable by the code in
_dl_map_object_from_fd.

>  			  __RTLD_OPENEXEC, LM_ID_BASE);
>  	  HP_TIMING_NOW (stop);
>  
> diff --git a/elf/tst-pie2.c b/elf/tst-pie2.c
> new file mode 100644
> index 0000000..1da674e
> --- /dev/null
> +++ b/elf/tst-pie2.c
> @@ -0,0 +1,38 @@
> +/* Test case for BZ 16381

Please use "Test case for BZ #16381."

Thanks.

> +
> +   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/>.  */
> +
> +
> +#include <assert.h>
> +
> +static int g;
> +
> +void init_g (void) __attribute__((constructor));
> +
> +void
> +init_g (void)
> +{
> +  assert (g == 0);
> +  g += 1;

OK.

> +}
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  return 0;
> +}

Cheers,
Carlos.