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

[PATCH v4 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 314adc2..7b3b177 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -621,25 +621,18 @@ dl_open_worker (void *a)
      This allows IFUNC relocations to work and it also means copy
      relocation of dependencies are if necessary overwritten.
      __dl_map_object_deps has already sorted l_initfini for us.  */
-  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);
@@ -654,9 +647,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 v4 0/3] elf: Allow dlopen of filter object to work [BZ #16272]

[David Kilroy]

Glibc currently supports filter objects (shared libraries compiled
with -Wl,--filter or -Wl,--auxiliary) by inserting the filtee ahead of
the filter object in the search path. This works for the case where
the application is linked against the filter object.

When the application tries to use dlopen to load the filter object,
glibc currently fails with:

Inconsistency detected by ld.so: dl-deps.c: 574: _dl_map_object_deps:
Assertion `map->l_searchlist.r_list[0] == map' failed!

This fails because dl_map_object_deps assumes that the library being
loaded is at the head of the search list.

The filtee object also needs to be relocated when dlopen is used.

The first patch attempts to address these in a minimal way, and adds
simple test cases for filter and auxiliary objects.

The follow up patches do some cleanup. If we can use l_initfini to do
the relocations, then the call to _dl_sort_maps is redundant. Once
that is removed there is no need for the stack allocation of map.

Outstanding issues/concerns from previous patchset reviews:

 - Florian points out that for this use case, the standard way to do
   API filtering is to have a linker script, and setup the soname and
   symlinks to point to the implementation library.   
   See thread at https://sourceware.org/ml/libc-alpha/2019-10/msg00636.html

   This should work. The install is a bit more complicated than just
   copying the library to the right place. I'd still like to fix the
   filter objects though...

 - Adhemerval pointed out that the code that removes relocation
   dependencies is not tested by the testcase in patchset 3. This is
   still the case, as I haven't been able to trigger this.
   See thread at https://sourceware.org/ml/libc-alpha/2020-01/msg00481.html

v4:
 - update copyrights to 2020
 - extend testing to cover auxiliary filter objects
 - switch a series of memcpy calls into loops to copy pointers
 - calculate map_index during earlier for loop
 - add missing file descriptions
 - update sort description

v3:
https://sourceware.org/ml/libc-alpha/2019-12/msg00099.html
 - rebased for changes in elf/Makefile
 - updates to commit messages

v2:
https://sourceware.org/ml/libc-alpha/2019-10/msg00698.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               | 18 ++++++++++++++++--
 elf/dl-deps.c              | 39 ++++++++++++++++++++++++++++----------
 elf/dl-open.c              | 39 +++++++++++++++++++-------------------
 elf/tst-auxobj-dlopen.c    | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 elf/tst-auxobj.c           | 42 +++++++++++++++++++++++++++++++++++++++++
 elf/tst-filterobj-aux.c    | 33 ++++++++++++++++++++++++++++++++
 elf/tst-filterobj-dlopen.c | 39 ++++++++++++++++++++++++++++++++++++++
 elf/tst-filterobj-filtee.c | 27 ++++++++++++++++++++++++++
 elf/tst-filterobj-filtee.h | 24 +++++++++++++++++++++++
 elf/tst-filterobj-flt.c    | 27 ++++++++++++++++++++++++++
 elf/tst-filterobj.c        | 36 +++++++++++++++++++++++++++++++++++
 11 files changed, 339 insertions(+), 32 deletions(-)
 create mode 100644 elf/tst-auxobj-dlopen.c
 create mode 100644 elf/tst-auxobj.c
 create mode 100644 elf/tst-filterobj-aux.c
 create mode 100644 elf/tst-filterobj-dlopen.c
 create mode 100644 elf/tst-filterobj-filtee.c
 create mode 100644 elf/tst-filterobj-filtee.h
 create mode 100644 elf/tst-filterobj-flt.c
 create mode 100644 elf/tst-filterobj.c

-- 
2.7.4

[PATCH v4 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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/elf/dl-open.c b/elf/dl-open.c
index ecb2ba9..314adc2 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -617,9 +617,10 @@ dl_open_worker (void *a)
   if (GLRO(dl_lazy))
     reloc_mode |= mode & RTLD_LAZY;
 
-  /* 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.  */
+  /* Objects must be sorted by dependency for the relocation process.
+     This allows IFUNC relocations to work and it also means copy
+     relocation of dependencies are if necessary overwritten.
+     __dl_map_object_deps has already sorted l_initfini for us.  */
   unsigned int nmaps = 0;
   unsigned int j = 0;
   struct link_map *l = new->l_initfini[0];
@@ -642,7 +643,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

[PATCH v4 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 relocation 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 auxiliary and filter objects are used, both as a normal link and
when dlopen'd.

Tested by running the testsuite on x86_64.
---
 elf/Makefile               | 18 ++++++++++++++++--
 elf/dl-deps.c              | 39 ++++++++++++++++++++++++++++----------
 elf/dl-open.c              | 11 +++++++----
 elf/tst-auxobj-dlopen.c    | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 elf/tst-auxobj.c           | 42 +++++++++++++++++++++++++++++++++++++++++
 elf/tst-filterobj-aux.c    | 33 ++++++++++++++++++++++++++++++++
 elf/tst-filterobj-dlopen.c | 39 ++++++++++++++++++++++++++++++++++++++
 elf/tst-filterobj-filtee.c | 27 ++++++++++++++++++++++++++
 elf/tst-filterobj-filtee.h | 24 +++++++++++++++++++++++
 elf/tst-filterobj-flt.c    | 27 ++++++++++++++++++++++++++
 elf/tst-filterobj.c        | 36 +++++++++++++++++++++++++++++++++++
 11 files changed, 327 insertions(+), 16 deletions(-)
 create mode 100644 elf/tst-auxobj-dlopen.c
 create mode 100644 elf/tst-auxobj.c
 create mode 100644 elf/tst-filterobj-aux.c
 create mode 100644 elf/tst-filterobj-dlopen.c
 create mode 100644 elf/tst-filterobj-filtee.c
 create mode 100644 elf/tst-filterobj-filtee.h
 create mode 100644 elf/tst-filterobj-flt.c
 create mode 100644 elf/tst-filterobj.c

diff --git a/elf/Makefile b/elf/Makefile
index 6c62ed6..0122431 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -201,7 +201,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
 	 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-dlopenfail-2
+	 tst-dlopenfail-2 \
+	 tst-filterobj tst-filterobj-dlopen tst-auxobj tst-auxobj-dlopen
 #	 reldep9
 tests-internal += loadtest unload unload2 circleload1 \
 	 neededtest neededtest2 neededtest3 neededtest4 \
@@ -312,7 +313,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
 		tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
 		tst-initlazyfailmod tst-finilazyfailmod \
 		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2 \
-		tst-dlopenfailmod3 tst-ldconfig-ld-mod
+		tst-dlopenfailmod3 tst-ldconfig-ld-mod \
+		tst-filterobj-flt tst-filterobj-aux tst-filterobj-filtee
 # 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%,\
@@ -1698,3 +1700,15 @@ LDFLAGS-tst-dlopen-nodelete-reloc-mod17.so = -Wl,--no-as-needed
 
 $(objpfx)tst-ldconfig-ld_so_conf-update.out: $(objpfx)tst-ldconfig-ld-mod.so
 $(objpfx)tst-ldconfig-ld_so_conf-update: $(libdl)
+
+LDFLAGS-tst-filterobj-flt.so = -Wl,--filter=$(objpfx)tst-filterobj-filtee.so
+$(objpfx)tst-filterobj: $(objpfx)tst-filterobj-flt.so
+$(objpfx)tst-filterobj-dlopen: $(libdl)
+$(objpfx)tst-filterobj.out: $(objpfx)tst-filterobj-filtee.so
+$(objpfx)tst-filterobj-dlopen.out: $(objpfx)tst-filterobj-filtee.so
+
+LDFLAGS-tst-filterobj-aux.so = -Wl,--auxiliary=$(objpfx)tst-filterobj-filtee.so
+$(objpfx)tst-auxobj: $(objpfx)tst-filterobj-aux.so
+$(objpfx)tst-auxobj-dlopen: $(libdl)
+$(objpfx)tst-auxobj.out: $(objpfx)tst-filterobj-filtee.so
+$(objpfx)tst-auxobj-dlopen.out: $(objpfx)tst-filterobj-filtee.so
diff --git a/elf/dl-deps.c b/elf/dl-deps.c
index 5103a8a..0730ea9 100644
--- a/elf/dl-deps.c
+++ b/elf/dl-deps.c
@@ -485,14 +485,18 @@ _dl_map_object_deps (struct link_map *map,
 
   map->l_searchlist.r_list = &l_initfini[nlist + 1];
   map->l_searchlist.r_nlist = nlist;
+  unsigned int map_index = UINT_MAX;
 
   for (nlist = 0, runp = known; runp; runp = runp->next)
     {
       if (__builtin_expect (trace_mode, 0) && runp->map->l_faked)
 	/* This can happen when we trace the loading.  */
 	--map->l_searchlist.r_nlist;
-      else
+      else {
+	if (runp->map == map)
+	  map_index = nlist;
 	map->l_searchlist.r_list[nlist++] = runp->map;
+      }
 
       /* Now clear all the mark bits we set in the objects on the search list
 	 to avoid duplicates, so the next call starts fresh.  */
@@ -550,13 +554,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 +586,30 @@ 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.  */
+  assert (map_index < nlist);
+  if (map_index > 0)
+    {
+      /* Copy the binary into position 0.  */
+      l_initfini[0] = map->l_searchlist.r_list[map_index];
+
+      /* Copy the filtees.  */
+      for (i = 0; i < map_index; ++i)
+	l_initfini[i+1] = map->l_searchlist.r_list[i];
+
+      /* Copy the remainder.  */
+      for (i = map_index + 1; i < nlist; ++i)
+	l_initfini[i] = map->l_searchlist.r_list[i];
+    }
+  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 623c975..ecb2ba9 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -621,22 +621,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-auxobj-dlopen.c b/elf/tst-auxobj-dlopen.c
new file mode 100644
index 0000000..7f91bdd
--- /dev/null
+++ b/elf/tst-auxobj-dlopen.c
@@ -0,0 +1,47 @@
+/* Test for BZ16272, dlopen'ing an auxiliary filter object.
+   Ensure that symbols from the resolve correctly.
+
+   Copyright (C) 2020 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-aux.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)");
+
+  fn = xdlsym (lib, "get_text2");
+  text = fn ();
+
+  printf ("%s\n", text);
+
+  /* Verify the text matches what we expect from the auxiliary object */
+  TEST_COMPARE_STRING (text, "Hello from auxiliary filter object (PASS)");
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-auxobj.c b/elf/tst-auxobj.c
new file mode 100644
index 0000000..bdc7713
--- /dev/null
+++ b/elf/tst-auxobj.c
@@ -0,0 +1,42 @@
+/* Test that symbols from auxiliary filter objects are resolved to the
+   filtee.
+
+   Copyright (C) 2020 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-filtee.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)");
+
+  text = get_text2 ();
+  printf ("%s\n", text);
+
+  /* Verify the text matches what we expect from the auxiliary object */
+  TEST_COMPARE_STRING (text, "Hello from auxiliary filter object (PASS)");
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/elf/tst-filterobj-aux.c b/elf/tst-filterobj-aux.c
new file mode 100644
index 0000000..0b732f2
--- /dev/null
+++ b/elf/tst-filterobj-aux.c
@@ -0,0 +1,33 @@
+/* Auxiliary filter object.
+   Contains symbols to be resolved in filtee, and one which doesn't.
+
+   Copyright (C) 2020 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-filtee.h"
+
+/* We never want to see the output of the auxiliary object.  */
+const char *get_text (void)
+{
+  return "Hello from auxiliary filter object (FAIL)";
+}
+
+/* The filtee doesn't implement this symbol, so this should resolve.  */
+const char *get_text2 (void)
+{
+  return "Hello from auxiliary filter object (PASS)";
+}
diff --git a/elf/tst-filterobj-dlopen.c b/elf/tst-filterobj-dlopen.c
new file mode 100644
index 0000000..089a3e9
--- /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) 2020 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-filtee.c b/elf/tst-filterobj-filtee.c
new file mode 100644
index 0000000..817576b
--- /dev/null
+++ b/elf/tst-filterobj-filtee.c
@@ -0,0 +1,27 @@
+/* Filtee for BZ16272 test.
+   Contains desired symbol implementations.
+
+   Copyright (C) 2020 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-filtee.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-filtee.h b/elf/tst-filterobj-filtee.h
new file mode 100644
index 0000000..69f4749
--- /dev/null
+++ b/elf/tst-filterobj-filtee.h
@@ -0,0 +1,24 @@
+/* Filtee header for BZ16272 test.
+   Contains prototypes for symbols implemented in the filtee.
+
+   Copyright (C) 2020 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);
+
+/* For testing auxiliary filter object.  */
+const char *get_text2 (void);
diff --git a/elf/tst-filterobj-flt.c b/elf/tst-filterobj-flt.c
new file mode 100644
index 0000000..9da5ef1
--- /dev/null
+++ b/elf/tst-filterobj-flt.c
@@ -0,0 +1,27 @@
+/* Filter object for BZ16272 test.
+   Contains symbols to be resolved in filtee.
+
+   Copyright (C) 2020 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-filtee.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.c b/elf/tst-filterobj.c
new file mode 100644
index 0000000..96bfae0
--- /dev/null
+++ b/elf/tst-filterobj.c
@@ -0,0 +1,36 @@
+/* Test that symbols from filter objects are resolved to the filtee.
+
+   Copyright (C) 2020 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-filtee.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

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

[Adhemerval Zanella]

On 29/01/2020 08:17, 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 relocation 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 auxiliary and filter objects are used, both as a normal link and
> when dlopen'd.
> 
> Tested by running the testsuite on x86_64.

LGTM, thanks.

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

> ---
>  elf/Makefile               | 18 ++++++++++++++++--
>  elf/dl-deps.c              | 39 ++++++++++++++++++++++++++++----------
>  elf/dl-open.c              | 11 +++++++----
>  elf/tst-auxobj-dlopen.c    | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  elf/tst-auxobj.c           | 42 +++++++++++++++++++++++++++++++++++++++++
>  elf/tst-filterobj-aux.c    | 33 ++++++++++++++++++++++++++++++++
>  elf/tst-filterobj-dlopen.c | 39 ++++++++++++++++++++++++++++++++++++++
>  elf/tst-filterobj-filtee.c | 27 ++++++++++++++++++++++++++
>  elf/tst-filterobj-filtee.h | 24 +++++++++++++++++++++++
>  elf/tst-filterobj-flt.c    | 27 ++++++++++++++++++++++++++
>  elf/tst-filterobj.c        | 36 +++++++++++++++++++++++++++++++++++
>  11 files changed, 327 insertions(+), 16 deletions(-)
>  create mode 100644 elf/tst-auxobj-dlopen.c
>  create mode 100644 elf/tst-auxobj.c
>  create mode 100644 elf/tst-filterobj-aux.c
>  create mode 100644 elf/tst-filterobj-dlopen.c
>  create mode 100644 elf/tst-filterobj-filtee.c
>  create mode 100644 elf/tst-filterobj-filtee.h
>  create mode 100644 elf/tst-filterobj-flt.c
>  create mode 100644 elf/tst-filterobj.c
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 6c62ed6..0122431 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -201,7 +201,8 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 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-dlopenfail-2
> +	 tst-dlopenfail-2 \
> +	 tst-filterobj tst-filterobj-dlopen tst-auxobj tst-auxobj-dlopen
>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \

Ok.

> @@ -312,7 +313,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-auditmanymod7 tst-auditmanymod8 tst-auditmanymod9 \
>  		tst-initlazyfailmod tst-finilazyfailmod \
>  		tst-dlopenfailmod1 tst-dlopenfaillinkmod tst-dlopenfailmod2 \
> -		tst-dlopenfailmod3 tst-ldconfig-ld-mod
> +		tst-dlopenfailmod3 tst-ldconfig-ld-mod \
> +		tst-filterobj-flt tst-filterobj-aux tst-filterobj-filtee
>  # 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%,\

Ok.

> @@ -1698,3 +1700,15 @@ LDFLAGS-tst-dlopen-nodelete-reloc-mod17.so = -Wl,--no-as-needed
>  
>  $(objpfx)tst-ldconfig-ld_so_conf-update.out: $(objpfx)tst-ldconfig-ld-mod.so
>  $(objpfx)tst-ldconfig-ld_so_conf-update: $(libdl)
> +
> +LDFLAGS-tst-filterobj-flt.so = -Wl,--filter=$(objpfx)tst-filterobj-filtee.so
> +$(objpfx)tst-filterobj: $(objpfx)tst-filterobj-flt.so
> +$(objpfx)tst-filterobj-dlopen: $(libdl)
> +$(objpfx)tst-filterobj.out: $(objpfx)tst-filterobj-filtee.so
> +$(objpfx)tst-filterobj-dlopen.out: $(objpfx)tst-filterobj-filtee.so
> +
> +LDFLAGS-tst-filterobj-aux.so = -Wl,--auxiliary=$(objpfx)tst-filterobj-filtee.so
> +$(objpfx)tst-auxobj: $(objpfx)tst-filterobj-aux.so
> +$(objpfx)tst-auxobj-dlopen: $(libdl)
> +$(objpfx)tst-auxobj.out: $(objpfx)tst-filterobj-filtee.so
> +$(objpfx)tst-auxobj-dlopen.out: $(objpfx)tst-filterobj-filtee.so

Ok.

> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 5103a8a..0730ea9 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -485,14 +485,18 @@ _dl_map_object_deps (struct link_map *map,
>  
>    map->l_searchlist.r_list = &l_initfini[nlist + 1];
>    map->l_searchlist.r_nlist = nlist;
> +  unsigned int map_index = UINT_MAX;
>  
>    for (nlist = 0, runp = known; runp; runp = runp->next)
>      {
>        if (__builtin_expect (trace_mode, 0) && runp->map->l_faked)
>  	/* This can happen when we trace the loading.  */
>  	--map->l_searchlist.r_nlist;
> -      else
> +      else {
> +	if (runp->map == map)
> +	  map_index = nlist;
>  	map->l_searchlist.r_list[nlist++] = runp->map;
> +      }
>  
>        /* Now clear all the mark bits we set in the objects on the search list
>  	 to avoid duplicates, so the next call starts fresh.  */

Ok.

> @@ -550,13 +554,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)

Ok.

> @@ -581,16 +586,30 @@ 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.  */
> +  assert (map_index < nlist);
> +  if (map_index > 0)
> +    {
> +      /* Copy the binary into position 0.  */
> +      l_initfini[0] = map->l_searchlist.r_list[map_index];
> +
> +      /* Copy the filtees.  */
> +      for (i = 0; i < map_index; ++i)
> +	l_initfini[i+1] = map->l_searchlist.r_list[i];
> +
> +      /* Copy the remainder.  */
> +      for (i = map_index + 1; i < nlist; ++i)
> +	l_initfini[i] = map->l_searchlist.r_list[i];
> +    }
> +  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.  */

Ok.

> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 623c975..ecb2ba9 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -621,22 +621,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);

Ok.

> diff --git a/elf/tst-auxobj-dlopen.c b/elf/tst-auxobj-dlopen.c
> new file mode 100644
> index 0000000..7f91bdd
> --- /dev/null
> +++ b/elf/tst-auxobj-dlopen.c
> @@ -0,0 +1,47 @@
> +/* Test for BZ16272, dlopen'ing an auxiliary filter object.

Just a nip and tuck: usually bugzilla reports are references as BZ#NNNNN.

> +   Ensure that symbols from the resolve correctly.
> +
> +   Copyright (C) 2020 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-aux.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)");
> +
> +  fn = xdlsym (lib, "get_text2");
> +  text = fn ();
> +
> +  printf ("%s\n", text);
> +
> +  /* Verify the text matches what we expect from the auxiliary object */
> +  TEST_COMPARE_STRING (text, "Hello from auxiliary filter object (PASS)");
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Ok.

> diff --git a/elf/tst-auxobj.c b/elf/tst-auxobj.c
> new file mode 100644
> index 0000000..bdc7713
> --- /dev/null
> +++ b/elf/tst-auxobj.c
> @@ -0,0 +1,42 @@
> +/* Test that symbols from auxiliary filter objects are resolved to the
> +   filtee.
> +
> +   Copyright (C) 2020 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-filtee.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)");
> +
> +  text = get_text2 ();
> +  printf ("%s\n", text);
> +
> +  /* Verify the text matches what we expect from the auxiliary object */
> +  TEST_COMPARE_STRING (text, "Hello from auxiliary filter object (PASS)");
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

Ok.

> diff --git a/elf/tst-filterobj-aux.c b/elf/tst-filterobj-aux.c
> new file mode 100644
> index 0000000..0b732f2
> --- /dev/null
> +++ b/elf/tst-filterobj-aux.c
> @@ -0,0 +1,33 @@
> +/* Auxiliary filter object.
> +   Contains symbols to be resolved in filtee, and one which doesn't.
> +
> +   Copyright (C) 2020 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-filtee.h"
> +
> +/* We never want to see the output of the auxiliary object.  */
> +const char *get_text (void)
> +{
> +  return "Hello from auxiliary filter object (FAIL)";
> +}
> +
> +/* The filtee doesn't implement this symbol, so this should resolve.  */
> +const char *get_text2 (void)
> +{
> +  return "Hello from auxiliary filter object (PASS)";
> +}

Ok.

> diff --git a/elf/tst-filterobj-dlopen.c b/elf/tst-filterobj-dlopen.c
> new file mode 100644
> index 0000000..089a3e9
> --- /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) 2020 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>

Ok.

> diff --git a/elf/tst-filterobj-filtee.c b/elf/tst-filterobj-filtee.c
> new file mode 100644
> index 0000000..817576b
> --- /dev/null
> +++ b/elf/tst-filterobj-filtee.c
> @@ -0,0 +1,27 @@
> +/* Filtee for BZ16272 test.
> +   Contains desired symbol implementations.
> +
> +   Copyright (C) 2020 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-filtee.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-filtee.h b/elf/tst-filterobj-filtee.h
> new file mode 100644
> index 0000000..69f4749
> --- /dev/null
> +++ b/elf/tst-filterobj-filtee.h
> @@ -0,0 +1,24 @@
> +/* Filtee header for BZ16272 test.
> +   Contains prototypes for symbols implemented in the filtee.
> +
> +   Copyright (C) 2020 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);
> +
> +/* For testing auxiliary filter object.  */
> +const char *get_text2 (void);

Ok.

> diff --git a/elf/tst-filterobj-flt.c b/elf/tst-filterobj-flt.c
> new file mode 100644
> index 0000000..9da5ef1
> --- /dev/null
> +++ b/elf/tst-filterobj-flt.c
> @@ -0,0 +1,27 @@
> +/* Filter object for BZ16272 test.
> +   Contains symbols to be resolved in filtee.
> +
> +   Copyright (C) 2020 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-filtee.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.c b/elf/tst-filterobj.c
> new file mode 100644
> index 0000000..96bfae0
> --- /dev/null
> +++ b/elf/tst-filterobj.c
> @@ -0,0 +1,36 @@
> +/* Test that symbols from filter objects are resolved to the filtee.
> +
> +   Copyright (C) 2020 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-filtee.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 v4 2/3] elf: avoid redundant sort in dlopen

[Adhemerval Zanella]

On 29/01/2020 08:17, 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.

Still LGTM, thanks.

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

> ---
>  elf/dl-open.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index ecb2ba9..314adc2 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -617,9 +617,10 @@ dl_open_worker (void *a)
>    if (GLRO(dl_lazy))
>      reloc_mode |= mode & RTLD_LAZY;
>  
> -  /* 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.  */
> +  /* Objects must be sorted by dependency for the relocation process.
> +     This allows IFUNC relocations to work and it also means copy
> +     relocation of dependencies are if necessary overwritten.
> +     __dl_map_object_deps has already sorted l_initfini for us.  */
>    unsigned int nmaps = 0;
>    unsigned int j = 0;
>    struct link_map *l = new->l_initfini[0];
> @@ -642,7 +643,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 v4 3/3] elf: avoid stack allocation in dl_open_worker

[Adhemerval Zanella]

On 29/01/2020 08:17, 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.

Still 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 314adc2..7b3b177 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -621,25 +621,18 @@ dl_open_worker (void *a)
>       This allows IFUNC relocations to work and it also means copy
>       relocation of dependencies are if necessary overwritten.
>       __dl_map_object_deps has already sorted l_initfini for us.  */
> -  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.

> @@ -654,9 +647,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.

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

[David Kilroy]

Adhemerval Zanella said:
> 
> On 29/01/2020 08:17, 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 relocation 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 auxiliary and filter objects are used, both as a normal link and
> > when dlopen'd.
> >
> > Tested by running the testsuite on x86_64.
> 
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>


<snip>
 
> > diff --git a/elf/tst-auxobj-dlopen.c b/elf/tst-auxobj-dlopen.c
> > new file mode 100644
> > index 0000000..7f91bdd
> > --- /dev/null
> > +++ b/elf/tst-auxobj-dlopen.c
> > @@ -0,0 +1,47 @@
> > +/* Test for BZ16272, dlopen'ing an auxiliary filter object.
> 
> Just a nip and tuck: usually bugzilla reports are references as
> BZ#NNNNN.

Thanks for reviewing! Should I resubmit with this fixed? I'd also need
someone to actually apply the changes (assuming no other objections).


FYI I'll be out of office until next week.


Regards,
Dave.

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

[Adhemerval Zanella]

On 11/02/2020 12:59, David Kilroy wrote:
> 
> Adhemerval Zanella said:
>>
>> On 29/01/2020 08:17, 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 relocation 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 auxiliary and filter objects are used, both as a normal link and
>>> when dlopen'd.
>>>
>>> Tested by running the testsuite on x86_64.
>>
>> LGTM, thanks.
>>
>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> 
> <snip>
>  
>>> diff --git a/elf/tst-auxobj-dlopen.c b/elf/tst-auxobj-dlopen.c
>>> new file mode 100644
>>> index 0000000..7f91bdd
>>> --- /dev/null
>>> +++ b/elf/tst-auxobj-dlopen.c
>>> @@ -0,0 +1,47 @@
>>> +/* Test for BZ16272, dlopen'ing an auxiliary filter object.
>>
>> Just a nip and tuck: usually bugzilla reports are references as
>> BZ#NNNNN.
> 
> Thanks for reviewing! Should I resubmit with this fixed? I'd also need
> someone to actually apply the changes (assuming no other objections).

I can take care of applying this patch and no need to resubmit, I can 
change it locally for you. 

> 
> 
> FYI I'll be out of office until next week.
> 
> 
> Regards,
> Dave.
> 

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

[David Kilroy]

> On 11/02/2020 12:59, David Kilroy wrote:
> >
> > Adhemerval Zanella said:
> >>
> >> On 29/01/2020 08:17, 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 relocation 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 auxiliary and filter objects are used, both as a normal link
> and
> >>> when dlopen'd.
> >>>
> >>> Tested by running the testsuite on x86_64.
> >>
> >> LGTM, thanks.
> >>
> >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> >
> >
> > <snip>
> >
> >>> diff --git a/elf/tst-auxobj-dlopen.c b/elf/tst-auxobj-dlopen.c
> >>> new file mode 100644
> >>> index 0000000..7f91bdd
> >>> --- /dev/null
> >>> +++ b/elf/tst-auxobj-dlopen.c
> >>> @@ -0,0 +1,47 @@
> >>> +/* Test for BZ16272, dlopen'ing an auxiliary filter object.
> >>
> >> Just a nip and tuck: usually bugzilla reports are references as
> >> BZ#NNNNN.
> >
> > Thanks for reviewing! Should I resubmit with this fixed? I'd also
> need
> > someone to actually apply the changes (assuming no other objections).
> 
> I can take care of applying this patch and no need to resubmit, I can
> change it locally for you.
> 

I see the patches have been committed now :)

Thanks for the help!


Dave.

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

[Andreas Schwab]

On Jan 29 2020, David Kilroy wrote:

> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 5103a8a..0730ea9 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -485,14 +485,18 @@ _dl_map_object_deps (struct link_map *map,
>  
>    map->l_searchlist.r_list = &l_initfini[nlist + 1];
>    map->l_searchlist.r_nlist = nlist;
> +  unsigned int map_index = UINT_MAX;
>  
>    for (nlist = 0, runp = known; runp; runp = runp->next)
>      {
>        if (__builtin_expect (trace_mode, 0) && runp->map->l_faked)
>  	/* This can happen when we trace the loading.  */
>  	--map->l_searchlist.r_nlist;
> -      else
> +      else {
> +	if (runp->map == map)
> +	  map_index = nlist;
>  	map->l_searchlist.r_list[nlist++] = runp->map;
> +      }

Wrong indentation.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Adhemerval Zanella]

On 10/03/2020 06:51, Andreas Schwab wrote:
> On Jan 29 2020, David Kilroy wrote:
> 
>> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
>> index 5103a8a..0730ea9 100644
>> --- a/elf/dl-deps.c
>> +++ b/elf/dl-deps.c
>> @@ -485,14 +485,18 @@ _dl_map_object_deps (struct link_map *map,
>>  
>>    map->l_searchlist.r_list = &l_initfini[nlist + 1];
>>    map->l_searchlist.r_nlist = nlist;
>> +  unsigned int map_index = UINT_MAX;
>>  
>>    for (nlist = 0, runp = known; runp; runp = runp->next)
>>      {
>>        if (__builtin_expect (trace_mode, 0) && runp->map->l_faked)
>>  	/* This can happen when we trace the loading.  */
>>  	--map->l_searchlist.r_nlist;
>> -      else
>> +      else {
>> +	if (runp->map == map)
>> +	  map_index = nlist;
>>  	map->l_searchlist.r_list[nlist++] = runp->map;
>> +      }
> 
> Wrong indentation.
> 

I will fix it.

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

[David Kilroy]

Andreas wrote:
> On Jan 29 2020, David Kilroy wrote:
> 
> > diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> > index 5103a8a..0730ea9 100644
> > --- a/elf/dl-deps.c
> > +++ b/elf/dl-deps.c
> > @@ -485,14 +485,18 @@ _dl_map_object_deps (struct link_map *map,
> >
> >    map->l_searchlist.r_list = &l_initfini[nlist + 1];
> >    map->l_searchlist.r_nlist = nlist;
> > +  unsigned int map_index = UINT_MAX;
> >
> >    for (nlist = 0, runp = known; runp; runp = runp->next)
> >      {
> >        if (__builtin_expect (trace_mode, 0) && runp->map->l_faked)
> >  	/* This can happen when we trace the loading.  */
> >  	--map->l_searchlist.r_nlist;
> > -      else
> > +      else {
> > +	if (runp->map == map)
> > +	  map_index = nlist;
> >  	map->l_searchlist.r_list[nlist++] = runp->map;
> > +      }
> 
> Wrong indentation.

Whoops. I'll fix up the breakage.


Apologies,

Dave.

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

[David Kilroy]

> Andreas wrote:
> > On Jan 29 2020, David Kilroy wrote:
> >
> > > diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> > > index 5103a8a..0730ea9 100644
> > > --- a/elf/dl-deps.c
> > > +++ b/elf/dl-deps.c
> > > @@ -485,14 +485,18 @@ _dl_map_object_deps (struct link_map *map,
> > >
> > >    map->l_searchlist.r_list = &l_initfini[nlist + 1];
> > >    map->l_searchlist.r_nlist = nlist;
> > > +  unsigned int map_index = UINT_MAX;
> > >
> > >    for (nlist = 0, runp = known; runp; runp = runp->next)
> > >      {
> > >        if (__builtin_expect (trace_mode, 0) && runp->map->l_faked)
> > >  	/* This can happen when we trace the loading.  */
> > >  	--map->l_searchlist.r_nlist;
> > > -      else
> > > +      else {
> > > +	if (runp->map == map)
> > > +	  map_index = nlist;
> > >  	map->l_searchlist.r_list[nlist++] = runp->map;
> > > +      }
> >
> > Wrong indentation.
> 
> Whoops. I'll fix up the breakage.

... and seen Adhemerval's email that he'll take care of it.


Dave.