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

Message ID 4eae391688a6e42b0b75467de265c122e6402668.1571301957.git.david.kilroy@arm.com
State Superseded
Headers

Commit Message

David Kilroy Oct. 17, 2019, 10:33 a.m. UTC
  _dl_map_object_deps assumes that map will always be at the beginning
of the search list in l_searchlist.r_list[]. This is not true if map
is a filter object, in which case the filtees are inserted before map.

Fix this by:

* avoiding relocation dependencies of map by setting l_reserved to 0
  and otherwise processing the rest of the search list.

* ensuring that map remains at the beginning of l_initfini - the list
  of things that need initialisation (and destruction). Do this by
  splitting the copy up. This may not be required, but matches the
  initialization order without dlopen.

With that in place, the filtee objects are not getting relocations
applied when dlopen is called. Modify dl_open_worker to attempt
relocations on new->l_initfini instead of objects referenced by
new->l_next (the filtees are in l_prev).

Add tests to verify that symbols resolve to the filtee implementation
when filter objects are used, both as a normal link and when dlopen'd.

Tested by running the testsuite on x86_64.
---
 elf/Makefile               | 12 +++++++++--
 elf/dl-deps.c              | 35 ++++++++++++++++++++++--------
 elf/dl-open.c              | 11 ++++++----
 elf/tst-filterobj-dlopen.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
 elf/tst-filterobj-flt.c    | 24 +++++++++++++++++++++
 elf/tst-filterobj-lib.c    | 24 +++++++++++++++++++++
 elf/tst-filterobj-lib.h    | 18 ++++++++++++++++
 elf/tst-filterobj.c        | 34 +++++++++++++++++++++++++++++
 8 files changed, 197 insertions(+), 15 deletions(-)
 create mode 100644 elf/tst-filterobj-dlopen.c
 create mode 100644 elf/tst-filterobj-flt.c
 create mode 100644 elf/tst-filterobj-lib.c
 create mode 100644 elf/tst-filterobj-lib.h
 create mode 100644 elf/tst-filterobj.c
  

Comments

Florian Weimer Oct. 18, 2019, 11:43 a.m. UTC | #1
* 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
  
David Kilroy Oct. 18, 2019, 3:09 p.m. UTC | #2
Hi Florian,

Thanks for the detailed review. Apologies for the various formatting issues,
I haven't got used the GNU style yet. I'll attempt to correct this in the
next submission.

From: Florian Weimer <fweimer@redhat.com>
>* David Kilroy:
>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 8f96299..55bed87 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -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.

Will do. I think the main conflicts are in the Makefile where we are both
adding tests. I've seen that you're also touching dl_open_worker, but
so far it looks like the changes are in different areas.

>> +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.

Well, spotted! Will do.

>> 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

>These changes look reasonable to me.

That's good to know. I tried a few different things before I realised I needed
to fix up the relocations as well.

>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.)

I doubt I have commit access (this is my first submission), but I will attempt
to fix all formatting issues.

>> 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];
>>    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?

That is something I'm not sure about (yet). I'll look into this further. My
reasoning for switching to using l_initfini was that the filtee's constructors
were crashing, and clearly anything that needed initialisation ought to be
relocated first. If I can't confirm whether l_initfini is a subset, there are a
couple alternatives here:

* I can verify that the filter is in l->l_prev, and walk that before
  constructing map via l->l_next.

* after relocating via l->l_next, check l_initfini for any un-relocated
  objects.

Both of these would invalidate the following patches.

>> 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.

So PFX ($(objpfx) in the makefile) is just a path that will be on the
library search path? I assumed it might be a file prefix too. This is
something I copied from an existing dlopen test (I don't have the code
to hand to double check this).

>> +       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");
>
>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.

Will do.



Regards,

Dave.
  
Florian Weimer Oct. 21, 2019, 11:27 a.m. UTC | #3
* David Kilroy:

[Changes related to inserting the filtee before the filter]

>>These changes look reasonable to me.
>
> That's good to know. I tried a few different things before I realised I needed
> to fix up the relocations as well.

>>> 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];
>>>    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?
>
> That is something I'm not sure about (yet). I'll look into this further. My
> reasoning for switching to using l_initfini was that the filtee's constructors
> were crashing, and clearly anything that needed initialisation ought to be
> relocated first.

In an off-list discussion, Carlos also had some concerns about the state
of filter support.  Based on the Solaris documentation,

  <https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter4-4.html>

it seems to me that filters are mostly a link editor feature.  Sorting
the filtee prior to the filter appears to be the right thing to do.
However, what seems to be missing currently is ignoring the filter
during symbol binding.  I think if the filter contains symbols that are
not present in the filtee, it is possible to bind them.  That does not
seem to match the documented Solaris behavior.

The Solaris documentation does not mention symbol filtering
(constraining the set of exported symbols) at run time.  This kind of
filtering is achieved at run time because the link editor only looks at
symbols in the filter and does not consult the filtee.

On GNU, I think we support this via a linker script which specifies a
link-time object with a link-time ABI (typically a subset of the
run-time ABI).  Therefore, filter support is redundant.

David, have you ever used filters on Solaris?  Can you comment on the
expected behavior for them?

Thanks,
Florian
  
David Kilroy Oct. 21, 2019, 2:11 p.m. UTC | #4
> Florian Weimer
> * David Kilroy:
> 
> [Changes related to inserting the filtee before the filter]

> David, have you ever used filters on Solaris?  Can you comment on the
> expected behavior for them?

Unfortunately, I don't have access to Solaris.

I've commented below.

> >>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?
> >
> > That is something I'm not sure about (yet). I'll look into this
> > further. My reasoning for switching to using l_initfini was that the
> > filtee's constructors were crashing, and clearly anything that needed
> > initialisation ought to be relocated first.
> 
> In an off-list discussion, Carlos also had some concerns about the state of
> filter support.  Based on the Solaris documentation,
> 
>   <https://docs.oracle.com/cd/E23824_01/html/819-0690/chapter4-4.html>
> 
> it seems to me that filters are mostly a link editor feature.  Sorting the filtee
> prior to the filter appears to be the right thing to do.

This is the documentation I've been looking at for filter objects.

> However, what seems to be missing currently is ignoring the filter during
> symbol binding.  I think if the filter contains symbols that are not present in
> the filtee, it is possible to bind them.  That does not seem to match the
> documented Solaris behavior.

That's my understanding. To get the Solaris behaviour in this case we could
remove the filter from search (and init) paths - but only for filter objects
though, as auxiliary objects may want to bind some symbols in the filter.

> The Solaris documentation does not mention symbol filtering (constraining
> the set of exported symbols) at run time.  This kind of filtering is achieved at
> run time because the link editor only looks at symbols in the filter and does
> not consult the filtee.

My assumption was that on Solaris only the symbols mentioned in the filter
actually get bound at runtime. I haven't looked at what would have to be done
to support that behaviour. I'm sounds non-trivial :)

This would matter if the filtee exported a symbol that the application actually
wanted to pick up from a library later on the search path. As far as I'm aware
this doesn't occur for us, so the lack of support is not a problem.

> On GNU, I think we support this via a linker script which specifies a link-time
> object with a link-time ABI (typically a subset of the run-time ABI).  Therefore,
> filter support is redundant.

Do you have a link with some description? I've been digging around, and not
come across separately specifying the link-time ABI.

I suspect that this wouldn't work in my use case though. External projects
link against the filter object we produce, and don't know about the filtee
(which is effectively an implementation detail). Different suppliers can also
supply the library (which may or may not be a filter object), so having the
external project link the library differently based on implementation is...
suboptimal.



Regards,

Dave.
  
Florian Weimer Oct. 21, 2019, 2:21 p.m. UTC | #5
* David Kilroy:

>> The Solaris documentation does not mention symbol filtering (constraining
>> the set of exported symbols) at run time.  This kind of filtering is achieved at
>> run time because the link editor only looks at symbols in the filter and does
>> not consult the filtee.
>
> My assumption was that on Solaris only the symbols mentioned in the filter
> actually get bound at runtime. I haven't looked at what would have to be done
> to support that behaviour. I'm sounds non-trivial :)

I expected that based on the name of the feature, but the Solaris
documentation does not support that.  It's only clear that they filter
at link time.

>> On GNU, I think we support this via a linker script which specifies a link-time
>> object with a link-time ABI (typically a subset of the run-time ABI).  Therefore,
>> filter support is redundant.
>
> Do you have a link with some description? I've been digging around, and not
> come across separately specifying the link-time ABI.

Look at the contents for libc.so.  You can reference arbitrary DSOs
there, it doesn't have to be the implementation DSO that is used at run
time.

> I suspect that this wouldn't work in my use case though. External projects
> link against the filter object we produce, and don't know about the filtee
> (which is effectively an implementation detail). Different suppliers can also
> supply the library (which may or may not be a filter object), so having the
> external project link the library differently based on implementation is...
> suboptimal.

If you currently use a symbolic link for the .so link object, it should
be easy enough to replace that with the script.  If the link time DSO is
stored under its soname, then of course this does not work, but you
wouldn't be able to transparently inject the filter either, so that
doesn't seem to be your scenario.

I'm less clear how this all interacts with dlopen.  It seems that you
want a simple alias mechanism for dlopen.  In that case, a simple
symbolic link with the soname should work, and you don't need a separate
filter object, either.

Thanks,
Florian
  
Szabolcs Nagy Oct. 21, 2019, 4:42 p.m. UTC | #6
On 21/10/2019 15:21, Florian Weimer wrote:
> * David Kilroy:
>> I suspect that this wouldn't work in my use case though. External projects
>> link against the filter object we produce, and don't know about the filtee
>> (which is effectively an implementation detail). Different suppliers can also
>> supply the library (which may or may not be a filter object), so having the
>> external project link the library differently based on implementation is...
>> suboptimal.
> 
> If you currently use a symbolic link for the .so link object, it should
> be easy enough to replace that with the script.  If the link time DSO is
> stored under its soname, then of course this does not work, but you
> wouldn't be able to transparently inject the filter either, so that
> doesn't seem to be your scenario.

linker script only works at static link time, not for dlopen.

> I'm less clear how this all interacts with dlopen.  It seems that you
> want a simple alias mechanism for dlopen.  In that case, a simple
> symbolic link with the soname should work, and you don't need a separate
> filter object, either.

the scenario is:

libA.so and libB.so export a set of symbols.  this is abi
and there are multiple providers of libA.so and libB.so.

one provider wants to have a single libinternal.so that
defines all the symbols of libA and libB as they share a
lot of code.

(1) having libA.so and libB.so as "wrapper libraries" around
libinternal.so with RTLD_NEXT would work, but that's less
efficient because of the extra indirection,

(2) the filter library magic would avoid the wrapper.

(3) symlinking libA.so and libB.so to libinternal.so makes
all symbols visible when either of them is loaded, polluting
the link namespace.

(4) creating two copies of libinternal.so with different
set of exported symbols would work just waste memory (assuming
there is no internal global state that needs to be shared)

i think if link namespace pollution is ok then (3) is the best
solution otherwise it's less clear, but probably (4) is most
reliable.

(libinternal.so is a video driver lib and libA, libB, .. are
various opengl libs with fixed abi)
  
David Kilroy Oct. 22, 2019, 8:27 a.m. UTC | #7
> Florian Weimer 
> * David Kilroy:
> 
> >> The Solaris documentation does not mention symbol filtering
> (constraining
> >> the set of exported symbols) at run time.  This kind of filtering is
> achieved at
> >> run time because the link editor only looks at symbols in the filter and
> does
> >> not consult the filtee.
> >
> > My assumption was that on Solaris only the symbols mentioned in the
> filter
> > actually get bound at runtime. I haven't looked at what would have to be
> done
> > to support that behaviour. I'm sounds non-trivial :)
> 
> I expected that based on the name of the feature, but the Solaris
> documentation does not support that.  It's only clear that they filter
> at link time.

[I just wanted to respond to this part of your email here. I'll respond to the
rest via Szabolcs email.]

The web page does say "The runtime linker uses the filtee to resolve any
symbols defined by filter.so.1. If the filtee is not found, or a filter symbol
is not found in the filtee, the filter is skipped for this symbol lookup."



Regards,

Dave.
  
Florian Weimer Oct. 22, 2019, 8:36 a.m. UTC | #8
* David Kilroy:

>> Florian Weimer 
>> * David Kilroy:
>> 
>> >> The Solaris documentation does not mention symbol filtering
>> (constraining
>> >> the set of exported symbols) at run time.  This kind of filtering is
>> achieved at
>> >> run time because the link editor only looks at symbols in the filter and
>> does
>> >> not consult the filtee.
>> >
>> > My assumption was that on Solaris only the symbols mentioned in the
>> filter
>> > actually get bound at runtime. I haven't looked at what would have to be
>> done
>> > to support that behaviour. I'm sounds non-trivial :)
>> 
>> I expected that based on the name of the feature, but the Solaris
>> documentation does not support that.  It's only clear that they filter
>> at link time.
>
> [I just wanted to respond to this part of your email here. I'll
> respond to the rest via Szabolcs email.]
>
> The web page does say "The runtime linker uses the filtee to resolve
> any symbols defined by filter.so.1. If the filtee is not found, or a
> filter symbol is not found in the filtee, the filter is skipped for
> this symbol lookup."

It says “not found in the filtee”.  I think this means that the filter
is never used for symbol lookup (something we do not implement in
glibc), not that the symbol set of the filtee is restricted to that of
the filter (not implemented either).  It is possible that there was a
typo here in and the roles of filter and filtee are swapped in this
documentation paragraph, though.

Thanks,
Florian
  
David Kilroy Oct. 22, 2019, 9:21 a.m. UTC | #9
> Szabolcs Nagy
> 
> On 21/10/2019 15:21, Florian Weimer wrote:
> > * David Kilroy:
> >> I suspect that this wouldn't work in my use case though. External
> >> projects link against the filter object we produce, and don't know
> >> about the filtee (which is effectively an implementation detail).
> >> Different suppliers can also supply the library (which may or may not
> >> be a filter object), so having the external project link the library
> differently based on implementation is...
> >> suboptimal.
> >
> > If you currently use a symbolic link for the .so link object, it
> > should be easy enough to replace that with the script.  If the link
> > time DSO is stored under its soname, then of course this does not
> > work, but you wouldn't be able to transparently inject the filter
> > either, so that doesn't seem to be your scenario.
> 
> linker script only works at static link time, not for dlopen.

This did confuse me. I think you're proposing something like:

Implementation library, libimpl.so
Filter libraries, libapi_a.so, libapi_b.so

libimpl.so       => libimpl.so.2
libimpl.so.2     => libimpl.so.2.3.4
libimpl.so.2.3.4    (the actual implementation, soname set to
                     libimpl.so.2)

libapi_a.so          (stub with api selected using version script,
                      and soname set to libapi_a.so.1)
libapi_a.so.1     => libimpl.so
libapi_a.so.1.2.2 => libimpl.so

libapi_b.so          (stub with api selected using version script,
                      and soname set to libapi_b.so.3)
libapi_b.so.3     => libimpl.so
libapi_b.so.3.4.5 => libimpl.so

I think this works when the application links, and the resulting
applications would run.

In the dlopen case, if you're in control of the applications, you can make
them all open the right link library (libapi_x.so.n). This isn't possible
if there are other implementations of the library that the application can
link against (which may have different versions). In that case the application
must dlopen libapi_x.so, which will get the stub library with this layout.

If this is what you intended, I think my main objection is that it's rather
unexpected...

> > I'm less clear how this all interacts with dlopen.  It seems that you
> > want a simple alias mechanism for dlopen.  In that case, a simple
> > symbolic link with the soname should work, and you don't need a
> > separate filter object, either.
> 
> the scenario is:
> 
> libA.so and libB.so export a set of symbols.  this is abi and there are
> multiple providers of libA.so and libB.so.
> 
> one provider wants to have a single libinternal.so that defines all the
> symbols of libA and libB as they share a lot of code.
> 
> (1) having libA.so and libB.so as "wrapper libraries" around libinternal.so
> with RTLD_NEXT would work, but that's less efficient because of the extra
> indirection,
> 
> (2) the filter library magic would avoid the wrapper.
> 
> (3) symlinking libA.so and libB.so to libinternal.so makes all symbols
> visible when either of them is loaded, polluting the link namespace.
> 
> (4) creating two copies of libinternal.so with different set of exported
> symbols would work just waste memory (assuming there is no internal global
> state that needs to be shared)
> 
> i think if link namespace pollution is ok then (3) is the best solution
> otherwise it's less clear, but probably (4) is most reliable.
>
> (libinternal.so is a video driver lib and libA, libB, .. are various opengl
> libs with fixed abi)

Thanks for the summarising the use case :)

Some potential issues with simply symlinking the libraries:

* The application would only retain a single dependency on the first symlink,
  unless it linked with -Wl,--no-as-needed.
* Not able to independently version the api libraries?

I think the important point for this use case is that we have a large
monolithic library (with global state), which applications expect to access via
multiple libraries. Symlinks were used in the past, and we're considering
both (1) and (2) as alternatives. The other potential solution is not having a
monolithic library.


Regards,

Dave.
  
David Kilroy Oct. 22, 2019, 9:39 a.m. UTC | #10
> Florian Weimer

> * David Kilroy:

> 

> >> Florian Weimer

> >> * David Kilroy:

> >>

> >> >> The Solaris documentation does not mention symbol filtering

> >> (constraining

> >> >> the set of exported symbols) at run time.  This kind of filtering

> >> >> is

> >> achieved at

> >> >> run time because the link editor only looks at symbols in the

> >> >> filter and

> >> does

> >> >> not consult the filtee.

> >> >

> >> > My assumption was that on Solaris only the symbols mentioned in the

> >> filter

> >> > actually get bound at runtime. I haven't looked at what would have

> >> > to be

> >> done

> >> > to support that behaviour. I'm sounds non-trivial :)

> >>

> >> I expected that based on the name of the feature, but the Solaris

> >> documentation does not support that.  It's only clear that they

> >> filter at link time.

> >

> > [I just wanted to respond to this part of your email here. I'll

> > respond to the rest via Szabolcs email.]

> >

> > The web page does say "The runtime linker uses the filtee to resolve

> > any symbols defined by filter.so.1. If the filtee is not found, or a

> > filter symbol is not found in the filtee, the filter is skipped for

> > this symbol lookup."

> 

> It says “not found in the filtee”.  I think this means that the filter is

> never used for symbol lookup (something we do not implement in glibc), not

> that the symbol set of the filtee is restricted to that of the filter (not

> implemented either).  It is possible that there was a typo here in and the

> roles of filter and filtee are swapped in this documentation paragraph,

> though.


It makes sense to me. I agree that the filter is never used for symbol lookup.

filter.so.1, has symbol a(), b() and has DT_FILTER filtee.so.1
filtee.so.1, has symbols a(), c()

The first sentence says that the symbols defined in filter.so.1 would be
resolved from filtee.so.1. So the runtime linker should look for a() and b()
in filtee.so.1.

The second sentence deals with b(), which is not in the filtee. To me it says
that b() shouldn't resolve to the filters implementation, and therefore b()
has to come from later in the search path.

Conversely an auxiliary object _would_ resolve b() to the auxiliary library
implementation. "If this filtee is found, the runtime linker uses the filtee
to resolve any symbols defined by filter.so.1. If the filtee is not found, or
a symbol from the filter is not found in the filtee, then the original symbol
within the filter is used."

I think this text for the auxiliary object is a little clearer.


Regards,

Dave.
  
Florian Weimer Oct. 22, 2019, 9:43 a.m. UTC | #11
* Szabolcs Nagy:

> the scenario is:
>
> libA.so and libB.so export a set of symbols.  this is abi
> and there are multiple providers of libA.so and libB.so.
>
> one provider wants to have a single libinternal.so that
> defines all the symbols of libA and libB as they share a
> lot of code.
>
> (1) having libA.so and libB.so as "wrapper libraries" around
> libinternal.so with RTLD_NEXT would work, but that's less
> efficient because of the extra indirection,

If libA.so and libB.so have libinternal.so as a DT_NEEDED dependency,
you do not need to implement forwarding with RTLD_NEXT.

> (3) symlinking libA.so and libB.so to libinternal.so makes
> all symbols visible when either of them is loaded, polluting
> the link namespace.

Filters do that as well.  There is no actual per-symbol run-time
filtering implemented today, and it is unclear if Solaris implements it.

> (libinternal.so is a video driver lib and libA, libB, .. are
> various opengl libs with fixed abi)

Is there an expectation that libinternal interposes symbols in libA
etc., perhaps with more efficient implementations?  And if not, a
fallback implementation in libA etc. is used?  This is clearly not
supported by the Solaris filter feature (as documented).

Thanks,
Florian
  
Florian Weimer Oct. 22, 2019, 9:49 a.m. UTC | #12
* David Kilroy:

>> Szabolcs Nagy
>> 
>> On 21/10/2019 15:21, Florian Weimer wrote:
>> > * David Kilroy:
>> >> I suspect that this wouldn't work in my use case though. External
>> >> projects link against the filter object we produce, and don't know
>> >> about the filtee (which is effectively an implementation detail).
>> >> Different suppliers can also supply the library (which may or may not
>> >> be a filter object), so having the external project link the library
>> differently based on implementation is...
>> >> suboptimal.
>> >
>> > If you currently use a symbolic link for the .so link object, it
>> > should be easy enough to replace that with the script.  If the link
>> > time DSO is stored under its soname, then of course this does not
>> > work, but you wouldn't be able to transparently inject the filter
>> > either, so that doesn't seem to be your scenario.
>> 
>> linker script only works at static link time, not for dlopen.
>
> This did confuse me. I think you're proposing something like:
>
> Implementation library, libimpl.so
> Filter libraries, libapi_a.so, libapi_b.so
>
> libimpl.so       => libimpl.so.2

This symbolic link should not be there if there is no expectation that
applications link directly with -limpl.

> libimpl.so.2     => libimpl.so.2.3.4
> libimpl.so.2.3.4    (the actual implementation, soname set to
>                      libimpl.so.2)
>
> libapi_a.so          (stub with api selected using version script,
>                       and soname set to libapi_a.so.1)
> libapi_a.so.1     => libimpl.so
> libapi_a.so.1.2.2 => libimpl.so

The two symbolic links above should probably go straight to
libimpl.so.2.3.4 at run time, so that libimpl.so can be made optional.

> libapi_b.so          (stub with api selected using version script,
>                       and soname set to libapi_b.so.3)
> libapi_b.so.3     => libimpl.so
> libapi_b.so.3.4.5 => libimpl.so

Likewise here.

> I think this works when the application links, and the resulting
> applications would run.
>
> In the dlopen case, if you're in control of the applications, you can
> make them all open the right link library (libapi_x.so.n). This isn't
> possible if there are other implementations of the library that the
> application can link against (which may have different versions). In
> that case the application must dlopen libapi_x.so, which will get the
> stub library with this layout.

The application must dlopen based on soname, which is either
libapi_a.so.1 or libapi_b.so.3, and that will load libimpl.so.2.3.4.  It
must not use the link-time symbolic links, otherwise it risks picking up
an incompatible library with a different ABI.  This is a general rule,
it is not related to this setup.

> If this is what you intended, I think my main objection is that it's
> rather unexpected...

Why is it unexpected?

Thanks,
Florian
  
Florian Weimer Oct. 22, 2019, 9:57 a.m. UTC | #13
* David Kilroy:

>> Florian Weimer
>> * David Kilroy:
>> 
>> >> Florian Weimer
>> >> * David Kilroy:
>> >>
>> >> >> The Solaris documentation does not mention symbol filtering
>> >> (constraining
>> >> >> the set of exported symbols) at run time.  This kind of filtering
>> >> >> is
>> >> achieved at
>> >> >> run time because the link editor only looks at symbols in the
>> >> >> filter and
>> >> does
>> >> >> not consult the filtee.
>> >> >
>> >> > My assumption was that on Solaris only the symbols mentioned in the
>> >> filter
>> >> > actually get bound at runtime. I haven't looked at what would have
>> >> > to be
>> >> done
>> >> > to support that behaviour. I'm sounds non-trivial :)
>> >>
>> >> I expected that based on the name of the feature, but the Solaris
>> >> documentation does not support that.  It's only clear that they
>> >> filter at link time.
>> >
>> > [I just wanted to respond to this part of your email here. I'll
>> > respond to the rest via Szabolcs email.]
>> >
>> > The web page does say "The runtime linker uses the filtee to resolve
>> > any symbols defined by filter.so.1. If the filtee is not found, or a
>> > filter symbol is not found in the filtee, the filter is skipped for
>> > this symbol lookup."
>> 
>> It says “not found in the filtee”.  I think this means that the filter is
>> never used for symbol lookup (something we do not implement in glibc), not
>> that the symbol set of the filtee is restricted to that of the filter (not
>> implemented either).  It is possible that there was a typo here in and the
>> roles of filter and filtee are swapped in this documentation paragraph,
>> though.
>
> It makes sense to me. I agree that the filter is never used for symbol lookup.
>
> filter.so.1, has symbol a(), b() and has DT_FILTER filtee.so.1
> filtee.so.1, has symbols a(), c()
>
> The first sentence says that the symbols defined in filter.so.1 would be
> resolved from filtee.so.1. So the runtime linker should look for a() and b()
> in filtee.so.1.
>
> The second sentence deals with b(), which is not in the filtee. To me it says
> that b() shouldn't resolve to the filters implementation, and therefore b()
> has to come from later in the search path.

Ahh, thanks for explaining it to me.  So this is the part that specifies
that filters do actually filtering.

As I said, I think glibc does not implement this, and I'm not sure if
implementing it would break your use case (hence my question about
interposition and default implementations).

Florian
  
David Kilroy Oct. 22, 2019, 11:17 a.m. UTC | #14
> Florian Weimer
> 
> * Szabolcs Nagy:
> 
> > the scenario is:
> >
> > libA.so and libB.so export a set of symbols.  this is abi and there
> > are multiple providers of libA.so and libB.so.
> >
> > one provider wants to have a single libinternal.so that defines all
> > the symbols of libA and libB as they share a lot of code.
> >
> > (1) having libA.so and libB.so as "wrapper libraries" around
> > libinternal.so with RTLD_NEXT would work, but that's less efficient
> > because of the extra indirection,
> 
> If libA.so and libB.so have libinternal.so as a DT_NEEDED dependency,
> you do not need to implement forwarding with RTLD_NEXT.

This is possible. You still need wrapper functions of some form. You
can try to avoid them by defining libA with no symbols and DT_NEEDED
libinternal.so, but then you need to link with -Wl,--copy-dt-needed
(BFD only) and end up with a dependency on libinternal.so rather than
libA.so.

> > (3) symlinking libA.so and libB.so to libinternal.so makes all
> symbols
> > visible when either of them is loaded, polluting the link namespace.
> 
> Filters do that as well.  There is no actual per-symbol run-time
> filtering implemented today, and it is unclear if Solaris implements it.

Agreed.
 
> > (libinternal.so is a video driver lib and libA, libB, .. are various
> > opengl libs with fixed abi)
> 
> Is there an expectation that libinternal interposes symbols in libA
> etc., perhaps with more efficient implementations?  And if not, a
> fallback implementation in libA etc. is used?  This is clearly not
> supported by the Solaris filter feature (as documented).

I'm not sure what you mean here. Libinternal.so is expected to supply the
implementations for the symbols in libA.so. Ideally we wouldn't pick up
other symbols from libinternal, but this is not an issue for us.
If libinternal.so were to use IFUNC relocations for libA's symbols, I
would hope it works, but I don't think that's being done.


Thanks,

Dave.
  
David Kilroy Oct. 22, 2019, 11:20 a.m. UTC | #15
> Florian Weimer
> 
> * David Kilroy:
> 
> >> Szabolcs Nagy
> >>
> >> On 21/10/2019 15:21, Florian Weimer wrote:
> >> > * David Kilroy:
> >> >> I suspect that this wouldn't work in my use case though. External
> >> >> projects link against the filter object we produce, and don't
> know
> >> >> about the filtee (which is effectively an implementation detail).
> >> >> Different suppliers can also supply the library (which may or may
> not
> >> >> be a filter object), so having the external project link the
> library
> >> differently based on implementation is...
> >> >> suboptimal.
> >> >
> >> > If you currently use a symbolic link for the .so link object, it
> >> > should be easy enough to replace that with the script.  If the
> link
> >> > time DSO is stored under its soname, then of course this does not
> >> > work, but you wouldn't be able to transparently inject the filter
> >> > either, so that doesn't seem to be your scenario.
> >>
> >> linker script only works at static link time, not for dlopen.
> >
> > This did confuse me. I think you're proposing something like:
> >
> > Implementation library, libimpl.so
> > Filter libraries, libapi_a.so, libapi_b.so
> >
> > libimpl.so       => libimpl.so.2
> 
> This symbolic link should not be there if there is no expectation that
> applications link directly with -limpl.
> 
> > libimpl.so.2     => libimpl.so.2.3.4
> > libimpl.so.2.3.4    (the actual implementation, soname set to
> >                      libimpl.so.2)
> >
> > libapi_a.so          (stub with api selected using version script,
> >                       and soname set to libapi_a.so.1)
> > libapi_a.so.1     => libimpl.so
> > libapi_a.so.1.2.2 => libimpl.so
> 
> The two symbolic links above should probably go straight to
> libimpl.so.2.3.4 at run time, so that libimpl.so can be made optional.
> 
> > libapi_b.so          (stub with api selected using version script,
> >                       and soname set to libapi_b.so.3)
> > libapi_b.so.3     => libimpl.so
> > libapi_b.so.3.4.5 => libimpl.so
> 
> Likewise here.
> 
> > I think this works when the application links, and the resulting
> > applications would run.
> >
> > In the dlopen case, if you're in control of the applications, you can
> > make them all open the right link library (libapi_x.so.n). This isn't
> > possible if there are other implementations of the library that the
> > application can link against (which may have different versions). In
> > that case the application must dlopen libapi_x.so, which will get the
> > stub library with this layout.
> 
> The application must dlopen based on soname, which is either
> libapi_a.so.1 or libapi_b.so.3, and that will load libimpl.so.2.3.4.
> It
> must not use the link-time symbolic links, otherwise it risks picking
> up
> an incompatible library with a different ABI.  This is a general rule,
> it is not related to this setup.

Thanks for clarifying!

> > If this is what you intended, I think my main objection is that it's
> > rather unexpected...
> 
> Why is it unexpected?

No particular reason - I'm just used to seeing the symlinks point to the
more specific versions of the library.


Thanks,

Dave.
  
Florian Weimer Oct. 22, 2019, 11:22 a.m. UTC | #16
* David Kilroy:

>> Is there an expectation that libinternal interposes symbols in libA
>> etc., perhaps with more efficient implementations?  And if not, a
>> fallback implementation in libA etc. is used?  This is clearly not
>> supported by the Solaris filter feature (as documented).
>
> I'm not sure what you mean here. Libinternal.so is expected to supply the
> implementations for the symbols in libA.so. Ideally we wouldn't pick up
> other symbols from libinternal, but this is not an issue for us.
> If libinternal.so were to use IFUNC relocations for libA's symbols, I
> would hope it works, but I don't think that's being done.

I mean something like this.  libA defines f1, f2, f3.  libinternal
replaces f1 and f3 with optimized implementations, but not f2.
Applications which call f2 are expected to get the unoptimized
implementation in f2.

Thanks,
Florian
  
David Kilroy Oct. 22, 2019, 11:24 a.m. UTC | #17
> Florian Weimer

> 

> * David Kilroy:

> 

> >> Florian Weimer

> >> * David Kilroy:

> >>

> >> >> Florian Weimer

> >> >> * David Kilroy:

> >> >>

> >> >> >> The Solaris documentation does not mention symbol filtering

> >> >> (constraining

> >> >> >> the set of exported symbols) at run time.  This kind of

> >> >> >> filtering is

> >> >> achieved at

> >> >> >> run time because the link editor only looks at symbols in the

> >> >> >> filter and

> >> >> does

> >> >> >> not consult the filtee.

> >> >> >

> >> >> > My assumption was that on Solaris only the symbols mentioned in

> >> >> > the

> >> >> filter

> >> >> > actually get bound at runtime. I haven't looked at what would

> >> >> > have to be

> >> >> done

> >> >> > to support that behaviour. I'm sounds non-trivial :)

> >> >>

> >> >> I expected that based on the name of the feature, but the Solaris

> >> >> documentation does not support that.  It's only clear that they

> >> >> filter at link time.

> >> >

> >> > [I just wanted to respond to this part of your email here. I'll

> >> > respond to the rest via Szabolcs email.]

> >> >

> >> > The web page does say "The runtime linker uses the filtee to

> >> > resolve any symbols defined by filter.so.1. If the filtee is not

> >> > found, or a filter symbol is not found in the filtee, the filter

> is

> >> > skipped for this symbol lookup."

> >>

> >> It says “not found in the filtee”.  I think this means that the

> >> filter is never used for symbol lookup (something we do not

> implement

> >> in glibc), not that the symbol set of the filtee is restricted to

> >> that of the filter (not implemented either).  It is possible that

> >> there was a typo here in and the roles of filter and filtee are

> >> swapped in this documentation paragraph, though.

> >

> > It makes sense to me. I agree that the filter is never used for

> symbol lookup.

> >

> > filter.so.1, has symbol a(), b() and has DT_FILTER filtee.so.1

> > filtee.so.1, has symbols a(), c()

> >

> > The first sentence says that the symbols defined in filter.so.1 would

> > be resolved from filtee.so.1. So the runtime linker should look for

> > a() and b() in filtee.so.1.

> >

> > The second sentence deals with b(), which is not in the filtee. To me

> > it says that b() shouldn't resolve to the filters implementation, and

> > therefore b() has to come from later in the search path.

> 

> Ahh, thanks for explaining it to me.  So this is the part that

> specifies that filters do actually filtering.

> 

> As I said, I think glibc does not implement this, and I'm not sure if

> implementing it would break your use case (hence my question about

> interposition and default implementations).


Agreed. Glad I could clear up that point :)

If glibc did implement something closer to the Solaris spec, I would
expect what we're doing to continue working. But the current
implementation is sufficient for our purposes (bar the dlopen behaviour)


Thanks,

Dave.
  
David Kilroy Oct. 22, 2019, 11:27 a.m. UTC | #18
> Florian Weimer
> 
> * David Kilroy:
> 
> >> Is there an expectation that libinternal interposes symbols in libA
> >> etc., perhaps with more efficient implementations?  And if not, a
> >> fallback implementation in libA etc. is used?  This is clearly not
> >> supported by the Solaris filter feature (as documented).
> >
> > I'm not sure what you mean here. Libinternal.so is expected to supply
> > the implementations for the symbols in libA.so. Ideally we wouldn't
> > pick up other symbols from libinternal, but this is not an issue for
> us.
> > If libinternal.so were to use IFUNC relocations for libA's symbols, I
> > would hope it works, but I don't think that's being done.
> 
> I mean something like this.  libA defines f1, f2, f3.  libinternal
> replaces f1 and f3 with optimized implementations, but not f2.
> Applications which call f2 are expected to get the unoptimized
> implementation in f2.

Ah, no we don't do that. Our filter libraries would only contains stubs.
The real implementation for all functions would live in libinternal.so.


Thanks,

Dave.
  
Florian Weimer Oct. 22, 2019, 11:30 a.m. UTC | #19
* David Kilroy:

>> Florian Weimer
>> 
>> * David Kilroy:
>> 
>> >> Is there an expectation that libinternal interposes symbols in libA
>> >> etc., perhaps with more efficient implementations?  And if not, a
>> >> fallback implementation in libA etc. is used?  This is clearly not
>> >> supported by the Solaris filter feature (as documented).
>> >
>> > I'm not sure what you mean here. Libinternal.so is expected to supply
>> > the implementations for the symbols in libA.so. Ideally we wouldn't
>> > pick up other symbols from libinternal, but this is not an issue for
>> us.
>> > If libinternal.so were to use IFUNC relocations for libA's symbols, I
>> > would hope it works, but I don't think that's being done.
>> 
>> I mean something like this.  libA defines f1, f2, f3.  libinternal
>> replaces f1 and f3 with optimized implementations, but not f2.
>> Applications which call f2 are expected to get the unoptimized
>> implementation in f2.
>
> Ah, no we don't do that. Our filter libraries would only contains stubs.
> The real implementation for all functions would live in libinternal.so.

In this case, I think you should reconsider the use of linker scripts
referencing stub objects.  I still think they provide the functionality
you need.  You even need to load fewer objects at run time. 8-)

Thanks,
Florian
  
David Kilroy Oct. 22, 2019, 2:28 p.m. UTC | #20
> Florian Weimer
> 
> * David Kilroy:
> 
> > 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];
> >    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?

I've tried to double check this. Having not seen this code until recently, I
may have some of the details wrong but I've to summarized what I think is
the case below.

In short, as far as I can tell all the libraries in the l_next list also exist
in l_initfini.

If anyone knows otherwise, I'd appreciate a pointer.



Regards,

Dave.

In dl_open_worker the field new->l_next is populated by _dl_map_object_from_fd
(via _dl_map_object), where it calls _dl_add_to_namespace_list. Every loaded
object should be added to the global list.

New->l_initfini is populated in _dl_object_map_deps()

* The list `known` is populated with the binary, followed by preloads

** note: for the call from dlopen_worker, preloads is set to NULL

* dependencies are added to `known`

** each dependency is opened (via openaux and _dl_map_object), so the l_next
   list contains all new dependencies.

** each dependency gets its own map->l_initfini populated

* If the object is an aux or filter object

** the filtee is inserted before the filter in `known`.

** the l_next list is modified to put the filtee before the filter

* l_initfini for the main binary is then redone (even if previously loaded)

** It gets each library in `known`, excluding those with l_faked set
   (library not found in trace mode)

*** l_faked is only set to 1 in dl-load.c:_dl_map_object:2194

** l_initfini is sorted. Note that the sort keeps the main object at the
   head of l_initfini, unlike the sort in _dl_open_worker
  

Patch

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
+
 # 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
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);
+  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.  */
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];
   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);
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
@@ -0,0 +1,54 @@ 
+/* Test for BZ16272, dlopen'ing a filter object.
+   Ensure that symbols from the filter object resolve to the filtee.
+
+   Copyright (C) 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <string.h>
+#include <stdio.h>
+#include <dlfcn.h>
+
+int main (int argc, const char**argv)
+{
+  void *lib = dlopen (PFX "tst-filterobj-flt.so", RTLD_LAZY);
+  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;
+}
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
@@ -0,0 +1,24 @@ 
+/* Copyright (C) 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include "tst-filterobj-lib.h"
+
+/* We never want to see the output of the filter object */
+const char *get_text(void)
+{
+  return "Hello from filter object (FAIL)";
+}
diff --git a/elf/tst-filterobj-lib.c b/elf/tst-filterobj-lib.c
new file mode 100644
index 0000000..07e2348
--- /dev/null
+++ b/elf/tst-filterobj-lib.c
@@ -0,0 +1,24 @@ 
+/* Copyright (C) 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include "tst-filterobj-lib.h"
+
+/* This is the real implementation that wants to be called */
+const char *get_text (void)
+{
+  return "Hello from filtee (PASS)";
+}
diff --git a/elf/tst-filterobj-lib.h b/elf/tst-filterobj-lib.h
new file mode 100644
index 0000000..bed9bf8
--- /dev/null
+++ b/elf/tst-filterobj-lib.h
@@ -0,0 +1,18 @@ 
+/* Copyright (C) 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
+   <https://www.gnu.org/licenses/>.  */
+
+const char *get_text (void);
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
@@ -0,0 +1,34 @@ 
+/* Test that symbols from filter objects are resolved to the filtee.
+
+   Copyright (C) 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <string.h>
+#include <stdio.h>
+#include <dlfcn.h>
+
+#include "tst-filterobj-lib.h"
+
+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;
+}