[PATCH] elf: Self-dlopen failure with explict loader invocation [BZ #24900]
Carlos O'Donell
carlos@redhat.com
Thu Aug 15 16:10:00 GMT 2019
On 8/15/19 8:38 AM, Florian Weimer wrote:
> * Carlos O'Donell:
>
>> On 8/12/19 3:15 PM, Florian Weimer wrote:
>>> In case of an explicit loader invocation, ld.so essentially performs
>>> a dlopen call to load the main executable. Since the pathname of
>>> the executable is known at this point, it gets stored in the link
>>> map. In regular mode, the pathname is not known and "" is used
>>> instead.
>>>
>>> As a result, if a program calls dlopen on the pathname of the main
>>> program, the dlopen call succeeds and returns a handle for the main
>>> map. This results in an unnecessary difference between glibc
>>> testing (without --enable-hardcoded-path-in-tests) and production
>>> usage.
>>>
>>> This commit discards the names when building the link map in
>>> _dl_new_object for the main executable, but it still determines
>>> the origin at this point in case of an explict loader invocation.
>>> The reason is that the specified pathname has to be used; the kernel
>>> has a different notion of the main executable.
>>>
>>> Tested on x86-64 with and without --enable-hardcoded-path-in-tests.
>>
>> Thanks for v2 of this patch.
>>
>> Overall I think harmonizing the linker behaviour has long-term benefit,
>> so I approve of the approach, much in the vein of static vs. dynamic tests.
>>
>> If the strategy is to make direct linker invocation the same as non-direct
>> linker invocation then you need to test both:
>>
>> (a) Normal test with the test framework.
>> (b) Test-in-container test running the binary directly.
>>
>> Could you add the test-in-container test that runs the same test but
>> in the container directly under the linker in question?
>
> Here's a new patch with another test.
>
> Thanks,
> Florian
>
> elf: Self-dlopen failure with explict loader invocation [BZ #24900]
>
> In case of an explicit loader invocation, ld.so essentially performs
> a dlopen call to load the main executable. Since the pathname of
> the executable is known at this point, it gets stored in the link
> map. In regular mode, the pathname is not known and "" is used
> instead.
>
> As a result, if a program calls dlopen on the pathname of the main
> program, the dlopen call succeeds and returns a handle for the main
> map. This results in an unnecessary difference between glibc
> testing (without --enable-hardcoded-path-in-tests) and production
> usage.
>
> This commit discards the names when building the link map in
> _dl_new_object for the main executable, but it still determines
> the origin at this point in case of an explict loader invocation.
> The reason is that the specified pathname has to be used; the kernel
> has a different notion of the main executable.
>
OK for master.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 2019-08-15 Florian Weimer <fweimer@redhat.com>
>
> [BZ #24900]
> * elf/dl-object.c (_dl_new_object): Do not store pathnames in the
> new object in __RTLD_OPENEXEC mode (except for the origin).
> * elf/tst-dlopen-aout.c (check_dlopen_failure): New function with
> check for the error message.
> (do_test): Call it. Add check using relative path.
> * elf/Makefile (tests-container): Add tst-dlopen-aout-container.
> (tst-dlopen-aout-container): Link with libpthread.
> (LDFLAGS-tst-dlopen-aout-container): Set RPATH to $ORIGIN.
>
> diff --git a/elf/Makefile b/elf/Makefile
> index e8c3458963..d470e41402 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -199,7 +199,7 @@ tests-internal += loadtest unload unload2 circleload1 \
> tst-tls3 tst-tls6 tst-tls7 tst-tls8 tst-dlmopen2 \
> tst-ptrguard1 tst-stackguard1 tst-libc_dlvsym \
> tst-create_format1
> -tests-container += tst-pldd
> +tests-container += tst-pldd tst-dlopen-aout-container
OK.
> test-srcs = tst-pathopt
> selinux-enabled := $(shell cat /selinux/enforce 2> /dev/null)
> ifneq ($(selinux-enabled),1)
> @@ -1268,6 +1268,8 @@ $(objpfx)tst-addr1: $(libdl)
>
> $(objpfx)tst-thrlock: $(libdl) $(shared-thread-library)
> $(objpfx)tst-dlopen-aout: $(libdl) $(shared-thread-library)
> +$(objpfx)tst-dlopen-aout-container: $(libdl) $(shared-thread-library)
> +LDFLAGS-tst-dlopen-aout-container += -Wl,-rpath,\$$ORIGIN
OK. Use DST of $ORIGIN allows the binary to be found.
>
> CFLAGS-ifuncmain1pic.c += $(pic-ccflag)
> CFLAGS-ifuncmain1picstatic.c += $(pic-ccflag)
> diff --git a/elf/dl-object.c b/elf/dl-object.c
> index c3f5455ab4..e9520583cf 100644
> --- a/elf/dl-object.c
> +++ b/elf/dl-object.c
> @@ -57,14 +57,30 @@ struct link_map *
> _dl_new_object (char *realname, const char *libname, int type,
> struct link_map *loader, int mode, Lmid_t nsid)
> {
> +#ifdef SHARED
> + unsigned int naudit;
> + if (__glibc_unlikely ((mode & __RTLD_OPENEXEC) != 0))
> + {
> + assert (type == lt_executable);
> + assert (nsid == LM_ID_BASE);
> +
> + /* Ignore the specified libname for the main executable. It is
> + only known with an explicit loader invocation. */
> + libname = "";
> +
> + /* We create the map for the executable before we know whether
> + we have auditing libraries and if yes, how many. Assume the
> + worst. */
> + naudit = DL_NNS;
> + }
> + else
> + naudit = GLRO (dl_naudit);
> +#endif
OK.
> +
> size_t libname_len = strlen (libname) + 1;
> struct link_map *new;
> struct libname_list *newname;
> #ifdef SHARED
> - /* We create the map for the executable before we know whether we have
> - auditing libraries and if yes, how many. Assume the worst. */
> - unsigned int naudit = GLRO(dl_naudit) ?: ((mode & __RTLD_OPENEXEC)
> - ? DL_NNS : 0);
> size_t audit_space = naudit * sizeof (new->l_audit[0]);
> #else
> # define audit_space 0
> @@ -91,8 +107,20 @@ _dl_new_object (char *realname, const char *libname, int type,
> and won't get dumped during core file generation. Therefore to assist
> gdb and to create more self-contained core files we adjust l_name to
> point at the newly allocated copy (which will get dumped) instead of
> - the ld.so rodata copy. */
> - new->l_name = *realname ? realname : (char *) newname->name + libname_len - 1;
> + the ld.so rodata copy.
> +
> + Furthermore, in case of explicit loader invocation, discard the
> + name of the main executable, to match the regular behavior, where
> + name of the executable is not known. */
> +#ifdef SHARED
> + if (*realname != '\0' && (mode & __RTLD_OPENEXEC) == 0)
> +#else
> + if (*realname != '\0')
> +#endif
> + new->l_name = realname;
> + else
> + new->l_name = (char *) newname->name + libname_len - 1;
> +
OK.
> new->l_type = type;
> /* If we set the bit now since we know it is never used we avoid
> dirtying the cache line later. */
> @@ -149,7 +177,14 @@ _dl_new_object (char *realname, const char *libname, int type,
>
> new->l_local_scope[0] = &new->l_searchlist;
>
> - /* Don't try to find the origin for the main map which has the name "". */
> + /* Determine the origin. If allocating the link map for the main
> + executable, the realname is not known and "". In this case, the
> + origin needs to be determined by other means. However, in case
> + of an explicit loader invocation, the pathname of the main
> + executable is known and needs to be processed here: From the
> + point of view of the kernel, the main executable is the
> + dynamic loader, and this would lead to a computation of the wrong
> + origin. */
OK.
> if (realname[0] != '\0')
> {
> size_t realname_len = strlen (realname) + 1;
> diff --git a/elf/tst-dlopen-aout-container.c b/elf/tst-dlopen-aout-container.c
> new file mode 100644
> index 0000000000..9b9f86133d
> --- /dev/null
> +++ b/elf/tst-dlopen-aout-container.c
> @@ -0,0 +1,19 @@
> +/* Test case for BZ #16634 and BZ#24900. Container version.
> + Copyright (C) 2014-2019 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 "tst-dlopen-aout.c"
OK. Perfect, exactly what I was thinking.
> diff --git a/elf/tst-dlopen-aout.c b/elf/tst-dlopen-aout.c
> index 25cfe2f740..3e39fc6067 100644
> --- a/elf/tst-dlopen-aout.c
> +++ b/elf/tst-dlopen-aout.c
> @@ -1,7 +1,8 @@
> -/* Test case for BZ #16634.
> +/* Test case for BZ #16634 and BZ#24900.
>
> Verify that incorrectly dlopen()ing an executable without
> - __RTLD_OPENEXEC does not cause assertion in ld.so.
> + __RTLD_OPENEXEC does not cause assertion in ld.so, and that it
> + actually results in an error.
OK.
>
> Copyright (C) 2014-2019 Free Software Foundation, Inc.
> This file is part of the GNU C Library.
> @@ -24,6 +25,8 @@
> #include <pthread.h>
> #include <stdio.h>
> #include <stdlib.h>
> +#include <string.h>
> +#include <support/check.h>
OK.
> #include <support/support.h>
> #include <support/xthread.h>
>
> @@ -35,28 +38,35 @@ fn (void *p)
> return p;
> }
>
> +/* Call dlopen on PATH and check that fails with an error message
> + indicating an attempt to open an ET_EXEC or PIE object. */
> +static void
> +check_dlopen_failure (const char *path)
> +{
> + void *handle = dlopen (path, RTLD_LAZY);
> + if (handle != NULL)
> + FAIL_EXIT1 ("dlopen succeeded unexpectedly: %s", path);
> +
> + const char *message = dlerror ();
> + TEST_VERIFY_EXIT (message != NULL);
> + if ((strstr (message,
> + "cannot dynamically load position-independent executable")
> + == NULL)
> + && strstr (message, "cannot dynamically load executable") == NULL)
> + FAIL_EXIT1 ("invalid dlopen error message: \"%s\"", message);
> +}
OK.
> +
> static int
> do_test (int argc, char *argv[])
> {
> int j;
>
> - /* Use the full path so that the dynamic loader does not recognize
> - the main program as already loaded (even with an explicit ld.so
> - invocation). */
> - char *path = xasprintf ("%s/%s", support_objdir_root, "tst-dlopen-aout");
> - printf ("info: dlopen object: %s\n", path);
> -
> for (j = 0; j < 100; ++j)
> {
> pthread_t thr;
> - void *p;
>
> - p = dlopen (path, RTLD_LAZY);
> - if (p != NULL)
> - {
> - puts ("error: dlopen succeeded unexpectedly");
> - return 1;
> - }
> + check_dlopen_failure (argv[0]);
> +
> /* We create threads to force TLS allocation, which triggers
> the original bug i.e. running out of surplus slotinfo entries
> for TLS. */
> @@ -64,7 +74,10 @@ do_test (int argc, char *argv[])
> xpthread_join (thr);
> }
>
> - free (path);
> + /* The elf subdirectory (or $ORIGIN in the container case) is on the
> + library search path. */
> + check_dlopen_failure ("tst-dlopen-aout");
OK.
> +
> return 0;
> }
>
>
--
Cheers,
Carlos.
More information about the Libc-alpha
mailing list