diff mbox

[v2] Also use l_tls_dtor_count to decide on object unload (BZ #18657)

Message ID 1436552195-16464-1-git-send-email-siddhesh@redhat.com
State New
Headers show

Commit Message

Siddhesh Poyarekar July 10, 2015, 6:16 p.m. UTC 
When an TLS destructor is registered, we set the DF_1_NODELETE flag to
signal that the object should not be destroyed.  We then clear the
DF_1_NODELETE flag when all destructors are called, which is wrong -
the flag could have been set by other means too.

This patch replaces this use of the flag by using l_tls_dtor_count
directly to determine whether it is safe to unload the object.  This
change has the added advantage of eliminating the lock taking when
calling the destructors, which could result in a deadlock.  The patch
also fixes the test case tst-tls-atexit - it was making an invalid
dlclose call, which would just return an error silently.

Change verified on x86_64; the test suite does not show any
regressions due to the patch.

ChangeLog:

	[BZ #18657]
	* elf/dl-close.c (_dl_close_worker): Don't unload DSO if there
	are pending TLS destructor calls.
	* stdlib/cxa_thread_atexit_impl.c (__cxa_thread_atexit_impl):
	Don't touch the link map flag.  Atomically increment
	l_tls_dtor_count.
	(__call_tls_dtors): Atomically decrement l_tls_dtor_count.
	Avoid taking the load lock and don't touch the link map flag.
	* stdlib/tst-tls-atexit.c (do_test): dlopen
	tst-tls-atexit-lib.so again before dlclose.
	* stdlib/tst-tls-atexit-nodelete.c: New test case.
	* stdlib/Makefile (tests): Use it.
---
 elf/dl-close.c                   |   9 ++-
 stdlib/Makefile                  |   5 +-
 stdlib/cxa_thread_atexit_impl.c  |  25 +++------
 stdlib/tst-tls-atexit-nodelete.c | 118 +++++++++++++++++++++++++++++++++++++++
 stdlib/tst-tls-atexit.c          |   9 ++-
 5 files changed, 145 insertions(+), 21 deletions(-)
 create mode 100644 stdlib/tst-tls-atexit-nodelete.c

Comments

Ondřej Bílka July 10, 2015, 7:33 p.m. UTC | #1 
On Fri, Jul 10, 2015 at 11:46:35PM +0530, Siddhesh Poyarekar wrote:
> When an TLS destructor is registered, we set the DF_1_NODELETE flag to
> signal that the object should not be destroyed.  We then clear the
> DF_1_NODELETE flag when all destructors are called, which is wrong -
> the flag could have been set by other means too.
> 
> This patch replaces this use of the flag by using l_tls_dtor_count
> directly to determine whether it is safe to unload the object.  This
> change has the added advantage of eliminating the lock taking when
> calling the destructors, which could result in a deadlock.  The patch
> also fixes the test case tst-tls-atexit - it was making an invalid
> dlclose call, which would just return an error silently.
> 
> Change verified on x86_64; the test suite does not show any
> regressions due to the patch.
> 
Does not work. In thread A we register destructor just before atomic
add. Meanwhile we call dlclose in thread B which succeeds as count is
zero. After thread A resumes it causes SIGSEGV when destructor gets
called.
Roland McGrath July 10, 2015, 8:19 p.m. UTC | #2 
> +      size_t tls_dtor_count = atomic_load_relaxed (&l->l_tls_dtor_count);
> +
>        /* Check whether this object is still used.  */
>        if (l->l_type == lt_loaded
>  	  && l->l_direct_opencount == 0
>  	  && (l->l_flags_1 & DF_1_NODELETE) == 0
> +	  && tls_dtor_count == 0
>  	  && !used[done_index])
>  	continue;

IMHO the separate variable here is actually less readable than
just putting atomic_load_relaxed (...) inside the if.

> +static void *
> +use_handle (void *h)
> +{
> +  void (*foo) (void) = (void (*) (void)) dlsym(h, "do_foo");

Space before paren after dlsym.

> +  if (!foo)

No implicit Boolean coercion.

> +  /* Load our module.  We access (and hence construct the object) in the
> +     thread.  */

I'm not sure I understand this comment.  Access what?  Which object?  What
does construct mean here?  The best reading I can guess is, "We call into
the module in the thread created below, so it accesses its TLS and
registers its destructor in that thread."  Is that what it means?

> +  if (thr_ret != NULL)
> +    return 1;

What's the point of this test?  It should be an assert, if anything.
There is no aspect of what's being tested here that affects the return
value of the thread function, is there?

> +  /* This sequence should not unload the DSO.  */
> +  void *h2 = dlopen ("$ORIGIN/tst-tls-atexit-lib.so",
> +		     RTLD_LAZY | RTLD_NODELETE);

Perhaps use a macro for the DSO name string, so it's more obvious that it's
exactly the same thing in both calls.

> +  /* Close the handle we created in the thread.  */
> +  dlclose (handle);
> +  dlclose (h2);

I don't understand this comment.  I guess "create the handle" means "call
dlopen", and both calls are in this function, on the main thread.

> +  /* Run through our maps and ensure that the DSO is unloaded.  */
> +  FILE *f = fopen ("/proc/self/maps", "r");

This is unnecessarily Linux-specific.  I don't think you need to do
external inspection to test this.  You can just do something like save a
pointer from dlsym, and then try to use it and if the module was unloaded
that will crash.

It's also repeating code from tst-tls-atexit.c (which I don't recall
previously noticing was doing this /proc/self/maps funny business that I
don't think is really what we want).  Perhaps these two tests can share
that part of the code?


Thanks,
Roland
Siddhesh Poyarekar July 15, 2015, 10:51 a.m. UTC | #3 
On Fri, Jul 10, 2015 at 09:33:52PM +0200, Ondřej Bílka wrote:
> Does not work. In thread A we register destructor just before atomic
> add. Meanwhile we call dlclose in thread B which succeeds as count is
> zero. After thread A resumes it causes SIGSEGV when destructor gets
> called.

Right, thanks for pointing out.  Let me look at this again.

Siddhesh
diff mbox

Patch

diff --git a/elf/dl-close.c b/elf/dl-close.c
index 2104674..30e30e2 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -153,7 +153,11 @@  _dl_close_worker (struct link_map *map, bool force)
       maps[idx] = l;
       ++idx;
 
-      /* Clear DF_1_NODELETE to force object deletion.  */
+      /* Clear DF_1_NODELETE to force object deletion.  We don't need to touch
+	 l_tls_dtor_count because forced object deletion only happens when an
+	 error occurs during object load.  Destructor registration for TLS
+	 non-POD objects should not have happened till then for this
+	 object.  */
       if (force)
 	l->l_flags_1 &= ~DF_1_NODELETE;
     }
@@ -173,10 +177,13 @@  _dl_close_worker (struct link_map *map, bool force)
 	/* Already handled.  */
 	continue;
 
+      size_t tls_dtor_count = atomic_load_relaxed (&l->l_tls_dtor_count);
+
       /* Check whether this object is still used.  */
       if (l->l_type == lt_loaded
 	  && l->l_direct_opencount == 0
 	  && (l->l_flags_1 & DF_1_NODELETE) == 0
+	  && tls_dtor_count == 0
 	  && !used[done_index])
 	continue;
 
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 7fc5a80..402466a 100644
--- a/stdlib/Makefile
+++ b/stdlib/Makefile
@@ -74,7 +74,7 @@  tests		:= tst-strtol tst-strtod testmb testrand testsort testdiv   \
 		   tst-makecontext3 bug-getcontext bug-fmtmsg1		    \
 		   tst-secure-getenv tst-strtod-overflow tst-strtod-round   \
 		   tst-tininess tst-strtod-underflow tst-tls-atexit	    \
-		   tst-setcontext3
+		   tst-setcontext3 tst-tls-atexit-nodelete
 tests-static	:= tst-secure-getenv
 
 modules-names	= tst-tls-atexit-lib
@@ -159,6 +159,9 @@  tst-tls-atexit-lib.so-no-z-defs = yes
 $(objpfx)tst-tls-atexit: $(shared-thread-library) $(libdl)
 $(objpfx)tst-tls-atexit.out: $(objpfx)tst-tls-atexit-lib.so
 
+$(objpfx)tst-tls-atexit-nodelete: $(shared-thread-library) $(libdl)
+$(objpfx)tst-tls-atexit-nodelete.out: $(objpfx)tst-tls-atexit-lib.so
+
 $(objpfx)tst-setcontext3.out: tst-setcontext3.sh $(objpfx)tst-setcontext3
 	$(SHELL) $< $(common-objpfx) '$(test-program-prefix-before-env)' \
 		 '$(run-program-env)' '$(test-program-prefix-after-env)' \
diff --git a/stdlib/cxa_thread_atexit_impl.c b/stdlib/cxa_thread_atexit_impl.c
index 9120162..fac2cc9 100644
--- a/stdlib/cxa_thread_atexit_impl.c
+++ b/stdlib/cxa_thread_atexit_impl.c
@@ -50,27 +50,25 @@  __cxa_thread_atexit_impl (dtor_func func, void *obj, void *dso_symbol)
   tls_dtor_list = new;
 
   /* See if we already encountered the DSO.  */
-  __rtld_lock_lock_recursive (GL(dl_load_lock));
-
   if (__glibc_unlikely (dso_symbol_cache != dso_symbol))
     {
       ElfW(Addr) caller = (ElfW(Addr)) dso_symbol;
 
+      /* _dl_find_dso_for_object assumes that we have the dl_load_lock.  */
+      __rtld_lock_lock_recursive (GL(dl_load_lock));
       struct link_map *l = _dl_find_dso_for_object (caller);
+      __rtld_lock_unlock_recursive (GL(dl_load_lock));
 
       /* If the address is not recognized the call comes from the main
          program (we hope).  */
       lm_cache = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded;
     }
+
   /* A destructor could result in a thread_local construction and the former
      could have cleared the flag.  */
-  if (lm_cache->l_type == lt_loaded && lm_cache->l_tls_dtor_count == 0)
-    lm_cache->l_flags_1 |= DF_1_NODELETE;
+  atomic_increment (&lm_cache->l_tls_dtor_count);
 
   new->map = lm_cache;
-  new->map->l_tls_dtor_count++;
-
-  __rtld_lock_unlock_recursive (GL(dl_load_lock));
 
   return 0;
 }
@@ -83,19 +81,10 @@  __call_tls_dtors (void)
   while (tls_dtor_list)
     {
       struct dtor_list *cur = tls_dtor_list;
-      tls_dtor_list = tls_dtor_list->next;
 
+      tls_dtor_list = tls_dtor_list->next;
       cur->func (cur->obj);
-
-      __rtld_lock_lock_recursive (GL(dl_load_lock));
-
-      /* Allow DSO unload if count drops to zero.  */
-      cur->map->l_tls_dtor_count--;
-      if (cur->map->l_tls_dtor_count == 0 && cur->map->l_type == lt_loaded)
-        cur->map->l_flags_1 &= ~DF_1_NODELETE;
-
-      __rtld_lock_unlock_recursive (GL(dl_load_lock));
-
+      atomic_decrement (&cur->map->l_tls_dtor_count);
       free (cur);
     }
 }
diff --git a/stdlib/tst-tls-atexit-nodelete.c b/stdlib/tst-tls-atexit-nodelete.c
new file mode 100644
index 0000000..93daefc
--- /dev/null
+++ b/stdlib/tst-tls-atexit-nodelete.c
@@ -0,0 +1,118 @@ 
+/* Verify that a RTLD_NODELETE DSO is not unloaded even if its TLS objects are
+   destroyed.
+
+   Copyright (C) 2015 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <dlfcn.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+
+static void *
+use_handle (void *h)
+{
+  void (*foo) (void) = (void (*) (void)) dlsym(h, "do_foo");
+
+  if (!foo)
+    {
+      printf ("Unable to find symbol: %s\n", dlerror ());
+      exit (1);
+    }
+
+  foo ();
+
+  return NULL;
+}
+
+static int
+do_test (void)
+{
+  pthread_t t;
+  int ret;
+  void *thr_ret;
+
+  /* Load our module.  We access (and hence construct the object) in the
+     thread.  */
+  void *handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
+  if (handle == NULL)
+    {
+      printf ("Unable to load DSO: %s\n", dlerror ());
+      return 1;
+    }
+
+  if ((ret = pthread_create (&t, NULL, use_handle, handle)) != 0)
+    {
+      printf ("pthread_create failed: %s\n", strerror (ret));
+      return 1;
+    }
+
+  if ((ret = pthread_join (t, &thr_ret)) != 0)
+    {
+      printf ("pthread_create failed: %s\n", strerror (ret));
+      return 1;
+    }
+
+  if (thr_ret != NULL)
+    return 1;
+
+  /* This sequence should not unload the DSO.  */
+  void *h2 = dlopen ("$ORIGIN/tst-tls-atexit-lib.so",
+		     RTLD_LAZY | RTLD_NODELETE);
+  if (h2 == NULL)
+    {
+      printf ("main thread: Unable to load DSO: %s\n", dlerror ());
+      return 1;
+    }
+
+  use_handle (h2);
+
+  /* Close the handle we created in the thread.  */
+  dlclose (handle);
+  dlclose (h2);
+
+  /* Run through our maps and ensure that the DSO is unloaded.  */
+  FILE *f = fopen ("/proc/self/maps", "r");
+
+  if (f == NULL)
+    {
+      perror ("Failed to open /proc/self/maps");
+      fprintf (stderr, "Skipping verification of DSO unload\n");
+      return 0;
+    }
+
+  char *line = NULL;
+  size_t s = 0;
+  while (getline (&line, &s, f) > 0)
+    {
+      if (strstr (line, "tst-tls-atexit-lib.so"))
+        {
+	  printf ("DSO not unloaded yet:\n%s", line);
+	  return 0;
+	}
+    }
+  free (line);
+
+  /* The module was unloaded even when we specified RTLD_NODELETE.  FAIL.  */
+  printf ("tst-tls-atexit-lib.so was unloaded.\n");
+  return 1;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
index 5ee04a8..374470e 100644
--- a/stdlib/tst-tls-atexit.c
+++ b/stdlib/tst-tls-atexit.c
@@ -79,7 +79,14 @@  do_test (void)
   if (thr_ret != NULL)
     return 1;
 
-  /* Now this should unload the DSO.  */
+  /* Now this sequence should unload the DSO.  */
+  handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
+  if (handle == NULL)
+    {
+      printf ("main thread: Unable to load DSO: %s\n", dlerror ());
+      return 1;
+    }
+
   dlclose (handle);
 
   /* Run through our maps and ensure that the DSO is unloaded.  */