Re: [PATCH 4/8] mcheck: Wean away from malloc hooks

[DJ Delorie <dj@redhat.com>]

Siddhesh Poyarekar <siddhesh@sourceware.org> writes:
> Initialization using -lmcheck now depends on a new exported variable
> __libc_lmcheck which is a const flag that mcheck-init can override.

I wonder if we should bite the bullet and add an official malloc_ioctl()
or something.  Oh wait, we have mallopt() already.  Can we hijack this
instead of adding an ABI?

  mallopt (MALLOC_INTERNAL_GET_VER, 0) returns malloc_version + 1000
  (or returns 0 or 1, for "not supported")

  mallopt (MALLOC_INTERNAL_SET_DEBUG, debug_flags)

This could be done in a .ctor/.init; I wonder if that would be early
enough.

(and yes, this is why I initially did all this in a separate library
that overrode the libc symbols - linking means you get the new functions
right away)

> diff --git a/include/malloc.h b/include/malloc.h
> index b77761f74d..bb1123d9d3 100644
> --- a/include/malloc.h
> +++ b/include/malloc.h
> @@ -4,6 +4,11 @@
>  
>  # ifndef _ISOMAC
>  #  include <rtld-malloc.h>
> +#  include <stdbool.h>
> +#  include <mcheck.h>
> +
> +struct malloc_state;
> +typedef struct malloc_state *mstate;

Moved from below, ok.

>  /* In the GNU libc we rename the global variable
>     `__malloc_initialized' to `__libc_malloc_initialized'.  */
> @@ -11,8 +16,9 @@
>  /* Nonzero if the malloc is already initialized.  */
>  extern int __malloc_initialized attribute_hidden;
>  
> -struct malloc_state;
> -typedef struct malloc_state *mstate;
> +enum mcheck_status __mcheck_checkptr (const void *) attribute_hidden;
> +extern int __mcheck_initialize (void (*) (enum mcheck_status), bool)
> +     attribute_hidden;

Ok.

> diff --git a/malloc/Makefile b/malloc/Makefile
> index fd7f399250..f9433af880 100644
> --- a/malloc/Makefile
> +++ b/malloc/Makefile
> @@ -90,9 +90,7 @@ tests-exclude-mcheck = tst-mallocstate \
>  	tst-malloc-usable-static \
>  	tst-malloc-usable-static-tunables \
>  	tst-malloc-usable-tunables \
> -	tst-malloc_info \
> -	tst-memalign \
> -	tst-posix_memalign
> +	tst-malloc_info

Ok.

> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 77855801c8..b517a98ea2 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -36,6 +36,7 @@ enum malloc_debug_hooks
>  {
>    MALLOC_NONE_HOOK = 0,
>    MALLOC_CHECK_HOOK = 1 << 0,	/* MALLOC_CHECK_ or glibc.malloc.check.  */
> +  MALLOC_MCHECK_HOOK = 1 << 1,	/* mcheck()  */
>  };
>  static unsigned __malloc_debugging_hooks;

Ok.

> @@ -77,113 +78,180 @@ __malloc_debug_disable (enum malloc_debug_hooks flag)
>  }
>  
>  #include "malloc-check.c"
> +#include "mcheck-hooks.c"
>  
>  static __always_inline bool
> -_malloc_debug_before (size_t bytes, void **victimp, const void *address)
> +_malloc_debug_before (size_t *bytesp, void **victimp, const void *address)
>  {
>    _Static_assert (PTRDIFF_MAX <= SIZE_MAX / 2,
> -                  "PTRDIFF_MAX is not more than half of SIZE_MAX");
> +		  "PTRDIFF_MAX is not more than half of SIZE_MAX");

Ok.

> -      *victimp = (*hook)(bytes, address);
> +      *victimp = (*hook)(*bytesp, address);

Ok.

>      }
>  
> -  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +  if (__glibc_unlikely (__malloc_debugging_hooks))

Ok.  Updated for "any flag set"

> -      *victimp = malloc_check (bytes);
> -      return true;
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
> +	  && malloc_mcheck_before (bytesp, victimp))
> +	return true;
> +      if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
> +	{
> +	  *victimp = malloc_check (*bytesp);
> +	  return true;
> +	}
>      }
>    return false;
>  }

Ok.

> +static __always_inline void *
> +_malloc_debug_after (void *mem, size_t bytes, const void *address)
> +{
> +  if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> +    mem = malloc_mcheck_after (mem, bytes);
> +  return mem;
> +}

ok.

>  static __always_inline bool
> -_free_debug_before (void *mem, const void *address)
> +_free_debug_before (void **mem, const void *address)

Ok.

> -      (*hook)(mem, address);
> +      (*hook)(*mem, address);

Ok.

> -  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
>      {
> -      free_check (mem);
> -      return true;
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
> +	*mem = free_mcheck (*mem);
> +      if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
> +	{
> +	  free_check (*mem);
> +	  return true;
> +	}
>      }
>    return false;
>  }

Ok.

>  static __always_inline bool
> -_realloc_debug_before (void *oldmem, size_t bytes, void **victimp,
> -			const void *address)
> +_realloc_debug_before (void **oldmem, size_t *bytesp, size_t *oldsize,
> +		       void **victimp, const void *address)

Ok.

> -      *victimp = (*hook)(oldmem, bytes, address);
> +      *victimp = (*hook)(*oldmem, *bytesp, address);

Ok.

> -  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
>      {
> -      *victimp = realloc_check (oldmem, bytes);
> -      return true;
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
> +	  && realloc_mcheck_before (oldmem, bytesp, oldsize, victimp))
> +	return true;
> +      if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
> +	{
> +	  *victimp = realloc_check (*oldmem, *bytesp);
> +	  return true;
> +	}
>      }

Ok.

> +static __always_inline void *
> +_realloc_debug_after (void *mem, void *oldmem, size_t bytes, size_t oldsize,
> +		      const void *address)
> +{
> +  if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> +    mem = realloc_mcheck_after (mem, oldmem, bytes, oldsize);
> +  return mem;
> +}

Ok.

>  static __always_inline bool
> -_memalign_debug_before (size_t alignment, size_t bytes, void **victimp,
> -			 const void *address)
> +_memalign_debug_before (size_t alignment, size_t *bytesp, void **victimp,
> +			const void *address)

Ok.

> -      *victimp = (*hook)(alignment, bytes, address);
> +      *victimp = (*hook)(alignment, *bytesp, address);

Ok.

> -
> -  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
>      {
> -      *victimp = memalign_check (alignment, bytes);
> -      return true;
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
> +	  && memalign_mcheck_before (alignment, bytesp, victimp))
> +	return true;
> +      if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
> +	{
> +	  *victimp = memalign_check (alignment, *bytesp);
> +	  return true;
> +	}
>      }

Ok.

> +static __always_inline void *
> +_memalign_debug_after (void *mem, size_t alignment, size_t bytes,
> +		       const void *address)
> +{
> +  if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK) && mem != NULL)
> +    mem = memalign_mcheck_after (mem, alignment, bytes);
> +  return mem;
> +}

Ok.

>  static __always_inline bool
> -_calloc_debug_before (size_t bytes, void **victimp, const void *address)
> +_calloc_debug_before (size_t *bytesp, void **victimp, const void *address)

Ok

>    void *(*hook) (size_t, const void *) =
>      atomic_forced_read (__malloc_hook);
>    if (__builtin_expect (hook != NULL, 0))
>      {
> -      *victimp = (*hook)(bytes, address);
> +      *victimp = (*hook)(*bytesp, address);
> +
>        if (*victimp != NULL)
> -	memset (*victimp, 0, bytes);
> +	memset (*victimp, 0, *bytesp);
> +
>        return true;
>      }

Ok.

> -  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +  if (__glibc_unlikely (__malloc_debugging_hooks))
>      {
> -      *victimp = malloc_check (bytes);
> -      if (*victimp != NULL)
> -	memset (*victimp, 0, bytes);
> -      return true;
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
> +	  && malloc_mcheck_before (bytesp, victimp))
> +	return true;
> +      if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
> +	{
> +	  *victimp = malloc_check (*bytesp);
> +	  return true;

memset missing?  no, moved to below...  A comment would help here.

> +static __always_inline void *
> +_calloc_debug_after (void *mem, size_t bytes, const void *address)
> +{
> +  if (__glibc_unlikely (__malloc_debugging_hooks) && mem != NULL)
> +    {
> +      if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
> +	mem = malloc_mcheck_after (mem, bytes);
> +      memset (mem, 0, bytes);
> +    }
> +  return mem;
> +}

So we memset if *any* hook is called, ok.

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 60753446a1..5ea12d1d3b 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3192,12 +3192,13 @@ __libc_malloc (size_t bytes)
>  {
>    mstate ar_ptr;
>    void *victim;
> +  size_t orig_bytes = bytes;

Do we need to initialize victim to NULL here?

>    if (__malloc_initialized < 0)
>      ptmalloc_init ();
>  
> -  if (_malloc_debug_before (bytes, &victim, RETURN_ADDRESS (0)))
> -    return victim;
> +  if (_malloc_debug_before (&bytes, &victim, RETURN_ADDRESS (0)))
> +    goto out;

Ok.

> @@ -3205,7 +3206,8 @@ __libc_malloc (size_t bytes)
>    if (!checked_request2size (bytes, &tbytes))
>      {
>        __set_errno (ENOMEM);
> -      return NULL;
> +      victim = NULL;
> +      goto out;

Ok.

> @@ -3217,7 +3219,8 @@ __libc_malloc (size_t bytes)
>        && tcache->counts[tc_idx] > 0)
>      {
>        victim = tcache_get (tc_idx);
> -      return tag_new_usable (victim);
> +      victim = tag_new_usable (victim);
> +      goto out;

Ok.

> @@ -3227,7 +3230,7 @@ __libc_malloc (size_t bytes)
>        victim = tag_new_usable (_int_malloc (&main_arena, bytes));
>        assert (!victim || chunk_is_mmapped (mem2chunk (victim)) ||
>  	      &main_arena == arena_for_chunk (mem2chunk (victim)));
> -      return victim;
> +      goto out;

Ok.

> @@ -3249,7 +3252,9 @@ __libc_malloc (size_t bytes)
>  
>    assert (!victim || chunk_is_mmapped (mem2chunk (victim)) ||
>            ar_ptr == arena_for_chunk (mem2chunk (victim)));
> -  return victim;
> +
> +out:
> +  return _malloc_debug_after (victim, orig_bytes, RETURN_ADDRESS (0));
>  }

This handles both NULL and non-NULL cases, so ok.

> @@ -3262,7 +3267,7 @@ __libc_free (void *mem)
>    if (__malloc_initialized < 0)
>      ptmalloc_init ();
>  
> -  if (_free_debug_before (mem, RETURN_ADDRESS (0)))
> +  if (_free_debug_before (&mem, RETURN_ADDRESS (0)))
>      return;

Ok.

> @@ -3315,23 +3320,30 @@ __libc_realloc (void *oldmem, size_t bytes)
>    INTERNAL_SIZE_T nb;         /* padded request size */
>  
>    void *newp;             /* chunk to return */
> +  size_t orig_bytes = bytes, debug_osize = 0;

Ok.

> -  if (_realloc_debug_before (oldmem, bytes, &newp, RETURN_ADDRESS (0)))
> -    return newp;
> +  if (_realloc_debug_before (&oldmem, &bytes, &debug_osize, &newp,
> +			     RETURN_ADDRESS (0)))
> +    goto out;

Ok.

>  #if REALLOC_ZERO_BYTES_FREES
>    if (bytes == 0 && oldmem != NULL)
>      {
> -      __libc_free (oldmem); return 0;
> +      __libc_free (oldmem);
> +      newp = NULL;
> +      goto out;
>      }
>  #endif

Ok.

>    /* realloc of null is supposed to be same as malloc */
>    if (oldmem == 0)
> -    return __libc_malloc (bytes);
> +    {
> +      newp = __libc_malloc (bytes);
> +      goto out;
> +    }

Ok.

> @@ -3365,7 +3377,8 @@ __libc_realloc (void *oldmem, size_t bytes)
>    if (!checked_request2size (bytes, &nb))
>      {
>        __set_errno (ENOMEM);
> -      return NULL;
> +      newp = NULL;
> +      goto out;
>      }

Ok.

> @@ -3377,7 +3390,10 @@ __libc_realloc (void *oldmem, size_t bytes)
>  	  /* Must alloc, copy, free. */
>  	  void *newmem = __libc_malloc (bytes);
>  	  if (newmem == 0)
> -	    return NULL;
> +	    {
> +	      newp = NULL;
> +	      goto out;
> +	    }

Ok.

> @@ -3385,7 +3401,8 @@ __libc_realloc (void *oldmem, size_t bytes)
>  	  if (bytes > oldsize - SIZE_SZ)
>  	    bytes = oldsize - SIZE_SZ;
>  	  memcpy (newmem, oldmem, bytes);
> -	  return newmem;
> +	  newp = newmem;
> +	  goto out;

Ok.

> @@ -3400,21 +3417,29 @@ __libc_realloc (void *oldmem, size_t bytes)
>  	     reused.  There's a performance hit for both us and the
>  	     caller for doing this, so we might want to
>  	     reconsider.  */
> -	  return tag_new_usable (newmem);
> +	  newp = tag_new_usable (newmem);
> +	  goto out;

Ok.

>  #endif
>        /* Note the extra SIZE_SZ overhead. */
>        if (oldsize - SIZE_SZ >= nb)
> -        return oldmem;                         /* do nothing */
> +	{
> +	  newp = oldmem;                         /* do nothing */
> +	  goto out;
> +	}

Ok.

>        /* Must alloc, copy, free. */
>        newmem = __libc_malloc (bytes);
>        if (newmem == 0)
> -        return 0;              /* propagate failure */
> +	{
> +	  newp = NULL;              /* propagate failure */
> +	  goto out;
> +	}

Ok.

>        memcpy (newmem, oldmem, oldsize - CHUNK_HDR_SZ);
>        munmap_chunk (oldp);
> -      return newmem;
> +      newp = newmem;
> +      goto out;
>      }

Ok.

> @@ -3423,7 +3448,7 @@ __libc_realloc (void *oldmem, size_t bytes)
>        assert (!newp || chunk_is_mmapped (mem2chunk (newp)) ||
>  	      ar_ptr == arena_for_chunk (mem2chunk (newp)));
>  
> -      return newp;
> +      goto out;

Ok.

> @@ -3448,7 +3473,9 @@ __libc_realloc (void *oldmem, size_t bytes)
>          }
>      }
>  
> -  return newp;
> +out:
> +  return _realloc_debug_after (newp, oldmem, orig_bytes, debug_osize,
> +			       RETURN_ADDRESS (0));
>  }

Ok.

> @@ -3467,13 +3494,17 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
>  {
>    mstate ar_ptr;
>    void *p;
> +  size_t orig_bytes = bytes;
>  
> -  if (_memalign_debug_before (alignment, bytes, &p, address))
> -    return p;
> +  if (_memalign_debug_before (alignment, &bytes, &p, address))
> +    goto out;

Ok.

>    /* If we need less alignment than we give anyway, just relay to malloc.  */
>    if (alignment <= MALLOC_ALIGNMENT)
> -    return __libc_malloc (bytes);
> +    {
> +      p = __libc_malloc (bytes);
> +      goto out;
> +    }

Ok.

> @@ -3484,7 +3515,8 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
>    if (alignment > SIZE_MAX / 2 + 1)
>      {
>        __set_errno (EINVAL);
> -      return 0;
> +      p = NULL;
> +      goto out;
>      }

Ok.

> @@ -3502,7 +3534,8 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
>        p = _int_memalign (&main_arena, alignment, bytes);
>        assert (!p || chunk_is_mmapped (mem2chunk (p)) ||
>  	      &main_arena == arena_for_chunk (mem2chunk (p)));
> -      return tag_new_usable (p);
> +      p = tag_new_usable (p);
> +      goto out;
>      }

Ok.

> @@ -3520,7 +3553,9 @@ _mid_memalign (size_t alignment, size_t bytes, void *address)
>  
>    assert (!p || chunk_is_mmapped (mem2chunk (p)) ||
>            ar_ptr == arena_for_chunk (mem2chunk (p)));
> -  return tag_new_usable (p);
> +  p = tag_new_usable (p);
> +out:
> +  return _memalign_debug_after (p, alignment, orig_bytes, RETURN_ADDRESS (0));
>  }

Ok.


> @@ -3582,8 +3617,8 @@ __libc_calloc (size_t n, size_t elem_size)
>    if (__malloc_initialized < 0)
>      ptmalloc_init ();
>  
> -  if (_calloc_debug_before (sz, &mem, RETURN_ADDRESS (0)))
> -    return mem;
> +  if (_calloc_debug_before (&sz, &mem, RETURN_ADDRESS (0)))
> +    goto out;

Ok.

> @@ -3639,7 +3674,10 @@ __libc_calloc (size_t n, size_t elem_size)
>  
>    /* Allocation failed even after a retry.  */
>    if (mem == 0)
> -    return 0;
> +    {
> +      mem = NULL;
> +      goto out;
> +    }

Ok.

> @@ -3647,7 +3685,10 @@ __libc_calloc (size_t n, size_t elem_size)
>       regardless of MORECORE_CLEARS, so we zero the whole block while
>       doing so.  */
>    if (__glibc_unlikely (mtag_enabled))
> -    return tag_new_zero_region (mem, memsize (p));
> +    {
> +      mem = tag_new_zero_region (mem, memsize (p));
> +      goto out;
> +    }

Ok.

> @@ -3655,9 +3696,9 @@ __libc_calloc (size_t n, size_t elem_size)
>    if (chunk_is_mmapped (p))
>      {
>        if (__builtin_expect (perturb_byte, 0))
> -        return memset (mem, 0, sz);
> +        memset (mem, 0, sz);
>  
> -      return mem;
> +      goto out;
>      }

Ok.

> @@ -3677,7 +3718,10 @@ __libc_calloc (size_t n, size_t elem_size)
>    assert (nclears >= 3);
>  
>    if (nclears > 9)
> -    return memset (d, 0, clearsize);
> +    {
> +      memset (d, 0, clearsize);
> +      goto out;
> +    }

Ok; d is a copy of mem set before the patch text.

> @@ -3701,7 +3745,8 @@ __libc_calloc (size_t n, size_t elem_size)
>          }
>      }
>  
> -  return mem;
> +out:
> +  return _calloc_debug_after (mem, bytes, RETURN_ADDRESS (0));
>  }

Ok.

> diff --git a/malloc/mcheck-hooks.c b/malloc/mcheck-hooks.c
> new file mode 100644
> index 0000000000..f3520c7659
> --- /dev/null
> +++ b/malloc/mcheck-hooks.c
> @@ -0,0 +1,411 @@

Ok.

> diff --git a/malloc/mcheck.c b/malloc/mcheck.c
> index 2a1fc645d4..f60766eded 100644
> --- a/malloc/mcheck.c
> +++ b/malloc/mcheck.c
> @@ -1,4 +1,4 @@
> -/* Standard debugging hooks for `malloc'.
> +/* The mcheck() interface.
>     Copyright (C) 1990-2021 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>     Written May 1989 by Mike Haertel.
> @@ -23,378 +23,22 @@
>  # include <mcheck.h>
>  # include <stdint.h>
>  # include <stdio.h>
> -# include <libintl.h>
>  # include <errno.h>
>  #endif
>  
> -/* Old hook values.  */
> -static void (*old_free_hook)(void *ptr, const void *);
> -static void *(*old_malloc_hook) (size_t size, const void *);
> -static void *(*old_memalign_hook) (size_t alignment, size_t size,
> -				   const void *);
>  . . .
> -    }
> -
> -  return mcheck_used ? 0 : -1;
> +  return __mcheck_initialize (func, false);
>  }

Ok.

> @@ -403,14 +47,11 @@ libc_hidden_def (mcheck)
>  int
>  mcheck_pedantic (void (*func) (enum mcheck_status))
>  {
> -  int res = mcheck (func);
> -  if (res == 0)
> -    pedantic = 1;
> -  return res;
> +  return __mcheck_initialize (func, true);
>  }

Ok.

>  enum mcheck_status
>  mprobe (void *ptr)
>  {
> -  return mcheck_used ? checkhdr (((struct hdr *) ptr) - 1) : MCHECK_DISABLED;
> +  return __mcheck_checkptr (ptr);
>  }

Ok.