[patch] Fix for BZ 16381 -- explicit loader invocation "ld.so ./a.out" on a PIE binary calls global ctors twice
Carlos O'Donell
carlos@redhat.com
Wed Mar 5 00:01:00 GMT 2014
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.
More information about the Libc-alpha
mailing list