[PATCH] Remove Linuxism from tst-tls-atexit

[PATCH] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

The tst-tls-atexit test case searches for its module in /proc/PID/maps
to verify that it is unloaded, which is a Linux-specific test.  This
patch makes the test generic by trying to call foo (the symbol
obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
occurred.

Verified that the test continues to succeed on x86_64.  There is a bug
in the test case where it calls dlclose once again, which is actually
incorrect but still manages to unload the DSO thanks to an existing
bug in __tls_call_dtors.  This will be fixed in a later patch which
also fixes up the __cxa_thread_atexit_impl implementation.

	* stdlib/tst-tls-atexit.c: Include setjmp.h
	(foo): Move out from LOAD.
	(env): New variable.
	(segv_handler): New function.
	(load): Make static.
	(do_test): Install SIGSEGV handler and test for call to FOO.
---
 stdlib/tst-tls-atexit.c | 53 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 18 deletions(-)

diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
index 974264d..16b208d 100644
--- a/stdlib/tst-tls-atexit.c
+++ b/stdlib/tst-tls-atexit.c
@@ -27,10 +27,20 @@
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
+#include <setjmp.h>
 
-void *handle;
+static void *handle;
+static void (*foo) (void);
+static jmp_buf env;
 
-void *
+static void
+segv_handler (int sig)
+{
+  /* All good. */
+  longjmp (env, 1);
+}
+
+static void *
 load (void *u)
 {
   handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
@@ -40,7 +50,7 @@ load (void *u)
       return (void *) (uintptr_t) 1;
     }
 
-  void (*foo) (void) = (void (*) (void)) dlsym (handle, "do_foo");
+  foo = (void (*) (void)) dlsym (handle, "do_foo");
 
   if (foo == NULL)
     {
@@ -82,29 +92,36 @@ do_test (void)
   /* Now this should unload the DSO.  */
   dlclose (handle);
 
-  /* Run through our maps and ensure that the DSO is unloaded.  */
-  FILE *f = fopen ("/proc/self/maps", "r");
-
-  if (f == NULL)
+  /* Handle SIGSEGV.  We don't use the EXPECTED_SIGNAL macro because we want to
+     detect any other SIGSEGVs as failures.  */
+  struct sigaction sa, old_sa;
+  sa.sa_handler = segv_handler;
+  sigemptyset (&sa.sa_mask);
+  sa.sa_flags = 0;
+  if (sigaction (SIGSEGV, &sa, &old_sa) < 0)
     {
-      perror ("Failed to open /proc/self/maps");
-      fprintf (stderr, "Skipping verification of DSO unload\n");
-      return 0;
+      printf ("Failed to set SIGSEGV handler: %m\n");
+      return 1;
     }
 
-  char *line = NULL;
-  size_t s = 0;
-  while (getline (&line, &s, f) > 0)
+  if (setjmp (env) != 0)
     {
-      if (strstr (line, "tst-tls-atexit-lib.so"))
-        {
-	  printf ("DSO not unloaded yet:\n%s", line);
+      /* Restore the default handler so that we may catch any SIGSEGVs
+	 later.  */
+      if (sigaction (SIGSEGV, &old_sa, NULL) < 0)
+	{
+	  printf ("Failed to restore SIGSEGV handler: %m\n");
 	  return 1;
 	}
+
+      return 0;
     }
-  free (line);
 
-  return 0;
+  /* This should crash...  */
+  foo ();
+
+  /* ... or else we fail.  */
+  return 1;
 }
 
 #define TEST_FUNCTION do_test ()
-- 
2.4.3

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Andreas Schwab]

"Siddhesh Poyarekar" <siddhesh@redhat.com> writes:

> +static void
> +segv_handler (int sig)
> +{
> +  /* All good. */
> +  longjmp (env, 1);

longjmp isn't async-signal-safe.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Zack Weinberg]

On Tue, Jul 14, 2015 at 1:39 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> "Siddhesh Poyarekar" <siddhesh@redhat.com> writes:
>> +static void
>> +segv_handler (int sig)
>> +{
>> +  /* All good. */
>> +  longjmp (env, 1);
>
> longjmp isn't async-signal-safe.

This usage pattern does appear not to be guaranteed to work in POSIX,
but it is common enough that I don't think we actually need to worry
about it.

It should probably be `sigsetjmp` and `siglongjmp` though, to avoid
leaving SIGSEGV blocked.

zw

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Roland McGrath]

> The tst-tls-atexit test case searches for its module in /proc/PID/maps
> to verify that it is unloaded, which is a Linux-specific test.  This
> patch makes the test generic by trying to call foo (the symbol
> obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
> occurred.

Is there a reason to catch it instead of just setting EXPECTED_SIGNAL?

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

On Tue, Jul 14, 2015 at 01:55:17PM -0400, Zack Weinberg wrote:
> This usage pattern does appear not to be guaranteed to work in POSIX,
> but it is common enough that I don't think we actually need to worry
> about it.
> 
> It should probably be `sigsetjmp` and `siglongjmp` though, to avoid
> leaving SIGSEGV blocked.

Ugh, of course.  I'll fix up the patch.

Thanks,
Siddhesh

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

On Tue, Jul 14, 2015 at 12:40:25PM -0700, Roland McGrath wrote:
> > The tst-tls-atexit test case searches for its module in /proc/PID/maps
> > to verify that it is unloaded, which is a Linux-specific test.  This
> > patch makes the test generic by trying to call foo (the symbol
> > obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
> > occurred.
> 
> Is there a reason to catch it instead of just setting EXPECTED_SIGNAL?

EXPECTED_SIGNAL makes SIGSEGV OK for the entire test case and we don't
want that.  The dlclose inside the thread function LOAD for example
will cause a segfault in __tls_call_dtors if it incorrectly unloads
the DSO.

Siddhesh

[PATCH v2] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

The tst-tls-atexit test case searches for its module in /proc/PID/maps
to verify that it is unloaded, which is a Linux-specific test.  This
patch makes the test generic by trying to call foo (the symbol
obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
occurred.

Verified that the test continues to succeed on x86_64.  There is a bug
in the test case where it calls dlclose once again, which is actually
incorrect but still manages to unload the DSO thanks to an existing
bug in __tls_call_dtors.  This will be fixed in a later patch which
also fixes up the __cxa_thread_atexit_impl implementation.

v2: Replaced setjmp/longjmp with a simple _exit.

	* stdlib/tst-tls-atexit.c: Include setjmp.h
	(foo): Move out from LOAD.
	(env): New variable.
	(segv_handler): New function.
	(load): Make static.
	(do_test): Install SIGSEGV handler and test for call to FOO.
---
 stdlib/tst-tls-atexit.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
index 0c6c499..cd3e2b3 100644
--- a/stdlib/tst-tls-atexit.c
+++ b/stdlib/tst-tls-atexit.c
@@ -28,9 +28,17 @@
 #include <string.h>
 #include <errno.h>
 
-void *handle;
+static void *handle;
+static void (*foo) (void);
 
-void *
+static void
+segv_handler (int sig)
+{
+  /* All good. */
+  _exit (0);
+}
+
+static void *
 load (void *u)
 {
   handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
@@ -40,7 +48,7 @@ load (void *u)
       return (void *) (uintptr_t) 1;
     }
 
-  void (*foo) (void) = (void (*) (void)) dlsym (handle, "do_foo");
+  foo = (void (*) (void)) dlsym (handle, "do_foo");
 
   if (foo == NULL)
     {
@@ -82,29 +90,23 @@ do_test (void)
   /* Now this should unload the DSO.  */
   dlclose (handle);
 
-  /* Run through our maps and ensure that the DSO is unloaded.  */
-  FILE *f = fopen ("/proc/self/maps", "r");
-
-  if (f == NULL)
+  /* Handle SIGSEGV.  We don't use the EXPECTED_SIGNAL macro because we want to
+     detect any other SIGSEGVs as failures.  */
+  struct sigaction sa, old_sa;
+  sa.sa_handler = segv_handler;
+  sigemptyset (&sa.sa_mask);
+  sa.sa_flags = 0;
+  if (sigaction (SIGSEGV, &sa, &old_sa) < 0)
     {
-      perror ("Failed to open /proc/self/maps");
-      fprintf (stderr, "Skipping verification of DSO unload\n");
-      return 0;
+      printf ("Failed to set SIGSEGV handler: %m\n");
+      return 1;
     }
 
-  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 1;
-	}
-    }
-  free (line);
+  /* This should crash...  */
+  foo ();
 
-  return 0;
+  /* ... or else we fail.  */
+  return 1;
 }
 
 #define TEST_FUNCTION do_test ()
-- 
2.4.3

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Roland McGrath]

> EXPECTED_SIGNAL makes SIGSEGV OK for the entire test case and we don't
> want that.  The dlclose inside the thread function LOAD for example
> will cause a segfault in __tls_call_dtors if it incorrectly unloads
> the DSO.

Hmm.  Perhaps there is too much being tested in this one test, then?
One of the things on my list is to clean up the various test cases that
rely on signal handling when they are not testing something related to
signal handling.  It makes those tests unusable on non-POSIX
configurations (i.e. currently only NaCl) that do not support signals.
So adding another such case is kind of going backwards.  In this case,
going from gratuitous Linuxism to just thorough POSIXism is still an
incremental improvement (it should fix it for Hurd, e.g.).  So that is
fine for now if it really is unavoidably much harder to write a good
test without catching a signal.  But still not ideal.


Thanks,
Roland

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

On Tue, Jul 14, 2015 at 06:00:28PM -0700, Roland McGrath wrote:
> Hmm.  Perhaps there is too much being tested in this one test, then?
> One of the things on my list is to clean up the various test cases that
> rely on signal handling when they are not testing something related to
> signal handling.  It makes those tests unusable on non-POSIX
> configurations (i.e. currently only NaCl) that do not support signals.
> So adding another such case is kind of going backwards.  In this case,
> going from gratuitous Linuxism to just thorough POSIXism is still an
> incremental improvement (it should fix it for Hurd, e.g.).  So that is
> fine for now if it really is unavoidably much harder to write a good
> test without catching a signal.  But still not ideal.

I'm afraid it is all one test - make sure that the DSO does not unload
with the first dlclose and then make sure that it does with the
second.

Siddhesh

[PATCH v3] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

v3: I renamed some functions to make it clearer what they were doing. 
I also added foo_var to test reachability of the DSO address space
without side-effects.

The tst-tls-atexit test case searches for its module in /proc/PID/maps
to verify that it is unloaded, which is a Linux-specific test.  This
patch makes the test generic by trying to call foo (the symbol
obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
occurred.

Verified that the test continues to succeed on x86_64.  There is a bug
in the test case where it calls dlclose once again, which is actually
incorrect but still manages to unload the DSO thanks to an existing
bug in __tls_call_dtors.  This will be fixed in a later patch which
also fixes up the __cxa_thread_atexit_impl implementation.  I have
added a FIXME comment to that call momentarily, which I will remove
when I fix the problem.

	* stdlib/tst-tls-atexit-lib.c (foo_var): New variable.
	(do_foo): Rename to reg_dtor.
	* stdlib/tst-tls-atexit.c: (segv_handler): New function.
	(load): Rename to reg_dtor_and_close.  Move dlopen to...
	(do_test): ... here.  Use FOO_VAR from the DSO to test its
	availability.  Expect SIGSEGV after the DSO is closed.
---
 stdlib/tst-tls-atexit-lib.c |  3 +-
 stdlib/tst-tls-atexit.c     | 96 ++++++++++++++++++++++++++-------------------
 2 files changed, 58 insertions(+), 41 deletions(-)

diff --git a/stdlib/tst-tls-atexit-lib.c b/stdlib/tst-tls-atexit-lib.c
index 2945379..7e6e935 100644
--- a/stdlib/tst-tls-atexit-lib.c
+++ b/stdlib/tst-tls-atexit-lib.c
@@ -19,6 +19,7 @@
 #include <stdlib.h>
 
 extern void *__dso_handle;
+int foo_var = 42;
 
 typedef struct
 {
@@ -31,7 +32,7 @@ void A_dtor (void *obj)
   ((A *)obj)->val = obj;
 }
 
-void do_foo (void)
+void reg_dtor (void)
 {
   static __thread A b;
   __cxa_thread_atexit_impl (A_dtor, &b, __dso_handle);
diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
index 0c6c499..0a9e920 100644
--- a/stdlib/tst-tls-atexit.c
+++ b/stdlib/tst-tls-atexit.c
@@ -16,10 +16,11 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-/* There are two tests in this test case.  The first is implicit where it is
+/* There are two parts in this test case.  The first is implicit where it is
    assumed that the destructor call on exit of the LOAD function does not
    segfault.  The other is a verification that after the thread has exited, a
-   dlclose will unload the DSO.  */
+   dlclose will unload the DSO, which is done by calling into the DSO and
+   expecting that call to segfault.  */
 
 #include <dlfcn.h>
 #include <pthread.h>
@@ -28,31 +29,30 @@
 #include <string.h>
 #include <errno.h>
 
-void *handle;
-
-void *
-load (void *u)
+static void
+segv_handler (int sig)
 {
-  handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
-  if (handle == NULL)
-    {
-      printf ("Unable to load DSO: %s\n", dlerror ());
-      return (void *) (uintptr_t) 1;
-    }
+  /* All good.  */
+  _exit (0);
+}
 
-  void (*foo) (void) = (void (*) (void)) dlsym (handle, "do_foo");
+/* Accept a valid handle returned by DLOPEN, load the reg_dtor symbol to
+   register a destructor and then call dlclose on the handle.  The dlclose
+   should not unload the DSO since the destructor has not been called yet.  */
+static void *
+reg_dtor_and_close (void *h)
+{
+  void (*reg_dtor) (void) = (void (*) (void)) dlsym (h, "reg_dtor");
 
-  if (foo == NULL)
+  if (reg_dtor == NULL)
     {
       printf ("Unable to find symbol: %s\n", dlerror ());
-      exit (1);
+      return (void *) (uintptr_t) 1;
     }
 
-  foo ();
+  reg_dtor ();
 
-  /* This should not unload the DSO.  If it does, then the thread exit will
-     result in a segfault.  */
-  dlclose (handle);
+  dlclose (h);
 
   return NULL;
 }
@@ -64,7 +64,27 @@ do_test (void)
   int ret;
   void *thr_ret;
 
-  if ((ret = pthread_create (&t, NULL, load, NULL)) != 0)
+  /* Load the DSO.  */
+  void *handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
+  if (handle == NULL)
+    {
+      printf ("Unable to load DSO: %s\n", dlerror ());
+      return 1;
+    }
+
+  /* FOO is our variable to test if the DSO is unloaded.  */
+  int *foo = dlsym (handle, "foo_var");
+
+  if (foo == NULL)
+    {
+      printf ("Unable to find symbol do_foo: %s\n", dlerror ());
+      return 1;
+    }
+
+  /* Print FOO to be sure that it is working OK.  */
+  printf ("%d\n", *foo);
+
+  if ((ret = pthread_create (&t, NULL, reg_dtor_and_close, handle)) != 0)
     {
       printf ("pthread_create failed: %s\n", strerror (ret));
       return 1;
@@ -79,32 +99,28 @@ do_test (void)
   if (thr_ret != NULL)
     return 1;
 
-  /* Now this should unload the DSO.  */
+  /* Now this should unload the DSO.  FIXME: This is a bug, calling dlclose
+     like this is actually wrong, but it works because cxa_thread_atexit_impl
+     has a bug which results in dlclose allowing this to work.  */
   dlclose (handle);
 
-  /* Run through our maps and ensure that the DSO is unloaded.  */
-  FILE *f = fopen ("/proc/self/maps", "r");
-
-  if (f == NULL)
+  /* Handle SIGSEGV.  We don't use the EXPECTED_SIGNAL macro because we want to
+     detect any other SIGSEGVs as failures.  */
+  struct sigaction sa, old_sa;
+  sa.sa_handler = segv_handler;
+  sigemptyset (&sa.sa_mask);
+  sa.sa_flags = 0;
+  if (sigaction (SIGSEGV, &sa, &old_sa) < 0)
     {
-      perror ("Failed to open /proc/self/maps");
-      fprintf (stderr, "Skipping verification of DSO unload\n");
-      return 0;
+      printf ("Failed to set SIGSEGV handler: %m\n");
+      return 1;
     }
 
-  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 1;
-	}
-    }
-  free (line);
+  /* This should crash...  */
+  printf ("%d\n", *foo);
 
-  return 0;
+  /* ... or else we fail.  */
+  return 1;
 }
 
 #define TEST_FUNCTION do_test ()
-- 
2.4.3

Re: [PATCH] Remove Linuxism from tst-tls-atexit

[Szabolcs Nagy]

On 15/07/15 02:00, Roland McGrath wrote:
> One of the things on my list is to clean up the various test cases that
> rely on signal handling when they are not testing something related to
> signal handling.  It makes those tests unusable on non-POSIX
> configurations (i.e. currently only NaCl) that do not support signals.
> So adding another such case is kind of going backwards.  In this case,
> going from gratuitous Linuxism to just thorough POSIXism is still an
> incremental improvement (it should fix it for Hurd, e.g.).  So that is
> fine for now if it really is unavoidably much harder to write a good
> test without catching a signal.  But still not ideal.

hm i thought assuming posix is the right way to go.

if the tests cannot even assume a conforming posix
environment then what can they?

[ping][PATCH v3] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

Ping!

On Wed, Jul 15, 2015 at 11:53:13AM +0530, Siddhesh Poyarekar wrote:
> v3: I renamed some functions to make it clearer what they were doing. 
> I also added foo_var to test reachability of the DSO address space
> without side-effects.
> 
> The tst-tls-atexit test case searches for its module in /proc/PID/maps
> to verify that it is unloaded, which is a Linux-specific test.  This
> patch makes the test generic by trying to call foo (the symbol
> obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
> occurred.
> 
> Verified that the test continues to succeed on x86_64.  There is a bug
> in the test case where it calls dlclose once again, which is actually
> incorrect but still manages to unload the DSO thanks to an existing
> bug in __tls_call_dtors.  This will be fixed in a later patch which
> also fixes up the __cxa_thread_atexit_impl implementation.  I have
> added a FIXME comment to that call momentarily, which I will remove
> when I fix the problem.
> 
> 	* stdlib/tst-tls-atexit-lib.c (foo_var): New variable.
> 	(do_foo): Rename to reg_dtor.
> 	* stdlib/tst-tls-atexit.c: (segv_handler): New function.
> 	(load): Rename to reg_dtor_and_close.  Move dlopen to...
> 	(do_test): ... here.  Use FOO_VAR from the DSO to test its
> 	availability.  Expect SIGSEGV after the DSO is closed.
> ---
>  stdlib/tst-tls-atexit-lib.c |  3 +-
>  stdlib/tst-tls-atexit.c     | 96 ++++++++++++++++++++++++++-------------------
>  2 files changed, 58 insertions(+), 41 deletions(-)
> 
> diff --git a/stdlib/tst-tls-atexit-lib.c b/stdlib/tst-tls-atexit-lib.c
> index 2945379..7e6e935 100644
> --- a/stdlib/tst-tls-atexit-lib.c
> +++ b/stdlib/tst-tls-atexit-lib.c
> @@ -19,6 +19,7 @@
>  #include <stdlib.h>
>  
>  extern void *__dso_handle;
> +int foo_var = 42;
>  
>  typedef struct
>  {
> @@ -31,7 +32,7 @@ void A_dtor (void *obj)
>    ((A *)obj)->val = obj;
>  }
>  
> -void do_foo (void)
> +void reg_dtor (void)
>  {
>    static __thread A b;
>    __cxa_thread_atexit_impl (A_dtor, &b, __dso_handle);
> diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
> index 0c6c499..0a9e920 100644
> --- a/stdlib/tst-tls-atexit.c
> +++ b/stdlib/tst-tls-atexit.c
> @@ -16,10 +16,11 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -/* There are two tests in this test case.  The first is implicit where it is
> +/* There are two parts in this test case.  The first is implicit where it is
>     assumed that the destructor call on exit of the LOAD function does not
>     segfault.  The other is a verification that after the thread has exited, a
> -   dlclose will unload the DSO.  */
> +   dlclose will unload the DSO, which is done by calling into the DSO and
> +   expecting that call to segfault.  */
>  
>  #include <dlfcn.h>
>  #include <pthread.h>
> @@ -28,31 +29,30 @@
>  #include <string.h>
>  #include <errno.h>
>  
> -void *handle;
> -
> -void *
> -load (void *u)
> +static void
> +segv_handler (int sig)
>  {
> -  handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
> -  if (handle == NULL)
> -    {
> -      printf ("Unable to load DSO: %s\n", dlerror ());
> -      return (void *) (uintptr_t) 1;
> -    }
> +  /* All good.  */
> +  _exit (0);
> +}
>  
> -  void (*foo) (void) = (void (*) (void)) dlsym (handle, "do_foo");
> +/* Accept a valid handle returned by DLOPEN, load the reg_dtor symbol to
> +   register a destructor and then call dlclose on the handle.  The dlclose
> +   should not unload the DSO since the destructor has not been called yet.  */
> +static void *
> +reg_dtor_and_close (void *h)
> +{
> +  void (*reg_dtor) (void) = (void (*) (void)) dlsym (h, "reg_dtor");
>  
> -  if (foo == NULL)
> +  if (reg_dtor == NULL)
>      {
>        printf ("Unable to find symbol: %s\n", dlerror ());
> -      exit (1);
> +      return (void *) (uintptr_t) 1;
>      }
>  
> -  foo ();
> +  reg_dtor ();
>  
> -  /* This should not unload the DSO.  If it does, then the thread exit will
> -     result in a segfault.  */
> -  dlclose (handle);
> +  dlclose (h);
>  
>    return NULL;
>  }
> @@ -64,7 +64,27 @@ do_test (void)
>    int ret;
>    void *thr_ret;
>  
> -  if ((ret = pthread_create (&t, NULL, load, NULL)) != 0)
> +  /* Load the DSO.  */
> +  void *handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
> +  if (handle == NULL)
> +    {
> +      printf ("Unable to load DSO: %s\n", dlerror ());
> +      return 1;
> +    }
> +
> +  /* FOO is our variable to test if the DSO is unloaded.  */
> +  int *foo = dlsym (handle, "foo_var");
> +
> +  if (foo == NULL)
> +    {
> +      printf ("Unable to find symbol do_foo: %s\n", dlerror ());
> +      return 1;
> +    }
> +
> +  /* Print FOO to be sure that it is working OK.  */
> +  printf ("%d\n", *foo);
> +
> +  if ((ret = pthread_create (&t, NULL, reg_dtor_and_close, handle)) != 0)
>      {
>        printf ("pthread_create failed: %s\n", strerror (ret));
>        return 1;
> @@ -79,32 +99,28 @@ do_test (void)
>    if (thr_ret != NULL)
>      return 1;
>  
> -  /* Now this should unload the DSO.  */
> +  /* Now this should unload the DSO.  FIXME: This is a bug, calling dlclose
> +     like this is actually wrong, but it works because cxa_thread_atexit_impl
> +     has a bug which results in dlclose allowing this to work.  */
>    dlclose (handle);
>  
> -  /* Run through our maps and ensure that the DSO is unloaded.  */
> -  FILE *f = fopen ("/proc/self/maps", "r");
> -
> -  if (f == NULL)
> +  /* Handle SIGSEGV.  We don't use the EXPECTED_SIGNAL macro because we want to
> +     detect any other SIGSEGVs as failures.  */
> +  struct sigaction sa, old_sa;
> +  sa.sa_handler = segv_handler;
> +  sigemptyset (&sa.sa_mask);
> +  sa.sa_flags = 0;
> +  if (sigaction (SIGSEGV, &sa, &old_sa) < 0)
>      {
> -      perror ("Failed to open /proc/self/maps");
> -      fprintf (stderr, "Skipping verification of DSO unload\n");
> -      return 0;
> +      printf ("Failed to set SIGSEGV handler: %m\n");
> +      return 1;
>      }
>  
> -  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 1;
> -	}
> -    }
> -  free (line);
> +  /* This should crash...  */
> +  printf ("%d\n", *foo);
>  
> -  return 0;
> +  /* ... or else we fail.  */
> +  return 1;
>  }
>  
>  #define TEST_FUNCTION do_test ()
> -- 
> 2.4.3
> 

Re: [PATCH v3] Remove Linuxism from tst-tls-atexit

[Carlos O'Donell]

On 07/15/2015 02:23 AM, Siddhesh Poyarekar wrote:
> v3: I renamed some functions to make it clearer what they were doing. 
> I also added foo_var to test reachability of the DSO address space
> without side-effects.

> +static void
> +segv_handler (int sig)
>  {
> -  handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
> -  if (handle == NULL)
> -    {
> -      printf ("Unable to load DSO: %s\n", dlerror ());
> -      return (void *) (uintptr_t) 1;
> -    }
> +  /* All good.  */
> +  _exit (0);
> +}

We could crash for any number of reasons in printf.

May I suggest walking _r_debug->r_map and checking l_name
to see if the DSO is still loaded? Just like the debugger would do?

This doesn't require a SIGSEGV handler at all?

Cheers,
Carlos.

[PATCH v6] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

The tst-tls-atexit test case searches for its module in /proc/PID/maps
to verify that it is unloaded, which is a Linux-specific test.  This
patch makes the test generic by trying to call foo (the symbol
obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
occurred.

Verified that the test continues to succeed on x86_64.  There is a bug
in the test case where it calls dlclose once again, which is actually
incorrect but still manages to unload the DSO thanks to an existing
bug in __tls_call_dtors.  This will be fixed in a later patch which
also fixes up the __cxa_thread_atexit_impl implementation.  I have
added a FIXME comment to that call momentarily, which I will remove
when I fix the problem.

	* stdlib/tst-tls-atexit-lib.c (do_foo): Rename to reg_dtor.
	* stdlib/tst-tls-atexit.c: (is_loaded): New function.
	(load): Rename to reg_dtor_and_close.  Move dlopen to...
	(do_test): ... here.  Use IS_LOADED to test for its
	availability.
---
 stdlib/tst-tls-atexit-lib.c |  2 +-
 stdlib/tst-tls-atexit.c     | 84 +++++++++++++++++++++++----------------------
 2 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/stdlib/tst-tls-atexit-lib.c b/stdlib/tst-tls-atexit-lib.c
index 2945379..2478d80 100644
--- a/stdlib/tst-tls-atexit-lib.c
+++ b/stdlib/tst-tls-atexit-lib.c
@@ -31,7 +31,7 @@ void A_dtor (void *obj)
   ((A *)obj)->val = obj;
 }
 
-void do_foo (void)
+void reg_dtor (void)
 {
   static __thread A b;
   __cxa_thread_atexit_impl (A_dtor, &b, __dso_handle);
diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
index 0c6c499..1947048 100644
--- a/stdlib/tst-tls-atexit.c
+++ b/stdlib/tst-tls-atexit.c
@@ -16,10 +16,11 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-/* There are two tests in this test case.  The first is implicit where it is
+/* There are two parts in this test case.  The first is implicit where it is
    assumed that the destructor call on exit of the LOAD function does not
    segfault.  The other is a verification that after the thread has exited, a
-   dlclose will unload the DSO.  */
+   dlclose will unload the DSO, which is done by calling into the DSO and
+   expecting that call to segfault.  */
 
 #include <dlfcn.h>
 #include <pthread.h>
@@ -27,32 +28,41 @@
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
+#include <link.h>
 
-void *handle;
+#define DSO_NAME "$ORIGIN/tst-tls-atexit-lib.so"
 
-void *
-load (void *u)
+/* Walk through the map in the _r_debug structure to see if our lib is still
+   loaded.  */
+static bool
+is_loaded (void)
 {
-  handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
-  if (handle == NULL)
-    {
-      printf ("Unable to load DSO: %s\n", dlerror ());
-      return (void *) (uintptr_t) 1;
-    }
+  struct link_map *lm = (struct link_map *) _r_debug.r_map;
+
+  for (; lm; lm = lm->l_next)
+    if (lm->l_type == lt_loaded && lm->l_name
+	&& strcmp (basename (DSO_NAME), basename (lm->l_name)) == 0)
+	  return true;
+  return false;
+}
 
-  void (*foo) (void) = (void (*) (void)) dlsym (handle, "do_foo");
+/* Accept a valid handle returned by DLOPEN, load the reg_dtor symbol to
+   register a destructor and then call dlclose on the handle.  The dlclose
+   should not unload the DSO since the destructor has not been called yet.  */
+static void *
+reg_dtor_and_close (void *h)
+{
+  void (*reg_dtor) (void) = (void (*) (void)) dlsym (h, "reg_dtor");
 
-  if (foo == NULL)
+  if (reg_dtor == NULL)
     {
       printf ("Unable to find symbol: %s\n", dlerror ());
-      exit (1);
+      return (void *) (uintptr_t) 1;
     }
 
-  foo ();
+  reg_dtor ();
 
-  /* This should not unload the DSO.  If it does, then the thread exit will
-     result in a segfault.  */
-  dlclose (handle);
+  dlclose (h);
 
   return NULL;
 }
@@ -64,7 +74,15 @@ do_test (void)
   int ret;
   void *thr_ret;
 
-  if ((ret = pthread_create (&t, NULL, load, NULL)) != 0)
+  /* Load the DSO.  */
+  void *handle = dlopen (DSO_NAME, RTLD_LAZY);
+  if (handle == NULL)
+    {
+      printf ("Unable to load DSO: %s\n", dlerror ());
+      return 1;
+    }
+
+  if ((ret = pthread_create (&t, NULL, reg_dtor_and_close, handle)) != 0)
     {
       printf ("pthread_create failed: %s\n", strerror (ret));
       return 1;
@@ -79,30 +97,14 @@ do_test (void)
   if (thr_ret != NULL)
     return 1;
 
-  /* Now this should unload the DSO.  */
+  /* Now this should unload the DSO.  FIXME: This is a bug, calling dlclose
+     like this is actually wrong, but it works because cxa_thread_atexit_impl
+     has a bug which results in dlclose allowing this to work.  */
   dlclose (handle);
 
-  /* 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 1;
-	}
-    }
-  free (line);
+  /* Check link maps to ensure that the DSO has unloaded.  */
+  if (is_loaded ())
+    return 1;
 
   return 0;
 }
-- 
2.4.3

Re: [PATCH v6] Remove Linuxism from tst-tls-atexit

[Andreas Schwab]

"Siddhesh Poyarekar" <siddhesh@redhat.com> writes:

> The tst-tls-atexit test case searches for its module in /proc/PID/maps
> to verify that it is unloaded, which is a Linux-specific test.  This
> patch makes the test generic by trying to call foo (the symbol
> obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
> occurred.

This is no longer true. :-)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH v6] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

On Mon, Jul 20, 2015 at 07:04:55PM +0200, Andreas Schwab wrote:
> "Siddhesh Poyarekar" <siddhesh@redhat.com> writes:
> 
> > The tst-tls-atexit test case searches for its module in /proc/PID/maps
> > to verify that it is unloaded, which is a Linux-specific test.  This
> > patch makes the test generic by trying to call foo (the symbol
> > obtained from dlsym) and traps SIGSEGV momentarily to see if the crash
> > occurred.
> 
> This is no longer true. :-)

Ugh, thanks, I'll fix it up when I commit.

Siddhesh

[PATCH v7] Remove Linuxism from tst-tls-atexit

[Siddhesh Poyarekar]

The tst-tls-atexit test case searches for its module in /proc/PID/maps
to verify that it is unloaded, which is a Linux-specific test.  This
patch makes the test generic by looking for the library in the link
map list in the _r_debug structure.

Verified that the test continues to succeed on x86_64.  There is a bug
in the test case where it calls dlclose once again, which is actually
incorrect but still manages to unload the DSO thanks to an existing
bug in __tls_call_dtors.  This will be fixed in a later patch which
also fixes up the __cxa_thread_atexit_impl implementation.  I have
added a FIXME comment to that call momentarily, which I will remove
when I fix the problem.

	* stdlib/tst-tls-atexit-lib.c (do_foo): Rename to reg_dtor.
	* stdlib/tst-tls-atexit.c: (is_loaded): New function.
	(spawn_thread): New function.
	(load): Rename to reg_dtor_and_close.  Move dlopen to...
	(do_test): ... here.  Use IS_LOADED to test for its
	availability.
---
 stdlib/tst-tls-atexit-lib.c |  2 +-
 stdlib/tst-tls-atexit.c     | 96 +++++++++++++++++++++++++--------------------
 2 files changed, 55 insertions(+), 43 deletions(-)

diff --git a/stdlib/tst-tls-atexit-lib.c b/stdlib/tst-tls-atexit-lib.c
index 2945379..2478d80 100644
--- a/stdlib/tst-tls-atexit-lib.c
+++ b/stdlib/tst-tls-atexit-lib.c
@@ -31,7 +31,7 @@ void A_dtor (void *obj)
   ((A *)obj)->val = obj;
 }
 
-void do_foo (void)
+void reg_dtor (void)
 {
   static __thread A b;
   __cxa_thread_atexit_impl (A_dtor, &b, __dso_handle);
diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c
index 0c6c499..cea655d 100644
--- a/stdlib/tst-tls-atexit.c
+++ b/stdlib/tst-tls-atexit.c
@@ -16,10 +16,12 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-/* There are two tests in this test case.  The first is implicit where it is
-   assumed that the destructor call on exit of the LOAD function does not
-   segfault.  The other is a verification that after the thread has exited, a
-   dlclose will unload the DSO.  */
+/* This test dynamically loads a DSO and spawns a thread that subsequently
+   calls into the DSO to register a destructor for an object in the DSO and
+   then calls dlclose on the handle for the DSO.  When the thread exits, the
+   DSO should not be unloaded or else the destructor called during thread exit
+   will crash.  Further in the main thread, the DSO is opened and closed again,
+   at which point the DSO should be unloaded.  */
 
 #include <dlfcn.h>
 #include <pthread.h>
@@ -27,44 +29,53 @@
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
+#include <link.h>
 
-void *handle;
+#define DSO_NAME "$ORIGIN/tst-tls-atexit-lib.so"
 
-void *
-load (void *u)
+/* Walk through the map in the _r_debug structure to see if our lib is still
+   loaded.  */
+static bool
+is_loaded (void)
 {
-  handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY);
-  if (handle == NULL)
-    {
-      printf ("Unable to load DSO: %s\n", dlerror ());
-      return (void *) (uintptr_t) 1;
-    }
+  struct link_map *lm = (struct link_map *) _r_debug.r_map;
 
-  void (*foo) (void) = (void (*) (void)) dlsym (handle, "do_foo");
+  for (; lm; lm = lm->l_next)
+    if (lm->l_type == lt_loaded && lm->l_name
+	&& strcmp (basename (DSO_NAME), basename (lm->l_name)) == 0)
+	  return true;
+  return false;
+}
+
+/* Accept a valid handle returned by DLOPEN, load the reg_dtor symbol to
+   register a destructor and then call dlclose on the handle.  The dlclose
+   should not unload the DSO since the destructor has not been called yet.  */
+static void *
+reg_dtor_and_close (void *h)
+{
+  void (*reg_dtor) (void) = (void (*) (void)) dlsym (h, "reg_dtor");
 
-  if (foo == NULL)
+  if (reg_dtor == NULL)
     {
       printf ("Unable to find symbol: %s\n", dlerror ());
-      exit (1);
+      return (void *) (uintptr_t) 1;
     }
 
-  foo ();
+  reg_dtor ();
 
-  /* This should not unload the DSO.  If it does, then the thread exit will
-     result in a segfault.  */
-  dlclose (handle);
+  dlclose (h);
 
   return NULL;
 }
 
 static int
-do_test (void)
+spawn_thread (void *h)
 {
   pthread_t t;
   int ret;
   void *thr_ret;
 
-  if ((ret = pthread_create (&t, NULL, load, NULL)) != 0)
+  if ((ret = pthread_create (&t, NULL, reg_dtor_and_close, h)) != 0)
     {
       printf ("pthread_create failed: %s\n", strerror (ret));
       return 1;
@@ -79,30 +90,31 @@ do_test (void)
   if (thr_ret != NULL)
     return 1;
 
-  /* Now this should unload the DSO.  */
-  dlclose (handle);
-
-  /* Run through our maps and ensure that the DSO is unloaded.  */
-  FILE *f = fopen ("/proc/self/maps", "r");
+  return 0;
+}
 
-  if (f == NULL)
+static int
+do_test (void)
+{
+  /* Load the DSO.  */
+  void *h1 = dlopen (DSO_NAME, RTLD_LAZY);
+  if (h1 == NULL)
     {
-      perror ("Failed to open /proc/self/maps");
-      fprintf (stderr, "Skipping verification of DSO unload\n");
-      return 0;
+      printf ("h1: Unable to load DSO: %s\n", dlerror ());
+      return 1;
     }
 
-  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 1;
-	}
-    }
-  free (line);
+  if (spawn_thread (h1) != 0)
+    return 1;
+
+  /* Now this should unload the DSO.  FIXME: This is a bug, calling dlclose
+     like this is actually wrong, but it works because cxa_thread_atexit_impl
+     has a bug which results in dlclose allowing this to work.  */
+  dlclose (h1);
+
+  /* Check link maps to ensure that the DSO has unloaded.  */
+  if (is_loaded ())
+    return 1;
 
   return 0;
 }
-- 
2.4.3

Re: [PATCH v7] Remove Linuxism from tst-tls-atexit

[Carlos O'Donell]

On 07/20/2015 01:22 PM, Siddhesh Poyarekar wrote:
> The tst-tls-atexit test case searches for its module in /proc/PID/maps
> to verify that it is unloaded, which is a Linux-specific test.  This
> patch makes the test generic by looking for the library in the link
> map list in the _r_debug structure.
> 
> Verified that the test continues to succeed on x86_64.  There is a bug
> in the test case where it calls dlclose once again, which is actually
> incorrect but still manages to unload the DSO thanks to an existing
> bug in __tls_call_dtors.  This will be fixed in a later patch which
> also fixes up the __cxa_thread_atexit_impl implementation.  I have
> added a FIXME comment to that call momentarily, which I will remove
> when I fix the problem.
> 
> 	* stdlib/tst-tls-atexit-lib.c (do_foo): Rename to reg_dtor.
> 	* stdlib/tst-tls-atexit.c: (is_loaded): New function.
> 	(spawn_thread): New function.
> 	(load): Rename to reg_dtor_and_close.  Move dlopen to...
> 	(do_test): ... here.  Use IS_LOADED to test for its
> 	availability.

Looks good to me for 2.22.

I know this is coupled with the fix we want for the l_tls_dtors_count.

Cheers,
Carlos.