Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL)
Commit Message
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
Comments
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>
@@ -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]
@@ -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,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>