[v4,04/10] malloc: Move malloc hook references to hooks.c

Make the core malloc code hooks free and instead only have entry
points for _*_debug_before inline functions.

This also introduces the first (albeit very constrained) breakage for
malloc hooks behaviour.  The hook variables no longer call
ptmalloc_init, it is called directly on first invocation of an
allocator function.  This breaks debugging hooks that may depend on
overriding the hook symbols with their own implementations due to
which ptmalloc_init is never called.  In other words, it breaks hooks
users that use the malloc hooks to have their own implementation of
malloc that is conflicting with glibc malloc, which is not the
intended use of the hooks as documented.

None of the three debugging hooks in glibc depend on this behaviour
and hence continue to work as before.  In any case, future patches
that move the hooks out into their own layer ought to bring back this
functionality.  As a result, the breakage will not be visible to the
user in the end.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 malloc/arena.c           |   2 -
 malloc/hooks.c           | 110 ++++++++++++++++++++++++++++++++-------
 malloc/malloc-internal.h |   1 +
 malloc/malloc.c          |  85 ++++++++++--------------------
 4 files changed, 119 insertions(+), 79 deletions(-)

On 7/2/21 7:38 AM, Siddhesh Poyarekar wrote:
> Make the core malloc code hooks free and instead only have entry
> points for _*_debug_before inline functions.
> 
> This also introduces the first (albeit very constrained) breakage for
> malloc hooks behaviour.  The hook variables no longer call
> ptmalloc_init, it is called directly on first invocation of an
> allocator function.  This breaks debugging hooks that may depend on
> overriding the hook symbols with their own implementations due to
> which ptmalloc_init is never called.  In other words, it breaks hooks
> users that use the malloc hooks to have their own implementation of
> malloc that is conflicting with glibc malloc, which is not the
> intended use of the hooks as documented.
> 
> None of the three debugging hooks in glibc depend on this behaviour
> and hence continue to work as before.  In any case, future patches
> that move the hooks out into their own layer ought to bring back this
> functionality.  As a result, the breakage will not be visible to the
> user in the end.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> Reviewed-by: DJ Delorie <dj@redhat.com>
> ---
>  malloc/arena.c           |   2 -
>  malloc/hooks.c           | 110 ++++++++++++++++++++++++++++++++-------
>  malloc/malloc-internal.h |   1 +
>  malloc/malloc.c          |  85 ++++++++++--------------------
>  4 files changed, 119 insertions(+), 79 deletions(-)
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 7eb110445e..1861d20006 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -44,8 +44,6 @@
>  
>  /***************************************************************************/
>  
> -#define top(ar_ptr) ((ar_ptr)->top)
> -
>  /* A heap is a single contiguous memory region holding (coalesceable)
>     malloc_chunks.  It is allocated with mmap() and always starts at an
>     address aligned to HEAP_MAX_SIZE.  */
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 57a9b55788..4960aafd08 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -21,35 +21,107 @@
>     corrupt pointer is detected: do nothing (0), print an error message
>     (1), or call abort() (2). */
>  
> -/* Hooks for debugging versions.  The initial hooks just call the
> -   initialization routine, then do the normal work. */
> +/* Define and initialize the hook variables.  These weak definitions must
> +   appear before any use of the variables in a function (arena.c uses one).  */
> +#ifndef weak_variable
> +/* In GNU libc we want the hook variables to be weak definitions to
> +   avoid a problem with Emacs.  */
> +# define weak_variable weak_function
> +#endif
> +
> +/* Forward declarations.  */
> +
> +#if HAVE_MALLOC_INIT_HOOK
> +void (*__malloc_initialize_hook) (void) __attribute__ ((nocommon));
> +compat_symbol (libc, __malloc_initialize_hook,
> +	       __malloc_initialize_hook, GLIBC_2_0);
> +#endif
> +
> +void weak_variable (*__free_hook) (void *__ptr,
> +                                   const void *) = NULL;
> +void *weak_variable (*__malloc_hook)
> +  (size_t __size, const void *) = NULL;
> +void *weak_variable (*__realloc_hook)
> +  (void *__ptr, size_t __size, const void *) = NULL;
> +void *weak_variable (*__memalign_hook)
> +  (size_t __alignment, size_t __size, const void *) = NULL;
> +
> +static void ptmalloc_init (void);
>  
> -static void *
> -malloc_hook_ini (size_t sz, const void *caller)
> +#include "malloc-check.c"
> +
> +static __always_inline bool
> +_malloc_debug_before (size_t bytes, void **victimp, const void *address)
>  {
> -  __malloc_hook = NULL;
> -  ptmalloc_init ();
> -  return __libc_malloc (sz);
> +  _Static_assert (PTRDIFF_MAX <= SIZE_MAX / 2,
> +                  "PTRDIFF_MAX is not more than half of SIZE_MAX");
> +
> +  void *(*hook) (size_t, const void *)
> +    = atomic_forced_read (__malloc_hook);
> +  if (__builtin_expect (hook != NULL, 0))
> +    {
> +      *victimp = (*hook)(bytes, address);
> +      return true;
> +    }
> +  return false;
>  }
>  
> -static void *
> -realloc_hook_ini (void *ptr, size_t sz, const void *caller)
> +static __always_inline bool
> +_free_debug_before (void *mem, const void *address)
>  {
> -  __malloc_hook = NULL;
> -  __realloc_hook = NULL;
> -  ptmalloc_init ();
> -  return __libc_realloc (ptr, sz);
> +  void (*hook) (void *, const void *)
> +    = atomic_forced_read (__free_hook);
> +  if (__builtin_expect (hook != NULL, 0))
> +    {
> +      (*hook)(mem, address);
> +      return true;
> +    }
> +  return false;
>  }
>  
> -static void *
> -memalign_hook_ini (size_t alignment, size_t sz, const void *caller)
> +static __always_inline bool
> +_realloc_debug_before (void *oldmem, size_t bytes, void **victimp,
> +			const void *address)
>  {
> -  __memalign_hook = NULL;
> -  ptmalloc_init ();
> -  return __libc_memalign (alignment, sz);
> +  void *(*hook) (void *, size_t, const void *) =
> +    atomic_forced_read (__realloc_hook);
> +  if (__builtin_expect (hook != NULL, 0))
> +    {
> +      *victimp = (*hook)(oldmem, bytes, address);
> +      return true;
> +    }
> +
> +  return false;
>  }
>  
> -#include "malloc-check.c"
> +static __always_inline bool
> +_memalign_debug_before (size_t alignment, size_t bytes, void **victimp,
> +			 const void *address)
> +{
> +  void *(*hook) (size_t, size_t, const void *) =
> +    atomic_forced_read (__memalign_hook);
> +  if (__builtin_expect (hook != NULL, 0))
> +    {
> +      *victimp = (*hook)(alignment, bytes, address);
> +      return true;
> +    }
> +  return false;
> +}
> +
> +static __always_inline bool
> +_calloc_debug_before (size_t bytes, void **victimp, const void *address)
> +{
> +  void *(*hook) (size_t, const void *) =
> +    atomic_forced_read (__malloc_hook);
> +  if (__builtin_expect (hook != NULL, 0))
> +    {
> +      *victimp = (*hook)(bytes, address);
> +      if (*victimp != NULL)
> +	memset (*victimp, 0, bytes);
> +      return true;
> +    }
> +  return false;
> +}
>  
>  #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_25)
>  
> diff --git a/malloc/malloc-internal.h b/malloc/malloc-internal.h
> index 258f29584e..ee0f5697af 100644
> --- a/malloc/malloc-internal.h
> +++ b/malloc/malloc-internal.h
> @@ -61,6 +61,7 @@
>  /* The corresponding bit mask value.  */
>  #define MALLOC_ALIGN_MASK (MALLOC_ALIGNMENT - 1)
>  
> +#define top(ar_ptr) ((ar_ptr)->top)
>  
>  /* Called in the parent process before a fork.  */
>  void __malloc_fork_lock_parent (void) attribute_hidden;
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 0e2e1747e0..75ca6ec3f0 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2013,30 +2013,6 @@ static void     malloc_consolidate (mstate);
>  # define weak_variable weak_function
>  #endif
>  
> -/* Forward declarations.  */
> -static void *malloc_hook_ini (size_t sz,
> -                              const void *caller) __THROW;
> -static void *realloc_hook_ini (void *ptr, size_t sz,
> -                               const void *caller) __THROW;
> -static void *memalign_hook_ini (size_t alignment, size_t sz,
> -                                const void *caller) __THROW;
> -
> -#if HAVE_MALLOC_INIT_HOOK
> -void (*__malloc_initialize_hook) (void) __attribute__ ((nocommon));
> -compat_symbol (libc, __malloc_initialize_hook,
> -	       __malloc_initialize_hook, GLIBC_2_0);
> -#endif
> -
> -void weak_variable (*__free_hook) (void *__ptr,
> -                                   const void *) = NULL;
> -void *weak_variable (*__malloc_hook)
> -  (size_t __size, const void *) = malloc_hook_ini;
> -void *weak_variable (*__realloc_hook)
> -  (void *__ptr, size_t __size, const void *)
> -  = realloc_hook_ini;
> -void *weak_variable (*__memalign_hook)
> -  (size_t __alignment, size_t __size, const void *)
> -  = memalign_hook_ini;
>  void weak_variable (*__after_morecore_hook) (void) = NULL;
>  
>  /* This function is called from the arena shutdown hook, to free the
> @@ -2065,6 +2041,9 @@ free_perturb (char *p, size_t n)
>  
>  #include <stap-probe.h>
>  
> +/* ----------------- Support for debugging hooks -------------------- */
> +#include "hooks.c"
> +
>  /* ------------------- Support for multiple arenas -------------------- */
>  #include "arena.c"
>  
> @@ -2425,10 +2404,6 @@ do_check_malloc_state (mstate av)
>  #endif
>  
>  
> -/* ----------------- Support for debugging hooks -------------------- */
> -#include "hooks.c"
> -
> -
>  /* ----------- Routines dealing with system allocation -------------- */
>  
>  /*
> @@ -3225,13 +3200,12 @@ __libc_malloc (size_t bytes)
>    mstate ar_ptr;
>    void *victim;
>  
> -  _Static_assert (PTRDIFF_MAX <= SIZE_MAX / 2,
> -                  "PTRDIFF_MAX is not more than half of SIZE_MAX");
> +  if (__malloc_initialized < 0)
> +    ptmalloc_init ();
> +
> +  if (_malloc_debug_before (bytes, &victim, RETURN_ADDRESS (0)))
> +    return victim;
>  
> -  void *(*hook) (size_t, const void *)
> -    = atomic_forced_read (__malloc_hook);
> -  if (__builtin_expect (hook != NULL, 0))
> -    return (*hook)(bytes, RETURN_ADDRESS (0));
>  #if USE_TCACHE
>    /* int_free also calls request2size, be careful to not pad twice.  */
>    size_t tbytes;
> @@ -3292,13 +3266,11 @@ __libc_free (void *mem)
>    mstate ar_ptr;
>    mchunkptr p;                          /* chunk corresponding to mem */
>  
> -  void (*hook) (void *, const void *)
> -    = atomic_forced_read (__free_hook);
> -  if (__builtin_expect (hook != NULL, 0))
> -    {
> -      (*hook)(mem, RETURN_ADDRESS (0));
> -      return;
> -    }
> +  if (__malloc_initialized < 0)
> +    ptmalloc_init ();
> +
> +  if (_free_debug_before (mem, RETURN_ADDRESS (0)))
> +    return;
>  
>    if (mem == 0)                              /* free(0) has no effect */
>      return;
> @@ -3351,10 +3323,11 @@ __libc_realloc (void *oldmem, size_t bytes)
>  
>    void *newp;             /* chunk to return */
>  
> -  void *(*hook) (void *, size_t, const void *) =
> -    atomic_forced_read (__realloc_hook);
> -  if (__builtin_expect (hook != NULL, 0))
> -    return (*hook)(oldmem, bytes, RETURN_ADDRESS (0));
> +  if (__malloc_initialized < 0)
> +    ptmalloc_init ();
> +
> +  if (_realloc_debug_before (oldmem, bytes, &newp, RETURN_ADDRESS (0)))
> +    return newp;
>  
>  #if REALLOC_ZERO_BYTES_FREES
>    if (bytes == 0 && oldmem != NULL)
> @@ -3490,6 +3463,9 @@ void *
>  __libc_memalign (size_t alignment, size_t bytes)
>  {
>    void *address = RETURN_ADDRESS (0);
> +  if (__malloc_initialized < 0)
> +    ptmalloc_init ();
> +
>    return _mid_memalign (alignment, bytes, address);
>  }
>  
> @@ -3499,10 +3475,8 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
>    mstate ar_ptr;
>    void *p;
>  
> -  void *(*hook) (size_t, size_t, const void *) =
> -    atomic_forced_read (__memalign_hook);
> -  if (__builtin_expect (hook != NULL, 0))
> -    return (*hook)(alignment, bytes, address);
> +  if (_memalign_debug_before (alignment, bytes, &p, address))
> +    return p;
>  
>    /* If we need less alignment than we give anyway, just relay to malloc.  */
>    if (alignment <= MALLOC_ALIGNMENT)
> @@ -3612,16 +3586,11 @@ __libc_calloc (size_t n, size_t elem_size)
>  
>    sz = bytes;
>  
> -  void *(*hook) (size_t, const void *) =
> -    atomic_forced_read (__malloc_hook);
> -  if (__builtin_expect (hook != NULL, 0))
> -    {
> -      mem = (*hook)(sz, RETURN_ADDRESS (0));
> -      if (mem == 0)
> -        return 0;
> +  if (__malloc_initialized < 0)
> +    ptmalloc_init ();
>  
> -      return memset (mem, 0, sz);
> -    }
> +  if (_calloc_debug_before (sz, &mem, RETURN_ADDRESS (0)))
> +    return mem;
>  
>    MAYBE_INIT_TCACHE ();
>  
>