Re: [patch] Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL)

[Florian Weimer <fweimer at redhat dot com>]

* Paul Pluzhnikov:

> Subject: [PATCH] BZ #20544 assert on NULL fn in atexit, etc.

Perhaps:

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

(Or “(bug 20544)” if that's the syntax you prefer.)

> +  /* 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);

> +  /* 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  */

I would write: “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..98fe199eac
> --- /dev/null
> +++ b/stdlib/tst-bz20544.c

> +#pragma GCC diagnostic ignored "-Wnonnull"

The documentation for the “nonnull” attribute says this:

| The compiler may also choose to make optimizations based on the
| knowledge that certain function arguments will never be null.

So presumably GCC could emit a trap for these calls, or just drop the
calls altogher.  Sorry.  8-(

I think you need to remove the pragma and use this instead (see
stdio-common/tst-vfprintf-user-type.c for an existing example):

extern int atexit_alias (void (*) (void)) __asm__ ("atexit");
extern int on_exit_alias (void (*) (void)) __asm__ ("on_exit");

You should also add tests for at_quick_exit and __cxa_at_quick_exit.

> +#if defined(NDEBUG)
> +  FAIL_UNSUPPORTED("Assertions disabled (NDEBUG). "

Space before opening paren (twice).

> +                   "Can't verify that assertions fire.");
> +#else

You could drop the #else part because the code will compile just fine.

> +  {
> +    struct support_capture_subprocess result;
> +    result = support_capture_subprocess (do_test_bz20544_atexit, NULL);
> +    support_capture_subprocess_check (&result, "bz20544", -SIGABRT,
> +                                      sc_allow_stderr);
> +    TEST_COMPARE_STRING (result.err.buffer,
> +                         "tst-bz20544: cxa_atexit.c:41: __internal_atexit: " \
> +                         "Assertion `func != NULL' failed.\n");

You don't need the \ at the end of the line (three times).

But it may be better to search for "Assertion `func != NULL' failed.\n"
instead using strstr, so that we do not have this line number dependency
in the test.  Within the test, this would be fine, but across library in
test, I don't think that's a good idea.

Thanks,
Florian