Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL)
Commit Message
On Wed, Nov 28, 2018 at 12:14 PM Florian Weimer <fweimer@redhat.com> wrote:
> Thanks, committed.
Updated patch attached. Thanks!
Comments
* 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
@@ -1,3 +1,11 @@
+2018-11-28 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-28 Florian Weimer <fweimer@redhat.com>
support: Add signal support to support_capture_subprocess_check.
@@ -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,99 @@
+/* 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 <assert.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <support/capture_subprocess.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+
+extern int __cxa_atexit (void (*func) (void *), void *arg, void *d);
+
+#pragma GCC diagnostic ignored "-Wnonnull"
+
+#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)
+{
+#if defined(NDEBUG)
+ FAIL_UNSUPPORTED("Assertions disabled (NDEBUG). "
+ "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", -SIGABRT,
+ sc_allow_stderr);
+ TEST_COMPARE_STRING (result.err.buffer,
+ "tst-bz20544: cxa_atexit.c:41: __internal_atexit: " \
+ "Assertion `func != NULL' failed.\n");
+ support_capture_subprocess_free (&result);
+ }
+
+ {
+ struct support_capture_subprocess result;
+ result = support_capture_subprocess (do_test_bz20544_on_exit, NULL);
+ support_capture_subprocess_check (&result, "bz20544", -SIGABRT,
+ sc_allow_stderr);
+ TEST_COMPARE_STRING (result.err.buffer,
+ "tst-bz20544: on_exit.c:31: __on_exit: " \
+ "Assertion `func != NULL' failed.\n");
+ support_capture_subprocess_free (&result);
+ }
+
+ {
+ struct support_capture_subprocess result;
+ result = support_capture_subprocess (do_test_bz20544_cxa_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");
+ support_capture_subprocess_free (&result);
+ }
+#endif /* NDEBUG */
+
+ return 0;
+}
+
+#include <support/test-driver.c>