Re: [PATCH] elf: Self-dlopen failure with explict loader invocation [BZ #24900]

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

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.