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

  On Wed, Nov 14, 2018 at 10:01 AM Florian Weimer <fweimer@redhat.com> wrote:

> I think this will print assertion failure messages to the build log,

Indeed it does:

tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func !=
NULL' failed.
tst-bz20544: on_exit.c:31: __on_exit: Assertion `func != NULL' failed.
tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func !=
NULL' failed.

> which is confusing to anyone reviewing it for unexpected errors.  Can we
> avoid this?

Revised patch attached. Thanks!
  

On Thu, Nov 15, 2018 at 1:38 PM Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
>
> On Wed, Nov 14, 2018 at 10:01 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> > I think this will print assertion failure messages to the build log,

> Revised patch attached. Thanks!

Ping?
  

On 15/11/2018 19:38, Paul Pluzhnikov wrote:
> On Wed, Nov 14, 2018 at 10:01 AM Florian Weimer <fweimer@redhat.com> wrote:
> 
>> I think this will print assertion failure messages to the build log,
> Indeed it does:
> 
> tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func !=
> NULL' failed.
> tst-bz20544: on_exit.c:31: __on_exit: Assertion `func != NULL' failed.
> tst-bz20544: cxa_atexit.c:41: __internal_atexit: Assertion `func !=
> NULL' failed.
> 
>> which is confusing to anyone reviewing it for unexpected errors.  Can we
>> avoid this?
> Revised patch attached. Thanks!
> -- Paul Pluzhnikov
> 
> 
> glibc-bz20544-20181115.txt
> 
> From 83269977989f071105419bc15dbf0b992db8db5e 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 | 75 ++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 93 insertions(+), 1 deletion(-)
>  create mode 100644 stdlib/tst-bz20544.c
> 
> diff --git a/ChangeLog b/ChangeLog
> index 149f991b70..be1a4606c1 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,11 @@
> +2018-11-15  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-14  Samuel Thibault  <samuel.thibault@ens-lyon.org>
>  
>  	* sysdeps/mach/hurd/dl-sysdep.c (check_no_hidden): Use
> 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..79b0f9b561
> --- /dev/null
> +++ b/stdlib/tst-bz20544.c
> @@ -0,0 +1,75 @@
> +/* 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++) {

Wrong indentation, open bracket should be aligned on next line.

> +    pid_t pid = xfork ();
> +    if (pid == 0) {
> +      /* Child.  Close stderr so we don't pollute build log with (expected)
> +         assertion failures.  */
> +      close (STDERR_FILENO);

I think instead of using xfork, this test should use support_capture_subprocess
instead. It already takes care of handling the file descriptor, reaping the
child and checking for expected termination value.  In this case:

---
#ifndef NDEBUG
static void
do_test_bz20544_atexit (void *closure)
{
  atexit (NULL);  /* should assert.  */
  exit (EXIT_FAILURE);
}

static void
do_test_bz20544_on_exit (void *closure)
{
  on_exit (NULL, NULL);  /* should assert.  */
  exit (EXIT_FAILURE);
}

static void
do_test_bz20544_cxa_atexit (void *closure)
{
  __cxa_atexit (NULL, NULL, NULL);  /* Should assert.  */
  exit (EXIT_FAILURE);
}
#endif

static int
do_test (void)
{
#ifdef NDEBUG
  FAIL_UNSUPPORTED("Assertions disabled (NDEBUG defined). "
                   "Can't verify that assertions fire.");
#else
  {
    struct support_capture_subprocess result;
    result = support_capture_subprocess (do_test_bz20544_atexit, NULL);
    support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
                                      sc_allow_stderr);
    TEST_COMPARE_STRING (result.err.buffer,
                         "tst-bz20544: cxa_atexit.c:41: __internal_atexit: " \
                         "Assertion `func != NULL' failed.\n");
  }

  {
    struct support_capture_subprocess result;
    result = support_capture_subprocess (do_test_bz20544_on_exit, NULL);
    support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
                                      sc_allow_stderr);
    TEST_COMPARE_STRING (result.err.buffer,
                         "tst-bz20544: on_exit.c:31: __on_exit: " \
                         "Assertion `func != NULL' failed.\n");
  }

  {
    struct support_capture_subprocess result;
    result = support_capture_subprocess (do_test_bz20544_cxa_atexit, NULL);
    support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
                                      sc_allow_stderr);
    TEST_COMPARE_STRING (result.err.buffer,
                         "tst-bz20544: cxa_atexit.c:41: __internal_atexit: " \
                         "Assertion `func != NULL' failed.\n");
  }
#endif  /* NDEBUG  */

  return 0;
}
---
  

On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:

Thanks for the review.

> #else
>   {
>     struct support_capture_subprocess result;
>     result = support_capture_subprocess (do_test_bz20544_atexit, NULL);
>     support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
>                                       sc_allow_stderr);

This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134.

I notice that in libio/tst-vtables-common.c, Florian first initialized
expected SIGABRT termination_status in init_termination_status(), and
then used that in support_capture_subprocess_check() calls. Do I need
to do the same here?

A different way to ask: do different OSes encode WIFSIGNALED /
WIFEXITED differently?

Thanks,
  

On 28/11/2018 15:43, Paul Pluzhnikov wrote:
> On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
> 
> Thanks for the review.
> 
>> #else
>>   {
>>     struct support_capture_subprocess result;
>>     result = support_capture_subprocess (do_test_bz20544_atexit, NULL);
>>     support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
>>                                       sc_allow_stderr);
> 
> This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134.
> 
> I notice that in libio/tst-vtables-common.c, Florian first initialized
> expected SIGABRT termination_status in init_termination_status(), and
> then used that in support_capture_subprocess_check() calls. Do I need
> to do the same here?
> 
> A different way to ask: do different OSes encode WIFSIGNALED /
> WIFEXITED differently?

Yes, this is the most portable way indeed to get the expected termination
value and I think we should use the same strategy on this tests.

However I wouldn't not expected that Linux with same arch would have
different returned values.