diff mbox

Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL)

Message ID CALoOobO2MwM5W-iOY7VtZBgsaUxNs5G4r_fMpMhDA2TM8iqCcg@mail.gmail.com
State New, archived
Headers

Commit Message

Paul Pluzhnikov Nov. 13, 2018, 5:39 p.m. UTC 
  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.

Comments

Gabriel F. T. Gomes Nov. 14, 2018, 4:51 p.m. UTC | #1 
On Tue, 13 Nov 2018, Paul Pluzhnikov wrote:

>Thanks.  Revised patch attached.

Looks good to me.
Florian Weimer Nov. 14, 2018, 6:01 p.m. UTC | #2 
* 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
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 837c167a0b..59d4b9d0c0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@ 
+2018-11-13  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-13  Florian Weimer  <fweimer@redhat.com>
 
 	* malloc/malloc.c (fastbin_push_entry): New function.
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..f3b08531bd
--- /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)
+  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.  */
+      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>