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

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

[David Kilroy]

Repost of v2 with updated commit message resulting from follow-up
tests with a different implementation.

I'd still like to see this fixed, but the last patchset didn't get any
responses.

v3:
 - rebased for changes in elf/Makefile
 - updates to commit messages

v2:
https://sourceware.org/ml/libc-alpha/2019-10/msg00701.html
 - code formatting fixups
 - add dependency of test output on filtee library
 - tests changed to use the test framework

v1:
https://sourceware.org/ml/libc-alpha/2019-10/msg00519.html

David Kilroy (3):
  elf: Allow dlopen of filter object to work [BZ #16272]
  elf: avoid redundant sort in dlopen
  elf: avoid stack allocation in dl_open_worker

 elf/Makefile               | 12 ++++++++++--
 elf/dl-deps.c              | 35 ++++++++++++++++++++++++++---------
 elf/dl-open.c              | 32 +++++++++++++++-----------------
 elf/tst-filterobj-dlopen.c | 39 +++++++++++++++++++++++++++++++++++++++
 elf/tst-filterobj-flt.c    | 24 ++++++++++++++++++++++++
 elf/tst-filterobj-lib.c    | 24 ++++++++++++++++++++++++
 elf/tst-filterobj-lib.h    | 18 ++++++++++++++++++
 elf/tst-filterobj.c        | 36 ++++++++++++++++++++++++++++++++++++
 8 files changed, 192 insertions(+), 28 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

-- 
2.7.4

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

[David Kilroy]

There are two fixes that are needed to be able to dlopen filter
objects. First _dl_map_object_deps cannot assume that map will be at
the beginning of l_searchlist.r_list[], as filtees are inserted before
map. Secondly dl_open_worker needs to ensure that filtees get
relocated.

In _dl_map_object_deps:

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

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

Modify dl_open_worker to relocate the objects in new->l_inifini.
new->l_initfini is constructed in _dl_map_object_deps, and lists the
objects that need initialization and destruction. Originally the list
of objects in new->l_next are relocated. All of these objects should
also be included in new->l_initfini (both lists are populated with
dependencies in _dl_map_object_deps). We can't use new->l_prev to pick
up filtees, as during a recursive dlopen from an interposed malloc
call, l->prev can contain objects that are not ready for relocation.

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 | 39 +++++++++++++++++++++++++++++++++++++++
 elf/tst-filterobj-flt.c    | 24 ++++++++++++++++++++++++
 elf/tst-filterobj-lib.c    | 24 ++++++++++++++++++++++++
 elf/tst-filterobj-lib.h    | 18 ++++++++++++++++++
 elf/tst-filterobj.c        | 36 ++++++++++++++++++++++++++++++++++++
 8 files changed, 184 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

diff --git a/elf/Makefile b/elf/Makefile
index 0debea7..69f11c7 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -199,7 +199,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 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-tlsmodid \
-	 tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail
+	 tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail \
+	 tst-filterobj tst-filterobj-dlopen
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -292,7 +293,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
 		tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
 		tst-initlazyfailmod tst-finilazyfailmod \
-		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2
+		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2 \
+		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%,\
@@ -1627,3 +1629,9 @@ $(objpfx)tst-dlopenfailmod1.so: \
   $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
 LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
 $(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)
+
+LDFLAGS-tst-filterobj-flt.so = -Wl,--filter=$(objpfx)tst-filterobj-lib.so
+$(objpfx)tst-filterobj: $(objpfx)tst-filterobj-flt.so | $(objpfx)tst-filterobj-lib.so
+$(objpfx)tst-filterobj-dlopen: $(libdl) | $(objpfx)tst-filterobj-lib.so
+$(objpfx)tst-filterobj.out: $(objpfx)tst-filterobj-lib.so
+$(objpfx)tst-filterobj-dlopen.out: $(objpfx)tst-filterobj-lib.so
diff --git a/elf/dl-deps.c b/elf/dl-deps.c
index c29b988..bb85c83 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 df9f29a..9996fe9 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -637,22 +637,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..81eed0f
--- /dev/null
+++ b/elf/tst-filterobj-dlopen.c
@@ -0,0 +1,39 @@
+/* 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 <stdio.h>
+#include "support/check.h"
+#include "support/xdlfcn.h"
+
+static int do_test (void)
+{
+  void *lib = xdlopen ("tst-filterobj-flt.so", RTLD_LAZY);
+  char *(*fn)(void) = xdlsym (lib, "get_text");
+  const char* text = fn ();
+
+  printf ("%s\n", text);
+
+  /* Verify the text matches what we expect from the filtee */
+  TEST_COMPARE_STRING (text, "Hello from filtee (PASS)");
+
+  return 0;
+}
+
+#include "support/test-driver.c"
diff --git a/elf/tst-filterobj-flt.c b/elf/tst-filterobj-flt.c
new file mode 100644
index 0000000..b4e10b2
--- /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..d38eb9b
--- /dev/null
+++ b/elf/tst-filterobj.c
@@ -0,0 +1,36 @@
+/* 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 <stdio.h>
+#include "support/check.h"
+#include "tst-filterobj-lib.h"
+
+static int do_test (void)
+{
+  const char* text = get_text ();
+
+  printf ("%s\n", text);
+
+  /* Verify the text matches what we expect from the filtee */
+  TEST_COMPARE_STRING (text, "Hello from filtee (PASS)");
+
+  return 0;
+}
+
+#include "support/test-driver.c"
-- 
2.7.4

[PATCH v3 3/3] elf: avoid stack allocation in dl_open_worker

[David Kilroy]

As the sort was removed, there's no need to keep a separate map of
links. Instead, when relocating objects iterate over l_initfini
directly.

This allows us to remove the loop copying l_initfini elements into
map. We still need a loop to identify the first and last elements that
need relocation.

Tested by running the testsuite on x86_64.
---
 elf/dl-open.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/elf/dl-open.c b/elf/dl-open.c
index c4d09c7c..eb36a91 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -636,25 +636,18 @@ dl_open_worker (void *a)
   /* Sort the objects by dependency for the relocation process.  This
      allows IFUNC relocations to work and it also means copy
      relocation of dependencies are if necessary overwritten.  */
-  unsigned int nmaps = 0;
+  unsigned int first = UINT_MAX;
+  unsigned int last = 0;
   unsigned int j = 0;
   struct link_map *l = new->l_initfini[0];
   do
     {
       if (! l->l_real->l_relocated)
-	++nmaps;
-      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;
-  j = 0;
-  l = new->l_initfini[0];
-  do
-    {
-      if (! l->l_real->l_relocated)
-	maps[nmaps++] = l;
+	{
+	  if (first == UINT_MAX)
+	    first = j;
+	  last = j + 1;
+	}
       l = new->l_initfini[++j];
     }
   while (l != NULL);
@@ -669,9 +662,12 @@ dl_open_worker (void *a)
      them.  However, such relocation dependencies in IFUNC resolvers
      are undefined anyway, so this is not a problem.  */
 
-  for (unsigned int i = nmaps; i-- > 0; )
+  for (unsigned int i = last; i-- > first; )
     {
-      l = maps[i];
+      l = new->l_initfini[i];
+
+      if (l->l_real->l_relocated)
+	continue;
 
       if (! relocation_in_progress)
 	{
-- 
2.7.4

[PATCH v3 2/3] elf: avoid redundant sort in dlopen

[David Kilroy]

l_initfini is already sorted by dependency in _dl_map_object_deps(),
so avoid sorting again in dl_open_worker().

Tested by running the testsuite on x86_64.
---
 elf/dl-open.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/elf/dl-open.c b/elf/dl-open.c
index 9996fe9..c4d09c7c 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -658,7 +658,6 @@ dl_open_worker (void *a)
       l = new->l_initfini[++j];
     }
   while (l != NULL);
-  _dl_sort_maps (maps, nmaps, NULL, false);
 
   int relocation_in_progress = 0;
 
-- 
2.7.4

RE: [PATCH v3 0/3] elf: Allow dlopen of filter object to work [BZ #16272]

[David Kilroy]

> Repost of v2 with updated commit message resulting from follow-up
> tests with a different implementation.
> 
> I'd still like to see this fixed, but the last patchset didn't get any
> responses.
> 
> v3:
>  - rebased for changes in elf/Makefile
>  - updates to commit messages
> 
> v2:
> https://sourceware.org/ml/libc-alpha/2019-10/msg00701.html
>  - code formatting fixups
>  - add dependency of test output on filtee library
>  - tests changed to use the test framework
> 
> v1:
> https://sourceware.org/ml/libc-alpha/2019-10/msg00519.html

Ping.  Is there any interest in taking this series, or something like it?
I've seen that it needs another (almost trivial) rebase for changes in
elf/Makefile, but don't want do that unnecessarily.

It seems wrong that the toolchain can build a library as a forwarding library,
but it only works if you link against it normally (and not via dlopen).

This BZ is also referenced from https://gitlab.freedesktop.org/glvnd/libglvnd#issues

We came across this as we were exploring different ways to support multiple
stub libraries that at runtime forward to a single implementation library.
The Solaris-like forwarding libraries are nice for this, as the forwarding is
encapsulated in the compiled library.  What we end up doing is still under
consideration - we have a few options including link scripts+symlinks as
proposed by Florian in [1].  The different solutions have drawbacks for us,
so having dlopen'able forwarding libraries would help by giving us another
option.


Thanks,

Dave Kilroy.

[1] - https://sourceware.org/ml/libc-alpha/2019-10/msg00619.html

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

[Adhemerval Zanella]

On 18/12/2019 13:32, David Kilroy wrote:
>> Repost of v2 with updated commit message resulting from follow-up
>> tests with a different implementation.
>>
>> I'd still like to see this fixed, but the last patchset didn't get any
>> responses.
>>
>> v3:
>>  - rebased for changes in elf/Makefile
>>  - updates to commit messages
>>
>> v2:
>> https://sourceware.org/ml/libc-alpha/2019-10/msg00701.html
>>  - code formatting fixups
>>  - add dependency of test output on filtee library
>>  - tests changed to use the test framework
>>
>> v1:
>> https://sourceware.org/ml/libc-alpha/2019-10/msg00519.html
> 
> Ping.  Is there any interest in taking this series, or something like it?
> I've seen that it needs another (almost trivial) rebase for changes in
> elf/Makefile, but don't want do that unnecessarily.

I think there is interest, the main problem is always lack of manpower
and time from reviewers to move this forward. I will try to spend some
time to review it.

> 
> It seems wrong that the toolchain can build a library as a forwarding library,
> but it only works if you link against it normally (and not via dlopen).
> 
> This BZ is also referenced from https://gitlab.freedesktop.org/glvnd/libglvnd#issues
> 
> We came across this as we were exploring different ways to support multiple
> stub libraries that at runtime forward to a single implementation library.
> The Solaris-like forwarding libraries are nice for this, as the forwarding is
> encapsulated in the compiled library.  What we end up doing is still under
> consideration - we have a few options including link scripts+symlinks as
> proposed by Florian in [1].  The different solutions have drawbacks for us,
> so having dlopen'able forwarding libraries would help by giving us another
> option.
> 

Thanks for the links, I see that this issue comes and go with different
solutions that hack into the loader (like that dlmopen namespace proposal
sometime ago). At least this seems to fix an already available solution 
and also adds some code refactor and cleanup.

> 
> Thanks,
> 
> Dave Kilroy.
> 
> [1] - https://sourceware.org/ml/libc-alpha/2019-10/msg00619.html
> 

RE: [PATCH v3 0/3] elf: Allow dlopen of filter object to work [BZ #16272]

[David Kilroy]

Adhemerval Zanella wrote:
> On 18/12/2019 13:32, David Kilroy wrote:
> >> Repost of v2 with updated commit message resulting from follow-up
> >> tests with a different implementation.
> >>
> >> I'd still like to see this fixed, but the last patchset didn't get
> any
> >> responses.
> >>
> >> v3:
> >>  - rebased for changes in elf/Makefile
> >>  - updates to commit messages
> >>
> >> v2:
> >> https://sourceware.org/ml/libc-alpha/2019-10/msg00701.html
> >>  - code formatting fixups
> >>  - add dependency of test output on filtee library
> >>  - tests changed to use the test framework
> >>
> >> v1:
> >> https://sourceware.org/ml/libc-alpha/2019-10/msg00519.html
> >
> > Ping.  Is there any interest in taking this series, or something like
> it?
> > I've seen that it needs another (almost trivial) rebase for changes
> in
> > elf/Makefile, but don't want do that unnecessarily.
> 
> I think there is interest, the main problem is always lack of manpower
> and time from reviewers to move this forward. I will try to spend some
> time to review it.

Thanks Adhemerval. I understand - it's not always clear which is the case :)
I'll keep this on my list of things to chase.



Regards,

Dave Kilroy.

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

[Adhemerval Zanella]

On 03/12/2019 14:30, David Kilroy wrote:
> There are two fixes that are needed to be able to dlopen filter
> objects. First _dl_map_object_deps cannot assume that map will be at
> the beginning of l_searchlist.r_list[], as filtees are inserted before
> map. Secondly dl_open_worker needs to ensure that filtees get
> relocated.
> 
> In _dl_map_object_deps:
> 
> * avoiding removing relocatiion dependencies of map by setting

s/relocatiion/relocation

>   l_reserved to 0 and otherwise processing the rest of the search
>   list.
> 
> * ensure 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.
> 
> Modify dl_open_worker to relocate the objects in new->l_inifini.
> new->l_initfini is constructed in _dl_map_object_deps, and lists the
> objects that need initialization and destruction. Originally the list
> of objects in new->l_next are relocated. All of these objects should
> also be included in new->l_initfini (both lists are populated with
> dependencies in _dl_map_object_deps). We can't use new->l_prev to pick
> up filtees, as during a recursive dlopen from an interposed malloc
> call, l->prev can contain objects that are not ready for relocation.
> 
> 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.

Since these are the only filter tests, I think we should also add some
minimal one to check for invalid filter filter object (which does not
provide the required symbol interfaces provided by the filtee).

I think also it is worth to extend the tests to cover for auxiliary
filters as well (DT_AUXILIARY), since it intersects with DT_FILTERS.
It should be similar to DT_FILTER tests, with the only difference 
that non existent auxiliary filters does not trigger a loader error
(if the symbol is provided by the filter or another auxiliary filter).

The patch itself looks good, thanks for working on it. 

> 
> 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 | 39 +++++++++++++++++++++++++++++++++++++++
>  elf/tst-filterobj-flt.c    | 24 ++++++++++++++++++++++++
>  elf/tst-filterobj-lib.c    | 24 ++++++++++++++++++++++++
>  elf/tst-filterobj-lib.h    | 18 ++++++++++++++++++
>  elf/tst-filterobj.c        | 36 ++++++++++++++++++++++++++++++++++++
>  8 files changed, 184 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
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 0debea7..69f11c7 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -199,7 +199,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 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-tlsmodid \
> -	 tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail
> +	 tst-dlopen-self tst-auditmany tst-initfinilazyfail tst-dlopenfail \
> +	 tst-filterobj tst-filterobj-dlopen
>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \

Ok.

> @@ -292,7 +293,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-auditmanymod4 tst-auditmanymod5 tst-auditmanymod6 \
>  		tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
>  		tst-initlazyfailmod tst-finilazyfailmod \
> -		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2
> +		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2 \
> +		tst-filterobj-flt tst-filterobj-lib

Ok.

>  # 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%,\
> @@ -1627,3 +1629,9 @@ $(objpfx)tst-dlopenfailmod1.so: \
>    $(shared-thread-library) $(objpfx)tst-dlopenfaillinkmod.so
>  LDFLAGS-tst-dlopenfaillinkmod.so = -Wl,-soname,tst-dlopenfail-missingmod.so
>  $(objpfx)tst-dlopenfailmod2.so: $(shared-thread-library)
> +
> +LDFLAGS-tst-filterobj-flt.so = -Wl,--filter=$(objpfx)tst-filterobj-lib.so
> +$(objpfx)tst-filterobj: $(objpfx)tst-filterobj-flt.so | $(objpfx)tst-filterobj-lib.so
> +$(objpfx)tst-filterobj-dlopen: $(libdl) | $(objpfx)tst-filterobj-lib.so
> +$(objpfx)tst-filterobj.out: $(objpfx)tst-filterobj-lib.so
> +$(objpfx)tst-filterobj-dlopen.out: $(objpfx)tst-filterobj-lib.so

I think there is no need need to set the tst-filterobj* to be order-only
here, afaik we don't currently support issuing the 'check' without a 
full build if the tree is updated. I am not sure if order-only prerequisite
here would make some difference.

> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index c29b988..bb85c83 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);

Ok, the first entry is the filtee object.

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

I am trying to understand why we can't skip first element here. Neither
of the tests actually exercise this code patch (they won't add a
dependency on l_reldeps), so could you provide an example/testcase
where it requires such change?

>  
> -  /* 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;

Ok. I am not sure if it is worth the optimization trouble, but one option
might be to compute the 'map' index while 'map->l_searchlist.r_list' is
being set while iterating over 'known' maps (loop at line 489).

> +  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 *));

Ok (although I not sure if is the correct idiom to use memcpy to
copy array of pointers).


> +    }
> +  else
> +    memcpy (l_initfini, map->l_searchlist.r_list,
> +	    nlist * sizeof (struct link_map *));
> +

Ok

>    _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 df9f29a..9996fe9 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -637,22 +637,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);

Ok.

> +  /* 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);

Ok.

> diff --git a/elf/tst-filterobj-dlopen.c b/elf/tst-filterobj-dlopen.c
> new file mode 100644
> index 0000000..81eed0f
> --- /dev/null
> +++ b/elf/tst-filterobj-dlopen.c
> @@ -0,0 +1,39 @@
> +/* 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.

Update Copyright year to 2020.

> +   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 <stdio.h>
> +#include "support/check.h"
> +#include "support/xdlfcn.h"
> +
> +static int do_test (void)
> +{
> +  void *lib = xdlopen ("tst-filterobj-flt.so", RTLD_LAZY);
> +  char *(*fn)(void) = xdlsym (lib, "get_text");
> +  const char* text = fn ();
> +
> +  printf ("%s\n", text);
> +
> +  /* Verify the text matches what we expect from the filtee */
> +  TEST_COMPARE_STRING (text, "Hello from filtee (PASS)");
> +
> +  return 0;
> +}
> +
> +#include "support/test-driver.c"

Ok (although we usually use '<' for support files).

> diff --git a/elf/tst-filterobj-flt.c b/elf/tst-filterobj-flt.c
> new file mode 100644
> index 0000000..b4e10b2
> --- /dev/null
> +++ b/elf/tst-filterobj-flt.c
> @@ -0,0 +1,24 @@
> +/* Copyright (C) 2019 Free Software Foundation, Inc.

Missing one line description and Copyright year update to 2020.

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

Ok.

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

Missing one line description and Copyright year update to 2020.

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

Ok.

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

Missing one line description and Copyright year update to 2020.

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

Ok.

> diff --git a/elf/tst-filterobj.c b/elf/tst-filterobj.c
> new file mode 100644
> index 0000000..d38eb9b
> --- /dev/null
> +++ b/elf/tst-filterobj.c
> @@ -0,0 +1,36 @@
> +/* Test that symbols from filter objects are resolved to the filtee.
> +
> +   Copyright (C) 2019 Free Software Foundation, Inc.

Update Copyright year to 2020.

> +   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 <stdio.h>
> +#include "support/check.h"
> +#include "tst-filterobj-lib.h"
> +
> +static int do_test (void)
> +{
> +  const char* text = get_text ();
> +
> +  printf ("%s\n", text);
> +
> +  /* Verify the text matches what we expect from the filtee */
> +  TEST_COMPARE_STRING (text, "Hello from filtee (PASS)");
> +
> +  return 0;
> +}
> +
> +#include "support/test-driver.c"
> 

Ok.

Re: [PATCH v3 2/3] elf: avoid redundant sort in dlopen

[Adhemerval Zanella]

On 03/12/2019 14:30, David Kilroy wrote:
> l_initfini is already sorted by dependency in _dl_map_object_deps(),
> so avoid sorting again in dl_open_worker().
> 
> Tested by running the testsuite on x86_64.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-open.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 9996fe9..c4d09c7c 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -658,7 +658,6 @@ dl_open_worker (void *a)
>        l = new->l_initfini[++j];
>      }
>    while (l != NULL);
> -  _dl_sort_maps (maps, nmaps, NULL, false);
>  
>    int relocation_in_progress = 0;
>  
> 

Re: [PATCH v3 3/3] elf: avoid stack allocation in dl_open_worker

[Adhemerval Zanella]

On 03/12/2019 14:30, David Kilroy wrote:
> As the sort was removed, there's no need to keep a separate map of
> links. Instead, when relocating objects iterate over l_initfini
> directly.
> 
> This allows us to remove the loop copying l_initfini elements into
> map. We still need a loop to identify the first and last elements that
> need relocation.
> 
> Tested by running the testsuite on x86_64.

LGTM, thanks.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-open.c | 28 ++++++++++++----------------
>  1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index c4d09c7c..eb36a91 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -636,25 +636,18 @@ dl_open_worker (void *a)
>    /* Sort the objects by dependency for the relocation process.  This
>       allows IFUNC relocations to work and it also means copy
>       relocation of dependencies are if necessary overwritten.  */

I think we need to update the comment to state this is not sorting
anymore, but rather using the already sorted list (done by
_dl_map_object_deps).

> -  unsigned int nmaps = 0;
> +  unsigned int first = UINT_MAX;
> +  unsigned int last = 0;
>    unsigned int j = 0;
>    struct link_map *l = new->l_initfini[0];
>    do
>      {
>        if (! l->l_real->l_relocated)
> -	++nmaps;
> -      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;
> -  j = 0;
> -  l = new->l_initfini[0];
> -  do
> -    {
> -      if (! l->l_real->l_relocated)
> -	maps[nmaps++] = l;
> +	{
> +	  if (first == UINT_MAX)
> +	    first = j;
> +	  last = j + 1;
> +	}
>        l = new->l_initfini[++j];
>      }
>    while (l != NULL);

Ok, it sets the range [first, last] as the potential maps that requires
relocation.

> @@ -669,9 +662,12 @@ dl_open_worker (void *a)
>       them.  However, such relocation dependencies in IFUNC resolvers
>       are undefined anyway, so this is not a problem.  */
>  
> -  for (unsigned int i = nmaps; i-- > 0; )
> +  for (unsigned int i = last; i-- > first; )
>      {
> -      l = maps[i];
> +      l = new->l_initfini[i];
> +
> +      if (l->l_real->l_relocated)
> +	continue;
>  
>        if (! relocation_in_progress)
>  	{
> 

Ok, some objects objects might already relocated.

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

[David Kilroy]

> -----Original Message-----
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> Since these are the only filter tests, I think we should also add some
> minimal one to check for invalid filter filter object (which does not
> provide the required symbol interfaces provided by the filtee).
> 
> I think also it is worth to extend the tests to cover for auxiliary
> filters as well (DT_AUXILIARY), since it intersects with DT_FILTERS.
> It should be similar to DT_FILTER tests, with the only difference
> that non existent auxiliary filters does not trigger a loader error
> (if the symbol is provided by the filter or another auxiliary filter).
> 
> The patch itself looks good, thanks for working on it.

Thanks for the feedback!

I'll rebase and take a look at resolving your comments on the 3 commits.
That may take a couple days :)

I think all your comments are clear enough, and I should be able to
deal with. Just on this one...

> > -  /* 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 *));
...

> > +  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 *));
> 
> Ok (although I not sure if is the correct idiom to use memcpy to
> copy array of pointers
> 
> 
> > +    }
> > +  else
> > +    memcpy (l_initfini, map->l_searchlist.r_list,
> > +	    nlist * sizeof (struct link_map *));
> > +

The original code was using a memcpy (presumably for speed), so I tried to
stick with that. I'm happy to change the new branch to use a loop to copy the
pointers - should I leave the original copy as a memcpy?



Dave.

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

[Adhemerval Zanella]

On 15/01/2020 09:03, David Kilroy wrote:
>> -----Original Message-----
>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
>> Since these are the only filter tests, I think we should also add some
>> minimal one to check for invalid filter filter object (which does not
>> provide the required symbol interfaces provided by the filtee).
>>
>> I think also it is worth to extend the tests to cover for auxiliary
>> filters as well (DT_AUXILIARY), since it intersects with DT_FILTERS.
>> It should be similar to DT_FILTER tests, with the only difference
>> that non existent auxiliary filters does not trigger a loader error
>> (if the symbol is provided by the filter or another auxiliary filter).
>>
>> The patch itself looks good, thanks for working on it.
> 
> Thanks for the feedback!
> 
> I'll rebase and take a look at resolving your comments on the 3 commits.
> That may take a couple days :)
> 
> I think all your comments are clear enough, and I should be able to
> deal with. Just on this one...
> 
>>> -  /* 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 *));
> ...
> 
>>> +  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 *));
>>
>> Ok (although I not sure if is the correct idiom to use memcpy to
>> copy array of pointers
>>
>>
>>> +    }
>>> +  else
>>> +    memcpy (l_initfini, map->l_searchlist.r_list,
>>> +	    nlist * sizeof (struct link_map *));
>>> +
> 
> The original code was using a memcpy (presumably for speed), so I tried to
> stick with that. I'm happy to change the new branch to use a loop to copy the
> pointers - should I leave the original copy as a memcpy?

I don't have a strong preference, I think we can use the memcpy
code.

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

[David Kilroy]

> -----Original Message-----
> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> > diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> > index c29b988..bb85c83 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);
> 
> Ok, the first entry is the filtee object.
> 
> >    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;
> >      }
> 
> I am trying to understand why we can't skip first element here. Neither
> of the tests actually exercise this code patch (they won't add a
> dependency on l_reldeps), so could you provide an example/testcase
> where it requires such change?

I haven't observed the use case that this code handles. I just tried to
maintain the existing behaviour that we avoid doing this for the main object.

I've been trying to understand what triggers this, but not getting very far.
Does anyone have any hints as to how I need to setup the test so that we do
trigger relocation removals?


Thanks,

Dave.

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

[David Kilroy]

> -----Original Message-----
> From: David Kilroy
> > -----Original Message-----
> > From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> >
> > > diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> > > index c29b988..bb85c83 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);
> >
> > Ok, the first entry is the filtee object.
> >
> > >    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;
> > >      }
> >
> > I am trying to understand why we can't skip first element here.
> Neither
> > of the tests actually exercise this code patch (they won't add a
> > dependency on l_reldeps), so could you provide an example/testcase
> > where it requires such change?
> 
> I haven't observed the use case that this code handles. I just tried to
> maintain the existing behaviour that we avoid doing this for the main
> object.
> 
> I've been trying to understand what triggers this, but not getting very
> far.
> Does anyone have any hints as to how I need to setup the test so that
> we do
> trigger relocation removals?

OK, so I git blamed this code to commit c4bb124a75 from 2001. I adapted
elf/relmod5 to include a filter library, and that is working. But it's not
hitting the relocation removal code (checked by adding _dlerror_printfs).
I then ran all tests in the elf directory. Only elf/tst-libc_dlvsym-static and
elf/tst-dlmopen1 enter the for loop, but the l_reserved check is always false.
In both of the above cases map is libc.so.6 and list[i] is libdl.so.2

I verified the same is the case without any of my commits applied.

Have changes elsewhere superseded this? Suggestions as to what I should do
here appreciated :)


Dave.

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

[Adhemerval Zanella]

On 21/01/2020 12:59, David Kilroy wrote:
>> -----Original Message-----
>> From: David Kilroy
>>> -----Original Message-----
>>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>>
>>>> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
>>>> index c29b988..bb85c83 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);
>>>
>>> Ok, the first entry is the filtee object.
>>>
>>>>    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;
>>>>      }
>>>
>>> I am trying to understand why we can't skip first element here.
>> Neither
>>> of the tests actually exercise this code patch (they won't add a
>>> dependency on l_reldeps), so could you provide an example/testcase
>>> where it requires such change?
>>
>> I haven't observed the use case that this code handles. I just tried to
>> maintain the existing behaviour that we avoid doing this for the main
>> object.
>>
>> I've been trying to understand what triggers this, but not getting very
>> far.
>> Does anyone have any hints as to how I need to setup the test so that
>> we do
>> trigger relocation removals?
> 
> OK, so I git blamed this code to commit c4bb124a75 from 2001. I adapted
> elf/relmod5 to include a filter library, and that is working. But it's not
> hitting the relocation removal code (checked by adding _dlerror_printfs).
> I then ran all tests in the elf directory. Only elf/tst-libc_dlvsym-static and
> elf/tst-dlmopen1 enter the for loop, but the l_reserved check is always false.
> In both of the above cases map is libc.so.6 and list[i] is libdl.so.2
> 
> I verified the same is the case without any of my commits applied.
> 
> Have changes elsewhere superseded this? Suggestions as to what I should do
> here appreciated :)

I am trying to understand how exactly this code is exercised, the
l_reldeps will be set by add_dependency function and this is
set only on _dl_lookup_symbol for some specific case.  I will
probably need more time to dig into this, but my understanding
so far the this change should not change current semantic since
removing the main binary with 'map->l_reserved'.