Re: [PATCH 1/3] elf: Allow dlopen of filter object to work [BZ #16272]

[Florian Weimer <fweimer at redhat dot com>]

* David Kilroy:

> diff --git a/elf/Makefile b/elf/Makefile
> index 8f96299..55bed87 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -192,7 +192,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
>  	 tst-unwind-ctor tst-unwind-main tst-audit13 \
> -	 tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-aout
> +	 tst-sonamemove-link tst-sonamemove-dlopen tst-dlopen-aout \
> +	 tst-filterobj tst-filterobj-dlopen
>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \
> @@ -279,7 +280,9 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
>  		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
>  		tst-audit13mod1 tst-sonamemove-linkmod1 \
> -		tst-sonamemove-runmod1 tst-sonamemove-runmod2
> +		tst-sonamemove-runmod1 tst-sonamemove-runmod2 \
> +		tst-filterobj-flt tst-filterobj-lib
> +

This needs rebasing to current master.  Unfortunately, I've got various
patches in flight which conflict.

>  # Most modules build with _ISOMAC defined, but those filtered out
>  # depend on internal headers.
>  modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
> @@ -1553,3 +1556,8 @@ $(objpfx)tst-big-note-lib.so: $(objpfx)tst-big-note-lib.o
>  $(objpfx)tst-unwind-ctor: $(objpfx)tst-unwind-ctor-lib.so
>  
>  CFLAGS-tst-unwind-main.c += -funwind-tables -DUSE_PTHREADS=0
> +
> +LDFLAGS-tst-filterobj-flt.so=-Wl,--filter=$(objpfx)tst-filterobj-lib.so
> +CFLAGS-tst-filterobj-dlopen.c += -DPFX=\"$(objpfx)\"
> +$(objpfx)tst-filterobj: $(objpfx)tst-filterobj-flt.so | $(objpfx)tst-filterobj-lib.so
> +$(objpfx)tst-filterobj-dlopen: $(libdl) | $(objpfx)tst-filterobj-lib.so

Please use spaces around operators.  To add a run-time dependency to
tst-filterobj-dlopen, use something like this, please:

$(objpfx)tst-filterobj-dlopen.out : $(objpfx)tst-filterobj-lib.so

This way, the test is re-run if tst-filterobj-lib.so changed.

See below for PFX.

> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index c29b988..c177cef 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -550,13 +550,14 @@ Filters not supported with LD_TRACE_PRELINKING"));
>      }
>  
>    /* Maybe we can remove some relocation dependencies now.  */
> -  assert (map->l_searchlist.r_list[0] == map);
>    struct link_map_reldeps *l_reldeps = NULL;
>    if (map->l_reldeps != NULL)
>      {
> -      for (i = 1; i < nlist; ++i)
> +      for (i = 0; i < nlist; ++i)
>  	map->l_searchlist.r_list[i]->l_reserved = 1;
>  
> +      /* Avoid removing relocation dependencies of the main binary.  */
> +      map->l_reserved = 0;
>        struct link_map **list = &map->l_reldeps->list[0];
>        for (i = 0; i < map->l_reldeps->act; ++i)
>  	if (list[i]->l_reserved)
> @@ -581,16 +582,32 @@ Filters not supported with LD_TRACE_PRELINKING"));
>  	      }
>  	  }
>  
> -      for (i = 1; i < nlist; ++i)
> +      for (i = 0; i < nlist; ++i)
>  	map->l_searchlist.r_list[i]->l_reserved = 0;
>      }
>  
> -  /* Sort the initializer list to take dependencies into account.  The binary
> -     itself will always be initialize last.  */
> -  memcpy (l_initfini, map->l_searchlist.r_list,
> -	  nlist * sizeof (struct link_map *));
> -  /* We can skip looking for the binary itself which is at the front of
> -     the search list.  */
> +  /* Sort the initializer list to take dependencies into account.  Always
> +     initialize the binary itself last.  First, find it in the search list.  */
> +  for (i = 0; i < nlist; ++i)
> +    if (map->l_searchlist.r_list[i] == map)
> +      break;
> +  assert(i < nlist);

Missing space after assert.

> +  if (i > 0)
> +    {
> +      /* Copy the binary into position 0.  */
> +      memcpy (l_initfini, &map->l_searchlist.r_list[i],
> +	      sizeof (struct link_map *));
> +      /* Copy the filtees.  */
> +      memcpy (&l_initfini[1], map->l_searchlist.r_list,
> +	      i * sizeof (struct link_map *));
> +      /* Copy the remainder.  */
> +      memcpy (&l_initfini[i+1], &map->l_searchlist.r_list[i+1],
> +	      (nlist-i-1) * sizeof (struct link_map *));
> +    }
> +  else
> +    memcpy (l_initfini, map->l_searchlist.r_list,
> +	    nlist * sizeof (struct link_map *));
> +
>    _dl_sort_maps (&l_initfini[1], nlist - 1, NULL, false);
>  
>    /* Terminate the list of dependencies.  */

These changes look reasonable to me.  But please use spaces around
operators.

(Not sure if you want to commit this yourself; if not, I can clean up
such things for you prior to committing.)

> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index a9fd4cb..7fcfdc0 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -305,22 +305,25 @@ dl_open_worker (void *a)
>       allows IFUNC relocations to work and it also means copy
>       relocation of dependencies are if necessary overwritten.  */
>    unsigned int nmaps = 0;
> -  struct link_map *l = new;
> +  unsigned int j = 0;
> +  struct link_map *l =  new->l_initfini[0];

Extra space before new.

>    do
>      {
>        if (! l->l_real->l_relocated)
>  	++nmaps;
> -      l = l->l_next;
> +      l = new->l_initfini[++j];
>      }
>    while (l != NULL);
> +  /* Stack allocation is limited by the number of loaded objects.  */
>    struct link_map *maps[nmaps];
>    nmaps = 0;
> -  l = new;
> +  j = 0;
> +  l = new->l_initfini[0];
>    do
>      {
>        if (! l->l_real->l_relocated)
>  	maps[nmaps++] = l;
> -      l = l->l_next;
> +      l = new->l_initfini[++j];
>      }
>    while (l != NULL);
>    _dl_sort_maps (maps, nmaps, NULL, false);

I have much more trouble ascertaining whether this change is correct.
Are we certain that new->l_initfini is not a subset of the maps that
have been loaded?

> diff --git a/elf/tst-filterobj-dlopen.c b/elf/tst-filterobj-dlopen.c
> new file mode 100644
> index 0000000..7c24a1d
> --- /dev/null
> +++ b/elf/tst-filterobj-dlopen.c

> +int main (int argc, const char**argv)
> +{
> +  void *lib = dlopen (PFX "tst-filterobj-flt.so", RTLD_LAZY);

I think you should use xasprintf and support_objdir_root from
<support/support.h> to construct the path.  Alternatively, you can drop
this completely because it will be on the library search path set up by
the test framework.

> +  int result = 1;
> +
> +  if (lib != NULL)
> +    {
> +      char *(*fn)(void) = dlsym (lib, "get_text");
> +
> +      if (fn != NULL)
> +        {
> +	  const char* text = fn ();
> +
> +	  printf ("%s\n", text);
> +
> +	  /* Verify the text matches what we expect from the filtee */
> +	  result = strcmp (text, "Hello from filtee (PASS)") != 0;
> +        }
> +      else
> +        {
> +	  fprintf (stderr, "No function\n");
> +        }
> +    }
> +  else
> +    {
> +      fprintf (stderr, "No library\n");
> +    }
> +
> +  return result;
> +}

Please use the test framework (<support/test-driver.c>,
<support/check.h>, <support/xdlfcn.h>) and TEST_COMPARE_STRING.
Tests should write to standard output only.

> diff --git a/elf/tst-filterobj-flt.c b/elf/tst-filterobj-flt.c
> new file mode 100644
> index 0000000..ec84d4c
> --- /dev/null
> +++ b/elf/tst-filterobj-flt.c

> +const char *get_text(void)

Missing space before parenthesis.


> diff --git a/elf/tst-filterobj.c b/elf/tst-filterobj.c
> new file mode 100644
> index 0000000..99d50fb
> --- /dev/null
> +++ b/elf/tst-filterobj.c

> +int main (int argc, const char**argv)
> +{
> +  const char* text = get_text ();
> +
> +  printf ("%s\n", text);
> +
> +  /* Verify the text matches what we expect from the filtee */
> +  return strcmp (text, "Hello from filtee (PASS)") != 0;
> +}

Likewise, please use the test framework.

Thanks,
Florian