Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL)
Commit Message
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!
Comments
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.
@@ -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
@@ -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
@@ -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);
@@ -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);
new file mode 100644
@@ -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>