This is the mail archive of the libc-hacker@sources.redhat.com mailing list for the glibc project.

Note that libc-hacker is a closed list. You may look at the archives of this list, but subscription and posting are not open.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix recursive dlclose


Hi!

The attached testcase segfaults in recent glibcs.
The problem is that if dlclose is called recursively (a destructor called
from _dl_close either directly or indirectly calls dlclose) and the inner
_dl_close decides it needs to GC, unreferenced libraries are garbage
collected/unloaded in the inner _dl_close, although they could still
be used (both by the GC code in the parent _dl_close or in this particular
testcase by caller of the inner dlclose being in otherwise unused library).

The patch below seems to fix this by not doing garbage collection
recursively, instead simply recording that GC needs to be rerun whenever
the previous GC phase finishes and returning.

Also, it changes what I think was a bug.  Garbage collection was done if
a lt_loaded library dropped its l_direct_opencount to 0 or 1 (i.e. was
previously used once or twice).  For just one previous use that's what needs
to happen, but I don't see the point of doing this if it was used twice
before.

This patch relies on l_scope[idx] being always either a pointer to
its own l_symbolic_searchlist, or its own or some other link_map's
l_searchlist (because we should no longer use MAP in the GC code
- as the deferred GC phases will collect even libraries unloaded by
some other dlclose).  I have grepped the source and that seems to be the
case.

2005-04-26  Jakub Jelinek  <jakub@redhat.com>

	* elf/dl-close.c: Include stddef.h.
	(_dl_close): If called recursively, just remember GC needs to be rerun
	and decrease l_direct_opencount.  Avoid GC if l_direct_opencount
	decreased to 1.  Rerun GC at the end if any destructor unloaded some
	additional libraries.
	* elf/Makefile: Add rules to build and run unload6 test.
	* elf/unload6.c: New test.
	* elf/unload6mod1.c: New file.
	* elf/unload6mod2.c: New file.
	* elf/unload6mod3.c: New file.

--- libc/elf/dl-close.c.jj	2005-04-13 21:28:20.000000000 +0200
+++ libc/elf/dl-close.c	2005-04-26 15:19:48.000000000 +0200
@@ -20,6 +20,7 @@
 #include <assert.h>
 #include <dlfcn.h>
 #include <libintl.h>
+#include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -105,10 +106,6 @@ _dl_close (void *_map)
   struct link_map *map = _map;
   Lmid_t ns = map->l_ns;
   unsigned int i;
-#ifdef USE_TLS
-  bool any_tls = false;
-#endif
-
   /* First see whether we can remove the object at all.  */
   if (__builtin_expect (map->l_flags_1 & DF_1_NODELETE, 0)
       && map->l_init_called)
@@ -124,9 +121,17 @@ _dl_close (void *_map)
   /* One less direct use.  */
   --map->l_direct_opencount;
 
-  /* Decrement the reference count.  */
-  if (map->l_direct_opencount > 1 || map->l_type != lt_loaded)
+  /* If _dl_close is called recursively (some destructor call dlclose),
+     just record that the parent _dl_close will need to do garbage collection
+     again and return.  */
+  static enum { not_pending, pending, rerun } dl_close_state;
+
+  if (map->l_direct_opencount > 0 || map->l_type != lt_loaded
+      || dl_close_state != not_pending)
     {
+      if (map->l_direct_opencount == 0 && map->l_type == lt_loaded)
+	dl_close_state = rerun;
+
       /* There are still references to this object.  Do nothing more.  */
       if (__builtin_expect (GLRO(dl_debug_mask) & DL_DEBUG_FILES, 0))
 	_dl_debug_printf ("\nclosing file=%s; direct_opencount=%u\n",
@@ -136,12 +141,18 @@ _dl_close (void *_map)
       return;
     }
 
+ retry:
+  dl_close_state = pending;
+
+#ifdef USE_TLS
+  bool any_tls = false;
+#endif
   const unsigned int nloaded = GL(dl_ns)[ns]._ns_nloaded;
   char used[nloaded];
   char done[nloaded];
   struct link_map *maps[nloaded];
 
-  /* Run over the list and assign indeces to the link maps and enter
+  /* Run over the list and assign indexes to the link maps and enter
      them into the MAPS array.  */
   int idx = 0;
   for (struct link_map *l = GL(dl_ns)[ns]._ns_loaded; l != NULL; l = l->l_next)
@@ -302,7 +313,7 @@ _dl_close (void *_map)
 	  if (imap->l_searchlist.r_list == NULL
 	      && imap->l_initfini != NULL)
 	    {
-	      /* The object is still used.  But the object we are
+	      /* The object is still used.  But one of the objects we are
 		 unloading right now is responsible for loading it.  If
 		 the current object does not have it's own scope yet we
 		 have to create one.  This has to be done before running
@@ -318,15 +329,27 @@ _dl_close (void *_map)
 	      imap->l_searchlist.r_nlist = cnt;
 
 	      for (cnt = 0; imap->l_scope[cnt] != NULL; ++cnt)
-		if (imap->l_scope[cnt] == &map->l_searchlist)
+		/* This relies on l_scope[] entries being always set either
+		   to its own l_symbolic_searchlist address, or some other map's
+		   l_searchlist address.  */
+		if (imap->l_scope[cnt] != &imap->l_symbolic_searchlist)
 		  {
-		    imap->l_scope[cnt] = &imap->l_searchlist;
-		    break;
+		    struct link_map *tmap;
+
+		    tmap = (struct link_map *) ((char *) imap->l_scope[cnt]
+						- offsetof (struct link_map,
+							    l_searchlist));
+		    assert (tmap->l_ns == ns);
+		    if (tmap->l_idx != -1)
+		      {
+			imap->l_scope[cnt] = &imap->l_searchlist;
+			break;
+		      }
 		  }
 	    }
 
 	  /* The loader is gone, so mark the object as not having one.
-	     Note: l_idx == -1 -> object will be removed.  */
+	     Note: l_idx != -1 -> object will be removed.  */
 	  if (imap->l_loader != NULL && imap->l_loader->l_idx != -1)
 	    imap->l_loader = NULL;
 
@@ -583,8 +606,12 @@ _dl_close (void *_map)
   r->r_state = RT_CONSISTENT;
   _dl_debug_state ();
 
-  /* Release the lock.  */
+  /* Recheck if we need to retry, release the lock.  */
  out:
+  if (dl_close_state == rerun)
+    goto retry;
+
+  dl_close_state = not_pending;
   __rtld_lock_unlock_recursive (GL(dl_load_lock));
 }
 
@@ -654,7 +681,7 @@ libc_freeres_fn (free_mem)
 	free_slotinfo (&GL(dl_tls_dtv_slotinfo_list));
       else
 # endif
-        /* The first element of the list does not have to be deallocated.
+	/* The first element of the list does not have to be deallocated.
 	   It was allocated in the dynamic linker (i.e., with a different
 	   malloc), and in the static library it's in .bss space.  */
 	free_slotinfo (&GL(dl_tls_dtv_slotinfo_list)->next);
--- libc/elf/unload6.c.jj	2005-04-26 15:47:33.000000000 +0200
+++ libc/elf/unload6.c	2005-04-26 15:49:18.000000000 +0200
@@ -0,0 +1,30 @@
+#include <dlfcn.h>
+#include <stdio.h>
+
+int
+main (void)
+{
+  void *h = dlopen ("unload6mod1.so", RTLD_LAZY);
+  if (h == NULL)
+    {
+      puts ("dlopen unload6mod1.so failed");
+      return 1;
+    }
+
+  int (*fn) (int);
+  fn = dlsym (h, "foo");
+  if (fn == NULL)
+    {
+      puts ("dlsym failed");
+      return 1;
+    }
+
+  int val = fn (16);
+  if (val != 24)
+    {
+      printf ("foo returned %d != 24\n", val);
+      return 1;
+    }
+
+  return 0;
+}
--- libc/elf/Makefile.jj	2005-04-13 21:28:20.000000000 +0200
+++ libc/elf/Makefile	2005-04-26 15:58:11.000000000 +0200
@@ -86,7 +86,7 @@ distribute	:= rtld-Rules \
 		   tst-deep1mod1.c tst-deep1mod2.c tst-deep1mod3.c \
 		   unload3mod1.c unload3mod2.c unload3mod3.c unload3mod4.c \
 		   unload4mod1.c unload4mod2.c unload4mod3.c unload4mod4.c \
-		   tst-auditmod1.c \
+		   unload6mod1.c unload6mod2.c unload6mod3.c tst-auditmod1.c \
 		   order2mod1.c order2mod2.c order2mod3.c order2mod4.c
 
 CFLAGS-dl-runtime.c = -fexceptions -fasynchronous-unwind-tables
@@ -162,7 +162,7 @@ tests += loadtest restest1 preloadtest l
 	 tst-tls10 tst-tls11 tst-tls12 tst-tls13 tst-tls14 tst-tls15 tst-align \
 	 tst-align2 $(tests-execstack-$(have-z-execstack)) tst-dlmodcount \
 	 tst-dlopenrpath tst-deep1 tst-dlmopen1 tst-dlmopen2 tst-dlmopen3 \
-	 unload3 unload4 unload5 tst-audit1 tst-global1 order2
+	 unload3 unload4 unload5 unload6 tst-audit1 tst-global1 order2
 #	 reldep9
 test-srcs = tst-pathopt
 tests-vis-yes = vismain
@@ -201,6 +201,7 @@ modules-names = testobj1 testobj2 testob
 		tst-dlmopen1mod tst-auditmod1 \
 		unload3mod1 unload3mod2 unload3mod3 unload3mod4 \
 		unload4mod1 unload4mod2 unload4mod3 unload4mod4 \
+		unload6mod1 unload6mod2 unload6mod3 \
 		order2mod1 order2mod2 order2mod3 order2mod4
 ifeq (yes,$(have-initfini-array))
 modules-names += tst-array2dep
@@ -438,6 +439,9 @@ $(objpfx)unload3mod2.so: $(objpfx)unload
 $(objpfx)unload3mod3.so: $(objpfx)unload3mod4.so
 $(objpfx)unload4mod1.so: $(objpfx)unload4mod2.so $(objpfx)unload4mod3.so
 $(objpfx)unload4mod2.so: $(objpfx)unload4mod4.so $(objpfx)unload4mod3.so
+$(objpfx)unload6mod1.so: $(libdl)
+$(objpfx)unload6mod2.so: $(libdl)
+$(objpfx)unload6mod3.so: $(libdl)
 
 LDFLAGS-tst-tlsmod5.so = -nostdlib
 LDFLAGS-tst-tlsmod6.so = -nostdlib
@@ -710,6 +714,10 @@ $(objpfx)unload5: $(libdl)
 $(objpfx)unload5.out: $(objpfx)unload3mod1.so $(objpfx)unload3mod2.so \
 		      $(objpfx)unload3mod3.so $(objpfx)unload3mod4.so
 
+$(objpfx)unload6: $(libdl)
+$(objpfx)unload6.out: $(objpfx)unload6mod1.so $(objpfx)unload6mod2.so \
+		      $(objpfx)unload6mod3.so
+
 ifdef libdl
 $(objpfx)tst-tls9-static: $(common-objpfx)dlfcn/libdl.a
 $(objpfx)tst-tls9-static.out: $(objpfx)tst-tlsmod5.so $(objpfx)tst-tlsmod6.so
--- libc/elf/unload6mod1.c.jj	2005-04-26 15:49:42.000000000 +0200
+++ libc/elf/unload6mod1.c	2005-04-26 15:52:29.000000000 +0200
@@ -0,0 +1,16 @@
+#include <dlfcn.h>
+#include <stdio.h>
+
+int
+foo (int i)
+{
+  void *h = dlopen ("unload6mod2.so", RTLD_LAZY);
+  if (h == NULL)
+    {
+      puts ("dlopen unload6mod2.so failed");
+      return 1;
+    }
+
+  dlclose (h);
+  return i + 8;
+}
--- libc/elf/unload6mod2.c.jj	2005-04-26 15:49:42.000000000 +0200
+++ libc/elf/unload6mod2.c	2005-04-26 16:00:58.000000000 +0200
@@ -0,0 +1,23 @@
+#include <dlfcn.h>
+#include <stdio.h>
+#include <unistd.h>
+
+static void *h;
+
+static void __attribute__((constructor))
+mod2init (void)
+{
+  h = dlopen ("unload6mod3.so", RTLD_LAZY);
+  if (h == NULL)
+    {
+      puts ("dlopen unload6mod3.so failed");
+      fflush (stdout);
+      _exit (1);
+    }
+}
+
+static void __attribute__((destructor))
+mod2fini (void)
+{
+  dlclose (h);
+}
--- libc/elf/unload6mod3.c.jj	2005-04-26 15:49:42.000000000 +0200
+++ libc/elf/unload6mod3.c	2005-04-26 16:01:06.000000000 +0200
@@ -0,0 +1,23 @@
+#include <dlfcn.h>
+#include <stdio.h>
+#include <unistd.h>
+
+static void *h;
+
+static void __attribute__((constructor))
+mod3init (void)
+{
+  h = dlopen ("unload6mod1.so", RTLD_LAZY);
+  if (h == NULL)
+    {
+      puts ("dlopen unload6mod1.so failed");
+      fflush (stdout);
+      _exit (1);
+    }
+}
+
+static void __attribute__((destructor))
+mod3fini (void)
+{
+  dlclose (h);
+}

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]