[PATCH] elf: Self-dlopen failure with explict loader invocation [BZ #24900]
Carlos O'Donell
carlos@redhat.com
Thu Aug 15 11:47:00 GMT 2019
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?
> 2019-08-12 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.
>
> 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 = "";
OK.
> +
> + /* 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
> +
> 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. */
OK. I like this behaviour since it avoids a difference in the two cases.
> +#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. */
> if (realname[0] != '\0')
> {
> size_t realname_len = strlen (realname) + 1;
> diff --git a/elf/tst-dlopen-aout.c b/elf/tst-dlopen-aout.c
> index 25cfe2f740..a799fbc2cc 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#24899.
>
> 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.
>
> 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>
> #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]);
> +
OK. Refactor.
> /* 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,9 @@ do_test (int argc, char *argv[])
> xpthread_join (thr);
> }
>
> - free (path);
> + /* The elf subdirectory 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