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

login
register
mail settings
Submitter Paul Pluzhnikov
Date Nov. 29, 2018, 4:20 p.m.
Message ID <CALoOobPFPXXePV1PgeEAsMBzwFA+NmEJPaGa-tGRuH6d0-rS0Q@mail.gmail.com>
Download mbox | patch
Permalink /patch/30391/
State New
Headers show

Comments

Paul Pluzhnikov - Nov. 29, 2018, 4:20 p.m.
On Thu, Nov 29, 2018 at 7:06 AM Florian Weimer <fweimer@redhat.com> wrote:

> Perhaps:
>
>     stdlib: assert on NULL function pointer in atexit etc. [BZ #20544]

Thanks. Revised patch attached.
Florian Weimer - Nov. 29, 2018, 4:33 p.m.
* Paul Pluzhnikov:

> On Thu, Nov 29, 2018 at 7:06 AM Florian Weimer <fweimer@redhat.com> wrote:
>
>> Perhaps:
>>
>>     stdlib: assert on NULL function pointer in atexit etc. [BZ #20544]
>
> Thanks. Revised patch attached.

I think this version is okay.

Thanks,
Florian
Paul Pluzhnikov - Nov. 29, 2018, 6:16 p.m.
On Thu, Nov 29, 2018 at 8:33 AM Florian Weimer <fweimer@redhat.com> wrote:

> I think this version is okay.

Thanks. I'll submit this on 2018-12-01 if there are no further
comments until then.

Patch

diff --git a/ChangeLog b/ChangeLog
index 488774e236..b27cc6f0a4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@ 
+2018-11-29  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-29  Florian Weimer  <fweimer@redhat.com>
 
 	* posix/Makefile (before-compile): Remove testcases.h and
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..37b22d5801 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 (bug 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..1dff7ff631 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 (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..33b87cc77e
--- /dev/null
+++ b/stdlib/tst-bz20544.c
@@ -0,0 +1,115 @@ 
+/* 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 <string.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);
+extern int __cxa_at_quick_exit (void (*func) (void *), void *arg, void *d);
+
+/* GCC "knows" that atexit and on_exit should not be called with NULL
+   function pointer, and emits diagnostics if we try to do so.
+   Presumably it could emit a trap and drop the call altogether.
+
+   The aliases below are intended to bypass this.  */
+
+extern int atexit_alias (void (*) (void)) __asm__ ("atexit");
+extern int at_quick_exit_alias (void (*) (void)) __asm__ ("at_quick_exit");
+extern int on_exit_alias (void (*) (void), void *) __asm__ ("on_exit");
+
+
+static void
+test_bz20544_atexit (void *closure)
+{
+  atexit_alias (NULL);  /* Should assert.  */
+  exit (EXIT_FAILURE);
+}
+
+static void
+test_bz20544_at_quick_exit (void *closure)
+{
+  at_quick_exit_alias (NULL);  /* Should assert.  */
+  exit (EXIT_FAILURE);
+}
+
+static void
+test_bz20544_on_exit (void *closure)
+{
+  on_exit_alias (NULL, NULL);  /* Should assert.  */
+  exit (EXIT_FAILURE);
+}
+
+static void
+test_bz20544_cxa_atexit (void *closure)
+{
+  __cxa_atexit (NULL, NULL, NULL);  /* Should assert.  */
+  exit (EXIT_FAILURE);
+}
+
+static void
+test_bz20544_cxa_at_quick_exit (void *closure)
+{
+  __cxa_at_quick_exit (NULL, NULL, NULL);  /* Should assert.  */
+  exit (EXIT_FAILURE);
+}
+
+static void
+test_one_fn (void (*test_fn) (void *))
+{
+  const char expected_error[] = "Assertion `func != NULL' failed.\n";
+  struct support_capture_subprocess result;
+  result = support_capture_subprocess (test_fn, NULL);
+  support_capture_subprocess_check (&result, "bz20544", -SIGABRT,
+                                    sc_allow_stderr);
+
+  if (strstr (result.err.buffer, expected_error) == NULL)
+    {
+      support_record_failure ();
+      printf ("Did not find expected string in error output:\n"
+              "  expected: >>>%s<<<\n"
+              "  actual:   >>>%s<<<\n",
+              expected_error, result.err.buffer);
+    }
+
+  support_capture_subprocess_free (&result);
+}
+
+static int
+do_test (void)
+{
+#if defined (NDEBUG)
+  FAIL_UNSUPPORTED ("Assertions disabled (NDEBUG). "
+                    "Can't verify that assertions fire.");
+#endif
+  test_one_fn (test_bz20544_atexit);
+  test_one_fn (test_bz20544_at_quick_exit);
+  test_one_fn (test_bz20544_on_exit);
+  test_one_fn (test_bz20544_cxa_atexit);
+  test_one_fn (test_bz20544_cxa_at_quick_exit);
+
+  return 0;
+}
+
+#include <support/test-driver.c>