[glibc/fw/bug30981] elf: Fix force_first handling in dlclose (bug 30981)

[glibc/fw/bug30981] elf: Fix force_first handling in dlclose (bug 30981)

[Florian Weimer]

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=849274d48fc59bfa6db3c713c8ced8026b20f3b7

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:
---
 elf/dl-close.c           | 23 +++++++++++++++++++----
 elf/dl-sort-maps.c       |  7 +++----
 elf/dso-sort-tests-1.def | 12 +++++++-----
 3 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/elf/dl-close.c b/elf/dl-close.c
index 1c7a861db1..a97a1efa45 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -153,6 +153,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)
@@ -226,9 +236,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;
@@ -732,7 +743,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 5616c8a6a3..5c846c7c6f 100644
--- a/elf/dl-sort-maps.c
+++ b/elf/dl-sort-maps.c
@@ -255,13 +255,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 4bf9052db1..cf6453e9eb 100644
--- a/elf/dso-sort-tests-1.def
+++ b/elf/dso-sort-tests-1.def
@@ -56,14 +56,16 @@ output: b>a>{}<a<b
 # 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[<a<c<d<g<f<b<e];}
-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[<g<f<a<b<c<d<e];}
+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[<a<b<c<d<g<f<e];}
+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[<a<g<f<b<c<d<e];}
 
 # Test that even in the presence of dependency loops involving dlopen'ed
 # object, that object is initialized last (and not unloaded prematurely).