| Message ID | CALoOobN2H0dRjGUT_XSwYSO0pE_kWyVDoGrg+dSN_J4J5zpyJQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL) | expand |
On 08/27/2018 02:12 PM, Paul Pluzhnikov wrote: > Greetings, > > As discussed in bz#20544 and in > http://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers, > we should assert that the function passed to atexit, on_exit, etc. is > not NULL. This needs a test case that forks a child, runs the test, and catches a SIGABRT showing the failure resulting from the invalid API call. The test should cover all of the APIs we can tests by passing NULL. It is OK to call __cxa_atexit directly even though that might be invalid in a real program. > 2018-08-27 Paul Pluzhnikov <ppluzhnikov@google.com> > > [BZ #20544] > * stdlib/cxa_atexit.c (__internal_atexit): assert func != NULL. > * stdlib/on_exit.c (__on_exit): Likewise. > > > -- Paul Pluzhnikov > > > glibc-bz20544-20180825.txt > > > diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c > index 6d65f7e615..978f873990 100644 > --- a/stdlib/cxa_atexit.c > +++ b/stdlib/cxa_atexit.c > @@ -36,6 +36,9 @@ __internal_atexit (void (*func) (void *), void *arg, void *d, > { > struct exit_function *new; > > + /* BZ#20544 */ This should comment on why the assert is here. /* As a QoI issue we detect NULL early with an assertion instead of a SIGSEGV at program exit when the handler is run. */ > + assert (func != NULL); > + > __libc_lock_lock (__exit_funcs_lock); > new = __new_exitfn (listp); > > diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c > index 5241e0d86f..0ead1eafd4 100644 > --- a/stdlib/on_exit.c > +++ b/stdlib/on_exit.c > @@ -15,6 +15,7 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > +#include <assert.h> OK. > #include <stdlib.h> > #include "exit.h" > #include <sysdep.h> > @@ -25,6 +26,9 @@ __on_exit (void (*func) (int status, void *arg), void *arg) > { > struct exit_function *new; > > + /* BZ#20544 */ Likewise. > + assert (func != NULL); > + > __libc_lock_lock (__exit_funcs_lock); > new = __new_exitfn (&__exit_funcs); >
On Mon, Aug 27, 2018 at 1:02 PM Carlos O'Donell <carlos@redhat.com> wrote: > This needs a test case that forks a child, runs the test, and catches a > SIGABRT showing the failure resulting from the invalid API call. Revised patch attached. Thanks! -- Paul Pluzhnikov From 7eff89d5c35ea50ba4fd5f40bd351a0c35fe9803 Mon Sep 17 00:00:00 2001 From: Paul Pluzhnikov <ppluzhnikov@google.com> Date: Sat, 1 Sep 2018 10:50:41 -0700 Subject: [PATCH] BZ #20544 assert on NULL fn in atexit, etc. --- ChangeLog | 8 +++++ stdlib/Makefile | 2 +- stdlib/cxa_atexit.c | 4 +++ stdlib/on_exit.c | 5 +++ stdlib/tst-bz20544.c | 72 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 stdlib/tst-bz20544.c diff --git a/ChangeLog b/ChangeLog index d11440bc0f..4984b134cc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2018-09-01 Paul Pluzhnikov <ppluzhnikov@google.com> + + [BZ #20544] + * stdlib/cxa_atexit.c (__internal_atexit): assert func != NULL. + * stdlib/on_exit.c (__on_exit): Likewise. + * stdlib/Makefile (tests): Add tst-bz20544. + * stdlib/tst-bz20544.c: New test. + 2018-08-31 Paul Pluzhnikov <ppluzhnikov@google.com> [BZ #20271] diff --git a/stdlib/Makefile b/stdlib/Makefile index 01194bbf7c..c7f889b190 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -87,7 +87,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ tst-makecontext-align test-bz22786 tst-strtod-nan-sign \ tst-swapcontext1 tst-setcontext4 tst-setcontext5 \ tst-setcontext6 tst-setcontext7 tst-setcontext8 \ - tst-setcontext9 + tst-setcontext9 tst-bz20544 tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \ tst-tls-atexit tst-tls-atexit-nodelete diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c index 6d65f7e615..c1f146bfee 100644 --- a/stdlib/cxa_atexit.c +++ b/stdlib/cxa_atexit.c @@ -36,6 +36,10 @@ __internal_atexit (void (*func) (void *), void *arg, void *d, { struct exit_function *new; + /* As a QoI issue we detect NULL early with an assertion instead + of a SIGSEGV at program exit when the handler is run. BZ#20544 */ + assert (func != NULL); + __libc_lock_lock (__exit_funcs_lock); new = __new_exitfn (listp); diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c index 5241e0d86f..a8be62c513 100644 --- a/stdlib/on_exit.c +++ b/stdlib/on_exit.c @@ -15,6 +15,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <assert.h> #include <stdlib.h> #include "exit.h" #include <sysdep.h> @@ -25,6 +26,10 @@ __on_exit (void (*func) (int status, void *arg), void *arg) { struct exit_function *new; + /* As a QoI issue we detect NULL early with an assertion instead + of a SIGSEGV at program exit when the handler is run. BZ#20544 */ + assert (func != NULL); + __libc_lock_lock (__exit_funcs_lock); new = __new_exitfn (&__exit_funcs); diff --git a/stdlib/tst-bz20544.c b/stdlib/tst-bz20544.c new file mode 100644 index 0000000000..a472ebaa10 --- /dev/null +++ b/stdlib/tst-bz20544.c @@ -0,0 +1,72 @@ +/* Verify atexit, on_exit, etc. abort on NULL function pointer. + Copyright (C) 2018 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 <signal.h> +#include <stdlib.h> +#include <support/check.h> +#include <support/support.h> +#include <support/test-driver.h> +#include <support/xunistd.h> + +extern int __cxa_atexit (void (*func) (void *), void *arg, void *d); + +#pragma GCC diagnostic ignored "-Wnonnull" + +static int +do_test (void) +{ +#if defined(NDEBUG) + /* Assert disabled, can't verify that assertions fire. */ +#else + int j; + + for (j = 0; j < 3; j++) { + pid_t pid = xfork (); + if (pid == 0) { + /* Child. */ + switch (j) { + case 0: + atexit (NULL); /* Should assert. */ + exit (EXIT_FAILURE); + case 1: + on_exit (NULL, NULL); /* Should assert. */ + exit (EXIT_FAILURE); + case 2: + __cxa_atexit (NULL, NULL, NULL); /* Should assert. */ + exit (EXIT_FAILURE); + default: + /* We shouldn't be here. */ + exit (EXIT_FAILURE); + } + } else { + /* Parent. Verify child exited with SIGABRT. */ + int status; + + TEST_VERIFY (pid == xwaitpid (pid, &status, 0)); + TEST_VERIFY (WIFSIGNALED (status)); + TEST_VERIFY (WTERMSIG (status) == SIGABRT); + } + } +#endif /* NDEBUG */ + + return 0; +} + +#define TEST_FUNCTION do_test +#include <support/test-driver.c>
On Sat, Sep 1, 2018 at 10:51 AM Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > > On Mon, Aug 27, 2018 at 1:02 PM Carlos O'Donell <carlos@redhat.com> wrote: > > > This needs a test case that forks a child, runs the test, and catches a > > SIGABRT showing the failure resulting from the invalid API call. > > Revised patch attached. Thanks! Ping?
On Sat, 01 Sep 2018, Paul Pluzhnikov wrote: >+static int >+do_test (void) >+{ >+#if defined(NDEBUG) >+ /* Assert disabled, can't verify that assertions fire. */ >+#endif Wouldn't it be better to use FAIL_UNSUPPORTED, so that the test is reported as unsupported in the test summary? Something in the lines of: FAIL_UNSUPPORTED ("Assert disabled, can't verify that assertions fire."); >+#define TEST_FUNCTION do_test You don't need this line with the new test framework. >+#include <support/test-driver.c>
On Tue, Nov 13, 2018 at 5:59 AM Gabriel F. T. Gomes
<gabriel@inconstante.eti.br> wrote:
> FAIL_UNSUPPORTED ("Assert disabled, can't verify that assertions fire.");
Thanks. Revised patch attached.
On Tue, 13 Nov 2018, Paul Pluzhnikov wrote:
>Thanks. Revised patch attached.
Looks good to me.
* Paul Pluzhnikov: > + atexit (NULL); /* Should assert. */ > + exit (EXIT_FAILURE); I think this will print assertion failure messages to the build log, which is confusing to anyone reviewing it for unexpected errors. Can we avoid this? Thanks, Florian
On Wed, Nov 14, 2018 at 10:01 AM Florian Weimer <fweimer@redhat.com> wrote: > I think this will print assertion failure messages to the build log, Indeed it does: tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func != NULL' failed. tst-bz20544: on_exit.c:31: __on_exit: Assertion `func != NULL' failed. tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func != NULL' failed. > which is confusing to anyone reviewing it for unexpected errors. Can we > avoid this? Revised patch attached. Thanks!
On Thu, Nov 15, 2018 at 1:38 PM Paul Pluzhnikov <ppluzhnikov@google.com> wrote: > > On Wed, Nov 14, 2018 at 10:01 AM Florian Weimer <fweimer@redhat.com> wrote: > > > I think this will print assertion failure messages to the build log, > Revised patch attached. Thanks! Ping?
On 15/11/2018 19:38, Paul Pluzhnikov wrote: > On Wed, Nov 14, 2018 at 10:01 AM Florian Weimer <fweimer@redhat.com> wrote: > >> I think this will print assertion failure messages to the build log, > Indeed it does: > > tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func != > NULL' failed. > tst-bz20544: on_exit.c:31: __on_exit: Assertion `func != NULL' failed. > tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func != > NULL' failed. > >> which is confusing to anyone reviewing it for unexpected errors. Can we >> avoid this? > Revised patch attached. Thanks! > -- Paul Pluzhnikov > > > glibc-bz20544-20181115.txt > > From 83269977989f071105419bc15dbf0b992db8db5e Mon Sep 17 00:00:00 2001 > From: Paul Pluzhnikov <ppluzhnikov@google.com> > Date: Sat, 1 Sep 2018 10:50:41 -0700 > Subject: [PATCH] BZ #20544 assert on NULL fn in atexit, etc. > > --- > ChangeLog | 8 +++++ > stdlib/Makefile | 2 +- > stdlib/cxa_atexit.c | 4 +++ > stdlib/on_exit.c | 5 +++ > stdlib/tst-bz20544.c | 75 ++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 93 insertions(+), 1 deletion(-) > create mode 100644 stdlib/tst-bz20544.c > > diff --git a/ChangeLog b/ChangeLog > index 149f991b70..be1a4606c1 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,11 @@ > +2018-11-15 Paul Pluzhnikov <ppluzhnikov@google.com> > + > + [BZ #20544] > + * stdlib/cxa_atexit.c (__internal_atexit): assert func != NULL. > + * stdlib/on_exit.c (__on_exit): Likewise. > + * stdlib/Makefile (tests): Add tst-bz20544. > + * stdlib/tst-bz20544.c: New test. > + > 2018-11-14 Samuel Thibault <samuel.thibault@ens-lyon.org> > > * sysdeps/mach/hurd/dl-sysdep.c (check_no_hidden): Use > diff --git a/stdlib/Makefile b/stdlib/Makefile > index 98dbddc43c..8bce89fffe 100644 > --- a/stdlib/Makefile > +++ b/stdlib/Makefile > @@ -87,7 +87,7 @@ tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ > tst-makecontext-align test-bz22786 tst-strtod-nan-sign \ > tst-swapcontext1 tst-setcontext4 tst-setcontext5 \ > tst-setcontext6 tst-setcontext7 tst-setcontext8 \ > - tst-setcontext9 > + tst-setcontext9 tst-bz20544 > > tests-internal := tst-strtod1i tst-strtod3 tst-strtod4 tst-strtod5i \ > tst-tls-atexit tst-tls-atexit-nodelete > diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c > index 6d65f7e615..c1f146bfee 100644 > --- a/stdlib/cxa_atexit.c > +++ b/stdlib/cxa_atexit.c > @@ -36,6 +36,10 @@ __internal_atexit (void (*func) (void *), void *arg, void *d, > { > struct exit_function *new; > > + /* As a QoI issue we detect NULL early with an assertion instead > + of a SIGSEGV at program exit when the handler is run. BZ#20544 */ > + assert (func != NULL); > + > __libc_lock_lock (__exit_funcs_lock); > new = __new_exitfn (listp); > > diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c > index 5241e0d86f..a8be62c513 100644 > --- a/stdlib/on_exit.c > +++ b/stdlib/on_exit.c > @@ -15,6 +15,7 @@ > License along with the GNU C Library; if not, see > <http://www.gnu.org/licenses/>. */ > > +#include <assert.h> > #include <stdlib.h> > #include "exit.h" > #include <sysdep.h> > @@ -25,6 +26,10 @@ __on_exit (void (*func) (int status, void *arg), void *arg) > { > struct exit_function *new; > > + /* As a QoI issue we detect NULL early with an assertion instead > + of a SIGSEGV at program exit when the handler is run. BZ#20544 */ > + assert (func != NULL); > + > __libc_lock_lock (__exit_funcs_lock); > new = __new_exitfn (&__exit_funcs); > > diff --git a/stdlib/tst-bz20544.c b/stdlib/tst-bz20544.c > new file mode 100644 > index 0000000000..79b0f9b561 > --- /dev/null > +++ b/stdlib/tst-bz20544.c > @@ -0,0 +1,75 @@ > +/* Verify atexit, on_exit, etc. abort on NULL function pointer. > + Copyright (C) 2018 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 <signal.h> > +#include <stdlib.h> > +#include <support/check.h> > +#include <support/support.h> > +#include <support/test-driver.h> > +#include <support/xunistd.h> > + > +extern int __cxa_atexit (void (*func) (void *), void *arg, void *d); > + > +#pragma GCC diagnostic ignored "-Wnonnull" > + > +static int > +do_test (void) > +{ > +#if defined(NDEBUG) > + FAIL_UNSUPPORTED("Assertions disabled (NDEBUG). " > + "Can't verify that assertions fire."); > +#else > + int j; > + > + for (j = 0; j < 3; j++) { Wrong indentation, open bracket should be aligned on next line. > + pid_t pid = xfork (); > + if (pid == 0) { > + /* Child. Close stderr so we don't pollute build log with (expected) > + assertion failures. */ > + close (STDERR_FILENO); I think instead of using xfork, this test should use support_capture_subprocess instead. It already takes care of handling the file descriptor, reaping the child and checking for expected termination value. In this case: --- #ifndef NDEBUG static void do_test_bz20544_atexit (void *closure) { atexit (NULL); /* should assert. */ exit (EXIT_FAILURE); } static void do_test_bz20544_on_exit (void *closure) { on_exit (NULL, NULL); /* should assert. */ exit (EXIT_FAILURE); } static void do_test_bz20544_cxa_atexit (void *closure) { __cxa_atexit (NULL, NULL, NULL); /* Should assert. */ exit (EXIT_FAILURE); } #endif static int do_test (void) { #ifdef NDEBUG FAIL_UNSUPPORTED("Assertions disabled (NDEBUG defined). " "Can't verify that assertions fire."); #else { struct support_capture_subprocess result; result = support_capture_subprocess (do_test_bz20544_atexit, NULL); support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT, sc_allow_stderr); TEST_COMPARE_STRING (result.err.buffer, "tst-bz20544: cxa_atexit.c:41: __internal_atexit: " \ "Assertion `func != NULL' failed.\n"); } { struct support_capture_subprocess result; result = support_capture_subprocess (do_test_bz20544_on_exit, NULL); support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT, sc_allow_stderr); TEST_COMPARE_STRING (result.err.buffer, "tst-bz20544: on_exit.c:31: __on_exit: " \ "Assertion `func != NULL' failed.\n"); } { struct support_capture_subprocess result; result = support_capture_subprocess (do_test_bz20544_cxa_atexit, NULL); support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT, sc_allow_stderr); TEST_COMPARE_STRING (result.err.buffer, "tst-bz20544: cxa_atexit.c:41: __internal_atexit: " \ "Assertion `func != NULL' failed.\n"); } #endif /* NDEBUG */ return 0; } ---
On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: Thanks for the review. > #else > { > struct support_capture_subprocess result; > result = support_capture_subprocess (do_test_bz20544_atexit, NULL); > support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT, > sc_allow_stderr); This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134. I notice that in libio/tst-vtables-common.c, Florian first initialized expected SIGABRT termination_status in init_termination_status(), and then used that in support_capture_subprocess_check() calls. Do I need to do the same here? A different way to ask: do different OSes encode WIFSIGNALED / WIFEXITED differently? Thanks,
On 28/11/2018 15:43, Paul Pluzhnikov wrote: > On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: > > Thanks for the review. > >> #else >> { >> struct support_capture_subprocess result; >> result = support_capture_subprocess (do_test_bz20544_atexit, NULL); >> support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT, >> sc_allow_stderr); > > This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134. > > I notice that in libio/tst-vtables-common.c, Florian first initialized > expected SIGABRT termination_status in init_termination_status(), and > then used that in support_capture_subprocess_check() calls. Do I need > to do the same here? > > A different way to ask: do different OSes encode WIFSIGNALED / > WIFEXITED differently? Yes, this is the most portable way indeed to get the expected termination value and I think we should use the same strategy on this tests. However I wouldn't not expected that Linux with same arch would have different returned values.
On Wed, Nov 28, 2018 at 10:19 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > Yes, this is the most portable way indeed to get the expected termination > value and I think we should use the same strategy on this tests. Revised patch attached. Thanks!
* Paul Pluzhnikov: > On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella > <adhemerval.zanella@linaro.org> wrote: > > Thanks for the review. > >> #else >> { >> struct support_capture_subprocess result; >> result = support_capture_subprocess (do_test_bz20544_atexit, NULL); >> support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT, >> sc_allow_stderr); > > This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134. > > I notice that in libio/tst-vtables-common.c, Florian first initialized > expected SIGABRT termination_status in init_termination_status(), and > then used that in support_capture_subprocess_check() calls. Do I need > to do the same here? You can try the patch posted below. The initial API design was a bit lazy. > A different way to ask: do different OSes encode WIFSIGNALED / > WIFEXITED differently? Yes, but all glibc targets currently behave the same. I don't expect this to change because the a lot of things are hard-coded (see the JRuby/JVM discussion on reporting signal termination). We should probably add macros to go into the other direction to <sys/wait.h>, given that the layout is unlikely to change, ever. Thanks, Florian ----- support: Add signal support to support_capture_subprocess_check Adjust libio/tst-vtables-common.c to use this new functionality, instead of determining the termination status for a signal indirectly. 2018-11-28 Florian Weimer <fweimer@redhat.com> support: Add signal support to support_capture_subprocess_check. * support/capture_subprocess.h (support_capture_subprocess_check): Adjust comment and rename parameter. * support/support_capture_subprocess_check.c (print_actual_status): New function. (support_capture_subprocess_check): Support negative status_or_signal. Call print_actual_status. * support/tst-support_capture_subprocess.c (do_test): Call support_capture_subprocess_check. * libio/tst-vtables-common.c (termination_status) (init_termination_status): Remove. (check_for_termination): Adjust support_capture_subprocess_check call. (do_test): Remove call to init_termination_status. diff --git a/libio/tst-vtables-common.c b/libio/tst-vtables-common.c index 5e31012069..85e246cd11 100644 --- a/libio/tst-vtables-common.c +++ b/libio/tst-vtables-common.c @@ -380,21 +380,6 @@ without_compatibility_fflush (void *closure) _exit (1); } -/* Exit status after abnormal termination. */ -static int termination_status; - -static void -init_termination_status (void) -{ - pid_t pid = xfork (); - if (pid == 0) - abort (); - xwaitpid (pid, &termination_status, 0); - - TEST_VERIFY (WIFSIGNALED (termination_status)); - TEST_COMPARE (WTERMSIG (termination_status), SIGABRT); -} - static void check_for_termination (const char *name, void (*callback) (void *)) { @@ -404,7 +389,7 @@ check_for_termination (const char *name, void (*callback) (void *)) shared->calls = 0; struct support_capture_subprocess proc = support_capture_subprocess (callback, NULL); - support_capture_subprocess_check (&proc, name, termination_status, + support_capture_subprocess_check (&proc, name, -SIGABRT, sc_allow_stderr); const char *message = "Fatal error: glibc detected an invalid stdio handle\n"; @@ -491,7 +476,6 @@ run_tests (bool initially_disabled) shared = support_shared_allocate (sizeof (*shared)); shared->initially_disabled = initially_disabled; - init_termination_status (); if (initially_disabled) { diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h index b0886ba1d1..4734b877ad 100644 --- a/support/capture_subprocess.h +++ b/support/capture_subprocess.h @@ -49,13 +49,16 @@ enum support_capture_allow sc_allow_stderr = 0x04, }; -/* Check that the subprocess exited with STATUS and that only the - allowed outputs happened. ALLOWED is a combination of - support_capture_allow flags. Report errors under the CONTEXT - message. */ +/* Check that the subprocess exited and that only the allowed outputs + happened. If STATUS_OR_SIGNAL is nonnegative, it is the expected + (decoded) exit status of the process, as returned by WEXITSTATUS. + If STATUS_OR_SIGNAL is negative, -STATUS_OR_SIGNAL is the expected + termination signal, as returned by WTERMSIG. ALLOWED is a + combination of support_capture_allow flags. Report errors under + the CONTEXT message. */ void support_capture_subprocess_check (struct support_capture_subprocess *, - const char *context, int status, - int allowed) + const char *context, + int status_or_signal, int allowed) __attribute__ ((nonnull (1, 2))); #endif /* SUPPORT_CAPTURE_SUBPROCESS_H */ diff --git a/support/support_capture_subprocess_check.c b/support/support_capture_subprocess_check.c index ff5ee89fb0..8b4c352c96 100644 --- a/support/support_capture_subprocess_check.c +++ b/support/support_capture_subprocess_check.c @@ -20,6 +20,7 @@ #include <stdio.h> #include <support/capture_subprocess.h> #include <support/check.h> +#include <sys/wait.h> static void print_context (const char *context, bool *failed) @@ -31,9 +32,22 @@ print_context (const char *context, bool *failed) printf ("error: subprocess failed: %s\n", context); } +static void +print_actual_status (struct support_capture_subprocess *proc) +{ + if (WIFEXITED (proc->status)) + printf ("error: actual exit status: %d [0x%x]\n", + WEXITSTATUS (proc->status), proc->status); + else if (WIFSIGNALED (proc->status)) + printf ("error: actual termination signal: %d [0x%x]\n", + WTERMSIG (proc->status), proc->status); + else + printf ("error: actual undecoded exit status: [0x%x]\n", proc->status); +} + void support_capture_subprocess_check (struct support_capture_subprocess *proc, - const char *context, int status, + const char *context, int status_or_signal, int allowed) { TEST_VERIFY ((allowed & sc_allow_none) @@ -44,11 +58,28 @@ support_capture_subprocess_check (struct support_capture_subprocess *proc, || (allowed & sc_allow_stderr)))); bool failed = false; - if (proc->status != status) + if (status_or_signal >= 0) { - print_context (context, &failed); - printf ("error: expected exit status: %d\n", status); - printf ("error: actual exit status: %d\n", proc->status); + /* Expect regular termination. */ + if (!(WIFEXITED (proc->status) + && WEXITSTATUS (proc->status) == status_or_signal)) + { + print_context (context, &failed); + printf ("error: expected exit status: %d\n", status_or_signal); + print_actual_status (proc); + } + } + else + { + /* status_or_signal < 0. Expect termination by signal. */ + if (!(WIFSIGNALED (proc->status) + && WTERMSIG (proc->status) == -status_or_signal)) + { + print_context (context, &failed); + printf ("error: expected termination signal: %d\n", + -status_or_signal); + print_actual_status (proc); + } } if (!(allowed & sc_allow_stdout) && proc->out.length != 0) { diff --git a/support/tst-support_capture_subprocess.c b/support/tst-support_capture_subprocess.c index a685256091..5339e85b07 100644 --- a/support/tst-support_capture_subprocess.c +++ b/support/tst-support_capture_subprocess.c @@ -168,15 +168,29 @@ do_test (void) = support_capture_subprocess (callback, &test); check_stream ("stdout", &result.out, test.out); check_stream ("stderr", &result.err, test.err); + + /* Allowed output for support_capture_subprocess_check. */ + int check_allow = 0; + if (lengths[length_idx_stdout] > 0) + check_allow |= sc_allow_stdout; + if (lengths[length_idx_stderr] > 0) + check_allow |= sc_allow_stderr; + if (check_allow == 0) + check_allow = sc_allow_none; + if (test.signal != 0) { TEST_VERIFY (WIFSIGNALED (result.status)); TEST_VERIFY (WTERMSIG (result.status) == test.signal); + support_capture_subprocess_check (&result, "signal", + -SIGTERM, check_allow); } else { TEST_VERIFY (WIFEXITED (result.status)); TEST_VERIFY (WEXITSTATUS (result.status) == test.status); + support_capture_subprocess_check (&result, "exit", + test.status, check_allow); } support_capture_subprocess_free (&result); free (test.out);
On Wed, Nov 28, 2018 at 11:07 AM Florian Weimer <fweimer@redhat.com> wrote: > You can try the patch posted below. The initial API design was a bit > lazy. Looks good to me. I'll update my patch once you commit this one. Thanks!
On 28/11/2018 17:07, Florian Weimer wrote: > * Paul Pluzhnikov: > >> On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella >> <adhemerval.zanella@linaro.org> wrote: >> >> Thanks for the review. >> >>> #else >>> { >>> struct support_capture_subprocess result; >>> result = support_capture_subprocess (do_test_bz20544_atexit, NULL); >>> support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT, >>> sc_allow_stderr); >> >> This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134. >> >> I notice that in libio/tst-vtables-common.c, Florian first initialized >> expected SIGABRT termination_status in init_termination_status(), and >> then used that in support_capture_subprocess_check() calls. Do I need >> to do the same here? > > You can try the patch posted below. The initial API design was a bit > lazy. > >> A different way to ask: do different OSes encode WIFSIGNALED / >> WIFEXITED differently? > > Yes, but all glibc targets currently behave the same. I don't expect > this to change because the a lot of things are hard-coded (see the > JRuby/JVM discussion on reporting signal termination). Do you have a link for the discussion? > > We should probably add macros to go into the other direction to > <sys/wait.h>, given that the layout is unlikely to change, ever. Is is something usually required and/or reimplemented elsewhere? > > Thanks, > Florian > > ----- > support: Add signal support to support_capture_subprocess_check > > Adjust libio/tst-vtables-common.c to use this new functionality, > instead of determining the termination status for a signal indirectly. > > 2018-11-28 Florian Weimer <fweimer@redhat.com> > > support: Add signal support to support_capture_subprocess_check. > * support/capture_subprocess.h (support_capture_subprocess_check): > Adjust comment and rename parameter. > * support/support_capture_subprocess_check.c > (print_actual_status): New function. > (support_capture_subprocess_check): Support negative > status_or_signal. Call print_actual_status. > * support/tst-support_capture_subprocess.c (do_test): Call > support_capture_subprocess_check. > * libio/tst-vtables-common.c (termination_status) > (init_termination_status): Remove. > (check_for_termination): Adjust support_capture_subprocess_check > call. > (do_test): Remove call to init_termination_status. LGTM, thanks. > > diff --git a/libio/tst-vtables-common.c b/libio/tst-vtables-common.c > index 5e31012069..85e246cd11 100644 > --- a/libio/tst-vtables-common.c > +++ b/libio/tst-vtables-common.c > @@ -380,21 +380,6 @@ without_compatibility_fflush (void *closure) > _exit (1); > } > > -/* Exit status after abnormal termination. */ > -static int termination_status; > - > -static void > -init_termination_status (void) > -{ > - pid_t pid = xfork (); > - if (pid == 0) > - abort (); > - xwaitpid (pid, &termination_status, 0); > - > - TEST_VERIFY (WIFSIGNALED (termination_status)); > - TEST_COMPARE (WTERMSIG (termination_status), SIGABRT); > -} > - > static void > check_for_termination (const char *name, void (*callback) (void *)) > { > @@ -404,7 +389,7 @@ check_for_termination (const char *name, void (*callback) (void *)) > shared->calls = 0; > struct support_capture_subprocess proc > = support_capture_subprocess (callback, NULL); > - support_capture_subprocess_check (&proc, name, termination_status, > + support_capture_subprocess_check (&proc, name, -SIGABRT, > sc_allow_stderr); > const char *message > = "Fatal error: glibc detected an invalid stdio handle\n"; > @@ -491,7 +476,6 @@ run_tests (bool initially_disabled) > > shared = support_shared_allocate (sizeof (*shared)); > shared->initially_disabled = initially_disabled; > - init_termination_status (); > > if (initially_disabled) > { Ok. > diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h > index b0886ba1d1..4734b877ad 100644 > --- a/support/capture_subprocess.h > +++ b/support/capture_subprocess.h > @@ -49,13 +49,16 @@ enum support_capture_allow > sc_allow_stderr = 0x04, > }; > > -/* Check that the subprocess exited with STATUS and that only the > - allowed outputs happened. ALLOWED is a combination of > - support_capture_allow flags. Report errors under the CONTEXT > - message. */ > +/* Check that the subprocess exited and that only the allowed outputs > + happened. If STATUS_OR_SIGNAL is nonnegative, it is the expected > + (decoded) exit status of the process, as returned by WEXITSTATUS. > + If STATUS_OR_SIGNAL is negative, -STATUS_OR_SIGNAL is the expected > + termination signal, as returned by WTERMSIG. ALLOWED is a > + combination of support_capture_allow flags. Report errors under > + the CONTEXT message. */ > void support_capture_subprocess_check (struct support_capture_subprocess *, > - const char *context, int status, > - int allowed) > + const char *context, > + int status_or_signal, int allowed) > __attribute__ ((nonnull (1, 2))); > > #endif /* SUPPORT_CAPTURE_SUBPROCESS_H */ Ok. > diff --git a/support/support_capture_subprocess_check.c b/support/support_capture_subprocess_check.c > index ff5ee89fb0..8b4c352c96 100644 > --- a/support/support_capture_subprocess_check.c > +++ b/support/support_capture_subprocess_check.c > @@ -20,6 +20,7 @@ > #include <stdio.h> > #include <support/capture_subprocess.h> > #include <support/check.h> > +#include <sys/wait.h> > > static void > print_context (const char *context, bool *failed) > @@ -31,9 +32,22 @@ print_context (const char *context, bool *failed) > printf ("error: subprocess failed: %s\n", context); > } > > +static void > +print_actual_status (struct support_capture_subprocess *proc) > +{ > + if (WIFEXITED (proc->status)) > + printf ("error: actual exit status: %d [0x%x]\n", > + WEXITSTATUS (proc->status), proc->status); > + else if (WIFSIGNALED (proc->status)) > + printf ("error: actual termination signal: %d [0x%x]\n", > + WTERMSIG (proc->status), proc->status); > + else > + printf ("error: actual undecoded exit status: [0x%x]\n", proc->status); > +} > +> void > support_capture_subprocess_check (struct support_capture_subprocess *proc, > - const char *context, int status, > + const char *context, int status_or_signal, > int allowed) > { > TEST_VERIFY ((allowed & sc_allow_none) > @@ -44,11 +58,28 @@ support_capture_subprocess_check (struct support_capture_subprocess *proc, > || (allowed & sc_allow_stderr)))); > > bool failed = false; > - if (proc->status != status) > + if (status_or_signal >= 0) > { > - print_context (context, &failed); > - printf ("error: expected exit status: %d\n", status); > - printf ("error: actual exit status: %d\n", proc->status); > + /* Expect regular termination. */ > + if (!(WIFEXITED (proc->status) > + && WEXITSTATUS (proc->status) == status_or_signal)) > + { > + print_context (context, &failed); > + printf ("error: expected exit status: %d\n", status_or_signal); > + print_actual_status (proc); > + } > + } > + else > + { > + /* status_or_signal < 0. Expect termination by signal. */ > + if (!(WIFSIGNALED (proc->status) > + && WTERMSIG (proc->status) == -status_or_signal)) > + { > + print_context (context, &failed); > + printf ("error: expected termination signal: %d\n", > + -status_or_signal); > + print_actual_status (proc); > + } > } > if (!(allowed & sc_allow_stdout) && proc->out.length != 0) > { Ok. > diff --git a/support/tst-support_capture_subprocess.c b/support/tst-support_capture_subprocess.c > index a685256091..5339e85b07 100644 > --- a/support/tst-support_capture_subprocess.c > +++ b/support/tst-support_capture_subprocess.c > @@ -168,15 +168,29 @@ do_test (void) > = support_capture_subprocess (callback, &test); > check_stream ("stdout", &result.out, test.out); > check_stream ("stderr", &result.err, test.err); > + > + /* Allowed output for support_capture_subprocess_check. */ > + int check_allow = 0; > + if (lengths[length_idx_stdout] > 0) > + check_allow |= sc_allow_stdout; > + if (lengths[length_idx_stderr] > 0) > + check_allow |= sc_allow_stderr; > + if (check_allow == 0) > + check_allow = sc_allow_none; > + > if (test.signal != 0) > { > TEST_VERIFY (WIFSIGNALED (result.status)); > TEST_VERIFY (WTERMSIG (result.status) == test.signal); > + support_capture_subprocess_check (&result, "signal", > + -SIGTERM, check_allow); > } > else > { > TEST_VERIFY (WIFEXITED (result.status)); > TEST_VERIFY (WEXITSTATUS (result.status) == test.status); > + support_capture_subprocess_check (&result, "exit", > + test.status, check_allow); > } > support_capture_subprocess_free (&result); > free (test.out); > Ok.
* Adhemerval Zanella: >> Yes, but all glibc targets currently behave the same. I don't expect >> this to change because the a lot of things are hard-coded (see the >> JRuby/JVM discussion on reporting signal termination). > > Do you have a link for the discussion? I think this it: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-June/028864.html> In particular this: <http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-June/028885.html> Which mentions: Shutdown.exit(sig.getNumber() + 0200); >> We should probably add macros to go into the other direction to >> <sys/wait.h>, given that the layout is unlikely to change, ever. > > Is is something usually required and/or reimplemented elsewhere? I think it would be useful for writing tests at the very least. Thanks, Florian
* Paul Pluzhnikov: > On Wed, Nov 28, 2018 at 11:07 AM Florian Weimer <fweimer@redhat.com> wrote: > >> You can try the patch posted below. The initial API design was a bit >> lazy. > > Looks good to me. I'll update my patch once you commit this one. Thanks, committed. Florian
On Wed, Nov 28, 2018 at 12:14 PM Florian Weimer <fweimer@redhat.com> wrote:
> Thanks, committed.
Updated patch attached. Thanks!
* Paul Pluzhnikov: > Subject: [PATCH] BZ #20544 assert on NULL fn in atexit, etc. Perhaps: stdlib: assert on NULL function pointer in atexit etc. [BZ #20544] (Or “(bug 20544)” if that's the syntax you prefer.) > + /* As a QoI issue we detect NULL early with an assertion instead > + of a SIGSEGV at program exit when the handler is run. BZ#20544 */ > + assert (func != NULL); > + /* As a QoI issue we detect NULL early with an assertion instead > + of a SIGSEGV at program exit when the handler is run. BZ#20544 */ I would write: “is run (bug 20544). */” > + assert (func != NULL); > + > __libc_lock_lock (__exit_funcs_lock); > new = __new_exitfn (&__exit_funcs); > > diff --git a/stdlib/tst-bz20544.c b/stdlib/tst-bz20544.c > new file mode 100644 > index 0000000000..98fe199eac > --- /dev/null > +++ b/stdlib/tst-bz20544.c > +#pragma GCC diagnostic ignored "-Wnonnull" The documentation for the “nonnull” attribute says this: | The compiler may also choose to make optimizations based on the | knowledge that certain function arguments will never be null. So presumably GCC could emit a trap for these calls, or just drop the calls altogher. Sorry. 8-( I think you need to remove the pragma and use this instead (see stdio-common/tst-vfprintf-user-type.c for an existing example): extern int atexit_alias (void (*) (void)) __asm__ ("atexit"); extern int on_exit_alias (void (*) (void)) __asm__ ("on_exit"); You should also add tests for at_quick_exit and __cxa_at_quick_exit. > +#if defined(NDEBUG) > + FAIL_UNSUPPORTED("Assertions disabled (NDEBUG). " Space before opening paren (twice). > + "Can't verify that assertions fire."); > +#else You could drop the #else part because the code will compile just fine. > + { > + struct support_capture_subprocess result; > + result = support_capture_subprocess (do_test_bz20544_atexit, NULL); > + support_capture_subprocess_check (&result, "bz20544", -SIGABRT, > + sc_allow_stderr); > + TEST_COMPARE_STRING (result.err.buffer, > + "tst-bz20544: cxa_atexit.c:41: __internal_atexit: " \ > + "Assertion `func != NULL' failed.\n"); You don't need the \ at the end of the line (three times). But it may be better to search for "Assertion `func != NULL' failed.\n" instead using strstr, so that we do not have this line number dependency in the test. Within the test, this would be fine, but across library in test, I don't think that's a good idea. Thanks, Florian
On Thu, Nov 29, 2018 at 7:06 AM Florian Weimer <fweimer@redhat.com> wrote: > Perhaps: > > stdlib: assert on NULL function pointer in atexit etc. [BZ #20544] Thanks. Revised patch attached.
* Paul Pluzhnikov: > On Thu, Nov 29, 2018 at 7:06 AM Florian Weimer <fweimer@redhat.com> wrote: > >> Perhaps: >> >> stdlib: assert on NULL function pointer in atexit etc. [BZ #20544] > > Thanks. Revised patch attached. I think this version is okay. Thanks, Florian
On Thu, Nov 29, 2018 at 8:33 AM Florian Weimer <fweimer@redhat.com> wrote:
> I think this version is okay.
Thanks. I'll submit this on 2018-12-01 if there are no further
comments until then.
diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c index 6d65f7e615..978f873990 100644 --- a/stdlib/cxa_atexit.c +++ b/stdlib/cxa_atexit.c @@ -36,6 +36,9 @@ __internal_atexit (void (*func) (void *), void *arg, void *d, { struct exit_function *new; + /* BZ#20544 */ + assert (func != NULL); + __libc_lock_lock (__exit_funcs_lock); new = __new_exitfn (listp); diff --git a/stdlib/on_exit.c b/stdlib/on_exit.c index 5241e0d86f..0ead1eafd4 100644 --- a/stdlib/on_exit.c +++ b/stdlib/on_exit.c @@ -15,6 +15,7 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ +#include <assert.h> #include <stdlib.h> #include "exit.h" #include <sysdep.h> @@ -25,6 +26,9 @@ __on_exit (void (*func) (int status, void *arg), void *arg) { struct exit_function *new; + /* BZ#20544 */ + assert (func != NULL); + __libc_lock_lock (__exit_funcs_lock); new = __new_exitfn (&__exit_funcs);
Greetings, As discussed in bz#20544 and in http://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers, we should assert that the function passed to atexit, on_exit, etc. is not NULL. 2018-08-27 Paul Pluzhnikov <ppluzhnikov@google.com> [BZ #20544] * stdlib/cxa_atexit.c (__internal_atexit): assert func != NULL. * stdlib/on_exit.c (__on_exit): Likewise.