| Message ID | 1436883383-6903-1-git-send-email-siddhesh@redhat.com |
|---|---|
| State | New |
| Headers | show |
"Siddhesh Poyarekar" <siddhesh@redhat.com> writes: > +static void > +segv_handler (int sig) > +{ > + /* All good. */ > + longjmp (env, 1); longjmp isn't async-signal-safe. Andreas.
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
> 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?
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
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
> 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
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
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?
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 ()