commit 849274d48fc59bfa6db3c713c8ced8026b20f3b7

Author: Florian Weimer <fweimer@redhat.com>

Date:   Thu Nov 16 19:55:35 2023 +0100

    elf: Fix force_first handling in dlclose (bug 30981)

    The force_first parameter was ineffective because the dlclose'd

    object was not necessarily the first in the maps array.  Also

    enable force_first handling unconditionally, regardless of namespace.

    The initial object in a namespace should be destructed first, too.

    The _dl_sort_maps_dfs function had early returns for relocation

    dependency processing which broke force_first handling, too, and

    this is fixed in this change as well.

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

diff --git a/elf/dl-close.c b/elf/dl-close.c

index 66524b6708c59f29..8107c2d5f6ad2bc6 100644

--- a/elf/dl-close.c

+++ b/elf/dl-close.c

@@ -182,6 +182,16 @@ _dl_close_worker (struct link_map *map, bool force)

     }

   assert (idx == nloaded);

+  /* Put the dlclose'd map first, so that its destructor runs first.

+     The map variable is NULL after a retry.  */

+  if (map != NULL)

+    {

+      maps[map->l_idx] = maps[0];

+      maps[map->l_idx]->l_idx = map->l_idx;

+      maps[0] = map;

+      maps[0]->l_idx = 0;

+    }

+

   /* Keep track of the lowest index link map we have covered already.  */

   int done_index = -1;

   while (++done_index < nloaded)

@@ -255,9 +265,10 @@ _dl_close_worker (struct link_map *map, bool force)

 	  }

     }

-  /* Sort the entries.  We can skip looking for the binary itself which is

-     at the front of the search list for the main namespace.  */

-  _dl_sort_maps (maps, nloaded, (nsid == LM_ID_BASE), true);

+  /* Sort the entries.  Unless retrying, the maps[0] object (the

+     original argument to dlclose) needs to remain first, so that its

+     destructor runs first.  */

+  _dl_sort_maps (maps, nloaded, /* force_first */ map != NULL, true);

   /* Call all termination functions at once.  */

   bool unload_any = false;

@@ -768,7 +779,11 @@ _dl_close_worker (struct link_map *map, bool force)

   /* Recheck if we need to retry, release the lock.  */

  out:

   if (dl_close_state == rerun)

-    goto retry;

+    {

+      /* The map may have been deallocated.  */

+      map = NULL;

+      goto retry;

+    }

   dl_close_state = not_pending;

 }

diff --git a/elf/dl-sort-maps.c b/elf/dl-sort-maps.c

index aeb79b40b45054c0..c17ac325eca658ef 100644

--- a/elf/dl-sort-maps.c

+++ b/elf/dl-sort-maps.c

@@ -260,13 +260,12 @@ _dl_sort_maps_dfs (struct link_map **maps, unsigned int nmaps,

 	     The below memcpy is not needed in the do_reldeps case here,

 	     since we wrote back to maps[] during DFS traversal.  */

 	  if (maps_head == maps)

-	    return;

+	    break;

 	}

       assert (maps_head == maps);

-      return;

     }

-

-  memcpy (maps, rpo, sizeof (struct link_map *) * nmaps);

+  else

+    memcpy (maps, rpo, sizeof (struct link_map *) * nmaps);

   /* Skipping the first object at maps[0] is not valid in general,

      since traversing along object dependency-links may "find" that

diff --git a/elf/dso-sort-tests-1.def b/elf/dso-sort-tests-1.def

index 4bf9052db16fb352..cf6453e9eb85ac65 100644

--- a/elf/dso-sort-tests-1.def

+++ b/elf/dso-sort-tests-1.def

@@ -56,14 +56,16 @@ output: b>a>{}

 # relocation(dynamic) dependencies. While this is technically unspecified, the

 # presumed reasonable practical behavior is for the destructor order to respect

 # the static DT_NEEDED links (here this means the a->b->c->d order).

-# The older dynamic_sort=1 algorithm does not achieve this, while the DFS-based

-# dynamic_sort=2 algorithm does, although it is still arguable whether going

-# beyond spec to do this is the right thing to do.

+# The older dynamic_sort=1 algorithm originally did not achieve this,

+# but this was a bug in the way _dl_sort_maps was called from _dl_close_worker,

+# effectively disabling proper force_first handling.

+# The new dynamic_sort=2 algorithm shows the effect of the simpler force_first

+# handling: the a object is simply moved to the front.

 # The below expected outputs are what the two algorithms currently produce

 # respectively, for regression testing purposes.

 tst-bz15311: {+a;+e;+f;+g;+d;%d;-d;-g;-f;-e;-a};a->b->c->d;d=>[ba];c=>a;b=>e=>a;c=>f=>b;d=>g=>c

-output(glibc.rtld.dynamic_sort=1): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[

-output(glibc.rtld.dynamic_sort=2): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[

+output(glibc.rtld.dynamic_sort=1): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[

+output(glibc.rtld.dynamic_sort=2): {+a[d>c>b>a>];+e[e>];+f[f>];+g[g>];+d[];%d(b(e(a()))a()g(c(a()f(b(e(a()))))));-d[];-g[];-f[];-e[];-a[

 # Test that even in the presence of dependency loops involving dlopen'ed

 # object, that object is initialized last (and not unloaded prematurely).