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

login
register
mail settings
Submitter Paul Pluzhnikov
Date Sept. 1, 2018, 5:51 p.m.
Message ID <CALoOobPARbnzay8bSpGPXPxAJp-wkOvFFaQT1z8cbsQeGCxJYw@mail.gmail.com>
Download mbox | patch
Permalink /patch/29161/
State New
Headers show

Comments

Paul Pluzhnikov - Sept. 1, 2018, 5:51 p.m.
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
Paul Pluzhnikov - Nov. 11, 2018, 10:37 p.m.
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?
Gabriel F. T. Gomes - Nov. 13, 2018, 1:59 p.m.
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>

Patch

diff --git a/ChangeLog b/ChangeLog
index d11440bc0f..4984b134cc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -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]
diff --git a/stdlib/Makefile b/stdlib/Makefile
index 01194bbf7c..c7f889b190 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..a472ebaa10
--- /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)
+  /* 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>