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

Message ID CALoOobN2H0dRjGUT_XSwYSO0pE_kWyVDoGrg+dSN_J4J5zpyJQ@mail.gmail.com
State New, archived
Headers

Commit Message

Paul Pluzhnikov Aug. 27, 2018, 6:12 p.m. UTC
  Greetings,

As discussed in bz#20544 and in
http://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers,
we should assert that the function passed to atexit, on_exit, etc. is
not NULL.

2018-08-27  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #20544]
        * stdlib/cxa_atexit.c (__internal_atexit): assert func != NULL.
        * stdlib/on_exit.c (__on_exit): Likewise.
  

Comments

Carlos O'Donell Aug. 27, 2018, 8:02 p.m. UTC | #1
On 08/27/2018 02:12 PM, Paul Pluzhnikov wrote:
> Greetings,
> 
> As discussed in bz#20544 and in
> http://sourceware.org/glibc/wiki/Style_and_Conventions#Invalid_pointers,
> we should assert that the function passed to atexit, on_exit, etc. is
> not NULL.

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.

The test should cover all of the APIs we can tests by passing NULL.
It is OK to call __cxa_atexit directly even though that might be invalid
in a real program.

> 2018-08-27  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>         [BZ #20544]
>         * stdlib/cxa_atexit.c (__internal_atexit): assert func != NULL.
>         * stdlib/on_exit.c (__on_exit): Likewise.
> 
> 
> -- Paul Pluzhnikov
> 
> 
> glibc-bz20544-20180825.txt
> 
> 
> diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
> index 6d65f7e615..978f873990 100644
> --- a/stdlib/cxa_atexit.c
> +++ b/stdlib/cxa_atexit.c
> @@ -36,6 +36,9 @@ __internal_atexit (void (*func) (void *), void *arg, void *d,
>  {
>    struct exit_function *new;
>  
> +  /* BZ#20544  */

This should comment on why the assert is here.

/* As a QoI issue we detect NULL early with an assertion instead
   of a SIGSEGV at program exit when the handler is run.  */

> +  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..0ead1eafd4 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>

OK.

>  #include <stdlib.h>
>  #include "exit.h"
>  #include <sysdep.h>
> @@ -25,6 +26,9 @@ __on_exit (void (*func) (int status, void *arg), void *arg)
>  {
>    struct exit_function *new;
>  
> +  /* BZ#20544  */

Likewise.

> +  assert (func != NULL);
> +
>     __libc_lock_lock (__exit_funcs_lock);
>    new = __new_exitfn (&__exit_funcs);
>
  

Patch

diff --git a/stdlib/cxa_atexit.c b/stdlib/cxa_atexit.c
index 6d65f7e615..978f873990 100644
--- a/stdlib/cxa_atexit.c
+++ b/stdlib/cxa_atexit.c
@@ -36,6 +36,9 @@  __internal_atexit (void (*func) (void *), void *arg, void *d,
 {
   struct exit_function *new;
 
+  /* 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..0ead1eafd4 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,9 @@  __on_exit (void (*func) (int status, void *arg), void *arg)
 {
   struct exit_function *new;
 
+  /* BZ#20544  */
+  assert (func != NULL);
+
    __libc_lock_lock (__exit_funcs_lock);
   new = __new_exitfn (&__exit_funcs);