diff mbox series

elf: Fix force_first handling in dlclose (bug 30785)

Message ID 8734y1wpbp.fsf@oldenburg.str.redhat.com
State New
Headers show
Series elf: Fix force_first handling in dlclose (bug 30785) | expand

Commit Message

Florian Weimer Oct. 23, 2023, 11:28 a.m. UTC 
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.

Tested on x86_64-linux-gnu.  We have pushed this change into several
Fedora releases recently, and initial feedback has been positive, both
from automated testing and end users.  We have also verified that it
fixes the reported third-party application issue.  I hope this is going
to be the final change in this area for a while.

Thanks,
Florian

---
 elf/dl-close.c           | 21 +++++++++++++++++----
 elf/dl-sort-maps.c       |  7 +++----
 elf/dso-sort-tests-1.def | 12 +++++++-----
 3 files changed, 27 insertions(+), 13 deletions(-)


base-commit: 2aa0974d2573441bffd596b07bff8698b1f2f18c

Comments

Florian Weimer Oct. 25, 2023, 3:56 p.m. UTC | #1 
* Florian Weimer:

> 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.
>
> Tested on x86_64-linux-gnu.  We have pushed this change into several
> Fedora releases recently, and initial feedback has been positive, both
> from automated testing and end users.  We have also verified that it
> fixes the reported third-party application issue.  I hope this is going
> to be the final change in this area for a while.

This change exposes a use-after-free bug in dlclose if recursive dlclose
is involved, which I believe is pre-existing.  I'll try to create a test
case for it.

Thanks,
Florian
diff mbox series

Patch

diff --git a/elf/dl-close.c b/elf/dl-close.c
index 1c7a861db1..3bbb9d07c4 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -153,6 +153,14 @@  _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[0] = map;
+    }
+
   /* Keep track of the lowest index link map we have covered already.  */
   int done_index = -1;
   while (++done_index < nloaded)
@@ -226,9 +234,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 +741,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).