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

login
register
mail settings
Submitter Paul Pluzhnikov
Date Nov. 13, 2018, 5:39 p.m.
Message ID <CALoOobO2MwM5W-iOY7VtZBgsaUxNs5G4r_fMpMhDA2TM8iqCcg@mail.gmail.com>
Download mbox | patch
Permalink /patch/30135/
State New
Headers show

Comments

Paul Pluzhnikov - Nov. 13, 2018, 5:39 p.m.
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.
Gabriel F. T. Gomes - Nov. 14, 2018, 4:51 p.m.
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.
* 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

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>