| Message ID | CALoOobMDtHke_UPTEXhJ+Pui7oQJscZSy=L7qjSAvYHT7WNP+g@mail.gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
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.
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++) { + pid_t pid = xfork (); + if (pid == 0) { + /* Child. Close stderr so we don't pollute build log with (expected) + assertion failures. */ + close (STDERR_FILENO); + + 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; +} + +#include <support/test-driver.c>