diff mbox series

[v4,05/10] glibc.malloc.check: Wean away from malloc hooks

Message ID 20210702113845.3367306-6-siddhesh@sourceware.org
State Superseded
Headers show
Series Remove malloc hooks | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Siddhesh Poyarekar July 2, 2021, 11:38 a.m. UTC
Set up a new internal debugging hooks flag variable and migrate
glibc.malloc.check to it so that it no longer uses the malloc hooks.

Reviewed-by: DJ Delorie <dj@redhat.com>
---
 malloc/arena.c        |  4 +--
 malloc/hooks.c        | 63 ++++++++++++++++++++++++++++++++++++++++++-
 malloc/malloc-check.c | 30 ++++++---------------
 malloc/malloc.c       |  9 +------
 4 files changed, 73 insertions(+), 33 deletions(-)

Comments

Carlos O'Donell July 2, 2021, 7:06 p.m. UTC | #1
On 7/2/21 7:38 AM, Siddhesh Poyarekar wrote:
> Set up a new internal debugging hooks flag variable and migrate
> glibc.malloc.check to it so that it no longer uses the malloc hooks.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
 
> Reviewed-by: DJ Delorie <dj@redhat.com>
> ---
>  malloc/arena.c        |  4 +--
>  malloc/hooks.c        | 63 ++++++++++++++++++++++++++++++++++++++++++-
>  malloc/malloc-check.c | 30 ++++++---------------
>  malloc/malloc.c       |  9 +------
>  4 files changed, 73 insertions(+), 33 deletions(-)
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 1861d20006..357a3b0b30 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -210,7 +210,7 @@ TUNABLE_CALLBACK (set_mallopt_check) (tunable_val_t *valp)
>  {
>    int32_t value = (int32_t) valp->numval;
>    if (value != 0)
> -    __malloc_check_init ();
> +    __malloc_debug_enable (MALLOC_CHECK_HOOK);
>  }
>  
>  # define TUNABLE_CALLBACK_FNDECL(__name, __type) \
> @@ -400,7 +400,7 @@ ptmalloc_init (void)
>          }
>      }
>    if (s && s[0] != '\0' && s[0] != '0')
> -    __malloc_check_init ();
> +    __malloc_debug_enable (MALLOC_CHECK_HOOK);
>  #endif
>  
>  #if HAVE_MALLOC_INIT_HOOK
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 4960aafd08..77855801c8 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -21,6 +21,8 @@
>     corrupt pointer is detected: do nothing (0), print an error message
>     (1), or call abort() (2). */
>  
> +#  include <stdbool.h>
> +
>  /* 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
> @@ -29,6 +31,14 @@
>  # define weak_variable weak_function
>  #endif
>  
> +/* The internal malloc debugging hooks.  */
> +enum malloc_debug_hooks
> +{
> +  MALLOC_NONE_HOOK = 0,
> +  MALLOC_CHECK_HOOK = 1 << 0,	/* MALLOC_CHECK_ or glibc.malloc.check.  */
> +};
> +static unsigned __malloc_debugging_hooks;
> +
>  /* Forward declarations.  */
>  
>  #if HAVE_MALLOC_INIT_HOOK
> @@ -48,6 +58,24 @@ void *weak_variable (*__memalign_hook)
>  
>  static void ptmalloc_init (void);
>  
> +static __always_inline bool
> +__is_malloc_debug_enabled (enum malloc_debug_hooks flag)
> +{
> +  return __malloc_debugging_hooks & flag;
> +}
> +
> +static __always_inline void
> +__malloc_debug_enable (enum malloc_debug_hooks flag)
> +{
> +  __malloc_debugging_hooks |= flag;
> +}
> +
> +static __always_inline void
> +__malloc_debug_disable (enum malloc_debug_hooks flag)
> +{
> +  __malloc_debugging_hooks &= ~flag;
> +}
> +
>  #include "malloc-check.c"
>  
>  static __always_inline bool
> @@ -63,6 +91,12 @@ _malloc_debug_before (size_t bytes, void **victimp, const void *address)
>        *victimp = (*hook)(bytes, address);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = malloc_check (bytes);
> +      return true;
> +    }
>    return false;
>  }
>  
> @@ -76,6 +110,12 @@ _free_debug_before (void *mem, const void *address)
>        (*hook)(mem, address);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      free_check (mem);
> +      return true;
> +    }
>    return false;
>  }
>  
> @@ -91,6 +131,12 @@ _realloc_debug_before (void *oldmem, size_t bytes, void **victimp,
>        return true;
>      }
>  
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = realloc_check (oldmem, bytes);
> +      return true;
> +    }
> +
>    return false;
>  }
>  
> @@ -105,6 +151,13 @@ _memalign_debug_before (size_t alignment, size_t bytes, void **victimp,
>        *victimp = (*hook)(alignment, bytes, address);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = memalign_check (alignment, bytes);
> +      return true;
> +    }
> +
>    return false;
>  }
>  
> @@ -120,6 +173,14 @@ _calloc_debug_before (size_t bytes, void **victimp, const void *address)
>  	memset (*victimp, 0, bytes);
>        return true;
>      }
> +
> +  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
> +    {
> +      *victimp = malloc_check (bytes);
> +      if (*victimp != NULL)
> +	memset (*victimp, 0, bytes);
> +      return true;
> +    }
>    return false;
>  }
>  
> @@ -195,7 +256,7 @@ malloc_set_state (void *msptr)
>    __realloc_hook = NULL;
>    __free_hook = NULL;
>    __memalign_hook = NULL;
> -  using_malloc_checking = 0;
> +  __malloc_debug_disable (MALLOC_CHECK_HOOK);
>  
>    /* Patch the dumped heap.  We no longer try to integrate into the
>       existing heap.  Instead, we mark the existing chunks as mmapped.
> diff --git a/malloc/malloc-check.c b/malloc/malloc-check.c
> index dcab880510..a85e519498 100644
> --- a/malloc/malloc-check.c
> +++ b/malloc/malloc-check.c
> @@ -18,20 +18,6 @@
>     not, see <https://www.gnu.org/licenses/>.  */
>  
>  
> -/* Whether we are using malloc checking.  */
> -static int using_malloc_checking;
> -
> -/* Activate a standard set of debugging hooks. */
> -void
> -__malloc_check_init (void)
> -{
> -  using_malloc_checking = 1;
> -  __malloc_hook = malloc_check;
> -  __free_hook = free_check;
> -  __realloc_hook = realloc_check;
> -  __memalign_hook = memalign_check;
> -}
> -
>  /* When memory is tagged, the checking data is stored in the user part
>     of the chunk.  We can't rely on the user not having modified the
>     tags, so fetch the tag at each location before dereferencing
> @@ -69,7 +55,7 @@ malloc_check_get_size (mchunkptr p)
>    unsigned char c;
>    unsigned char magic = magicbyte (p);
>  
> -  assert (using_malloc_checking == 1);
> +  assert (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK));
>  
>    for (size = CHUNK_HDR_SZ + memsize (p) - 1;
>         (c = *SAFE_CHAR_OFFSET (p, size)) != magic;
> @@ -203,7 +189,7 @@ top_check (void)
>  }
>  
>  static void *
> -malloc_check (size_t sz, const void *caller)
> +malloc_check (size_t sz)
>  {
>    void *victim;
>    size_t nb;
> @@ -222,7 +208,7 @@ malloc_check (size_t sz, const void *caller)
>  }
>  
>  static void
> -free_check (void *mem, const void *caller)
> +free_check (void *mem)
>  {
>    mchunkptr p;
>  
> @@ -256,7 +242,7 @@ free_check (void *mem, const void *caller)
>  }
>  
>  static void *
> -realloc_check (void *oldmem, size_t bytes, const void *caller)
> +realloc_check (void *oldmem, size_t bytes)
>  {
>    INTERNAL_SIZE_T chnb;
>    void *newmem = 0;
> @@ -269,11 +255,11 @@ realloc_check (void *oldmem, size_t bytes, const void *caller)
>        return NULL;
>      }
>    if (oldmem == 0)
> -    return malloc_check (bytes, NULL);
> +    return malloc_check (bytes);
>  
>    if (bytes == 0)
>      {
> -      free_check (oldmem, NULL);
> +      free_check (oldmem);
>        return NULL;
>      }
>  
> @@ -348,12 +334,12 @@ invert:
>  }
>  
>  static void *
> -memalign_check (size_t alignment, size_t bytes, const void *caller)
> +memalign_check (size_t alignment, size_t bytes)
>  {
>    void *mem;
>  
>    if (alignment <= MALLOC_ALIGNMENT)
> -    return malloc_check (bytes, NULL);
> +    return malloc_check (bytes);
>  
>    if (alignment < MINSIZE)
>      alignment = MINSIZE;
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 75ca6ec3f0..60753446a1 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1124,13 +1124,6 @@ static void munmap_chunk(mchunkptr p);
>  static mchunkptr mremap_chunk(mchunkptr p, size_t new_size);
>  #endif
>  
> -static void*   malloc_check(size_t sz, const void *caller);
> -static void      free_check(void* mem, const void *caller);
> -static void*   realloc_check(void* oldmem, size_t bytes,
> -			       const void *caller);
> -static void*   memalign_check(size_t alignment, size_t bytes,
> -				const void *caller);
> -
>  /* ------------------ MMAP support ------------------  */
>  
>  
> @@ -5078,7 +5071,7 @@ musable (void *mem)
>  
>        p = mem2chunk (mem);
>  
> -      if (__builtin_expect (using_malloc_checking == 1, 0))
> +      if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
>  	return malloc_check_get_size (p);
>  
>        if (chunk_is_mmapped (p))
>
diff mbox series

Patch

diff --git a/malloc/arena.c b/malloc/arena.c
index 1861d20006..357a3b0b30 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -210,7 +210,7 @@  TUNABLE_CALLBACK (set_mallopt_check) (tunable_val_t *valp)
 {
   int32_t value = (int32_t) valp->numval;
   if (value != 0)
-    __malloc_check_init ();
+    __malloc_debug_enable (MALLOC_CHECK_HOOK);
 }
 
 # define TUNABLE_CALLBACK_FNDECL(__name, __type) \
@@ -400,7 +400,7 @@  ptmalloc_init (void)
         }
     }
   if (s && s[0] != '\0' && s[0] != '0')
-    __malloc_check_init ();
+    __malloc_debug_enable (MALLOC_CHECK_HOOK);
 #endif
 
 #if HAVE_MALLOC_INIT_HOOK
diff --git a/malloc/hooks.c b/malloc/hooks.c
index 4960aafd08..77855801c8 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -21,6 +21,8 @@ 
    corrupt pointer is detected: do nothing (0), print an error message
    (1), or call abort() (2). */
 
+#  include <stdbool.h>
+
 /* 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
@@ -29,6 +31,14 @@ 
 # define weak_variable weak_function
 #endif
 
+/* The internal malloc debugging hooks.  */
+enum malloc_debug_hooks
+{
+  MALLOC_NONE_HOOK = 0,
+  MALLOC_CHECK_HOOK = 1 << 0,	/* MALLOC_CHECK_ or glibc.malloc.check.  */
+};
+static unsigned __malloc_debugging_hooks;
+
 /* Forward declarations.  */
 
 #if HAVE_MALLOC_INIT_HOOK
@@ -48,6 +58,24 @@  void *weak_variable (*__memalign_hook)
 
 static void ptmalloc_init (void);
 
+static __always_inline bool
+__is_malloc_debug_enabled (enum malloc_debug_hooks flag)
+{
+  return __malloc_debugging_hooks & flag;
+}
+
+static __always_inline void
+__malloc_debug_enable (enum malloc_debug_hooks flag)
+{
+  __malloc_debugging_hooks |= flag;
+}
+
+static __always_inline void
+__malloc_debug_disable (enum malloc_debug_hooks flag)
+{
+  __malloc_debugging_hooks &= ~flag;
+}
+
 #include "malloc-check.c"
 
 static __always_inline bool
@@ -63,6 +91,12 @@  _malloc_debug_before (size_t bytes, void **victimp, const void *address)
       *victimp = (*hook)(bytes, address);
       return true;
     }
+
+  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
+    {
+      *victimp = malloc_check (bytes);
+      return true;
+    }
   return false;
 }
 
@@ -76,6 +110,12 @@  _free_debug_before (void *mem, const void *address)
       (*hook)(mem, address);
       return true;
     }
+
+  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
+    {
+      free_check (mem);
+      return true;
+    }
   return false;
 }
 
@@ -91,6 +131,12 @@  _realloc_debug_before (void *oldmem, size_t bytes, void **victimp,
       return true;
     }
 
+  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
+    {
+      *victimp = realloc_check (oldmem, bytes);
+      return true;
+    }
+
   return false;
 }
 
@@ -105,6 +151,13 @@  _memalign_debug_before (size_t alignment, size_t bytes, void **victimp,
       *victimp = (*hook)(alignment, bytes, address);
       return true;
     }
+
+  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
+    {
+      *victimp = memalign_check (alignment, bytes);
+      return true;
+    }
+
   return false;
 }
 
@@ -120,6 +173,14 @@  _calloc_debug_before (size_t bytes, void **victimp, const void *address)
 	memset (*victimp, 0, bytes);
       return true;
     }
+
+  if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
+    {
+      *victimp = malloc_check (bytes);
+      if (*victimp != NULL)
+	memset (*victimp, 0, bytes);
+      return true;
+    }
   return false;
 }
 
@@ -195,7 +256,7 @@  malloc_set_state (void *msptr)
   __realloc_hook = NULL;
   __free_hook = NULL;
   __memalign_hook = NULL;
-  using_malloc_checking = 0;
+  __malloc_debug_disable (MALLOC_CHECK_HOOK);
 
   /* Patch the dumped heap.  We no longer try to integrate into the
      existing heap.  Instead, we mark the existing chunks as mmapped.
diff --git a/malloc/malloc-check.c b/malloc/malloc-check.c
index dcab880510..a85e519498 100644
--- a/malloc/malloc-check.c
+++ b/malloc/malloc-check.c
@@ -18,20 +18,6 @@ 
    not, see <https://www.gnu.org/licenses/>.  */
 
 
-/* Whether we are using malloc checking.  */
-static int using_malloc_checking;
-
-/* Activate a standard set of debugging hooks. */
-void
-__malloc_check_init (void)
-{
-  using_malloc_checking = 1;
-  __malloc_hook = malloc_check;
-  __free_hook = free_check;
-  __realloc_hook = realloc_check;
-  __memalign_hook = memalign_check;
-}
-
 /* When memory is tagged, the checking data is stored in the user part
    of the chunk.  We can't rely on the user not having modified the
    tags, so fetch the tag at each location before dereferencing
@@ -69,7 +55,7 @@  malloc_check_get_size (mchunkptr p)
   unsigned char c;
   unsigned char magic = magicbyte (p);
 
-  assert (using_malloc_checking == 1);
+  assert (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK));
 
   for (size = CHUNK_HDR_SZ + memsize (p) - 1;
        (c = *SAFE_CHAR_OFFSET (p, size)) != magic;
@@ -203,7 +189,7 @@  top_check (void)
 }
 
 static void *
-malloc_check (size_t sz, const void *caller)
+malloc_check (size_t sz)
 {
   void *victim;
   size_t nb;
@@ -222,7 +208,7 @@  malloc_check (size_t sz, const void *caller)
 }
 
 static void
-free_check (void *mem, const void *caller)
+free_check (void *mem)
 {
   mchunkptr p;
 
@@ -256,7 +242,7 @@  free_check (void *mem, const void *caller)
 }
 
 static void *
-realloc_check (void *oldmem, size_t bytes, const void *caller)
+realloc_check (void *oldmem, size_t bytes)
 {
   INTERNAL_SIZE_T chnb;
   void *newmem = 0;
@@ -269,11 +255,11 @@  realloc_check (void *oldmem, size_t bytes, const void *caller)
       return NULL;
     }
   if (oldmem == 0)
-    return malloc_check (bytes, NULL);
+    return malloc_check (bytes);
 
   if (bytes == 0)
     {
-      free_check (oldmem, NULL);
+      free_check (oldmem);
       return NULL;
     }
 
@@ -348,12 +334,12 @@  invert:
 }
 
 static void *
-memalign_check (size_t alignment, size_t bytes, const void *caller)
+memalign_check (size_t alignment, size_t bytes)
 {
   void *mem;
 
   if (alignment <= MALLOC_ALIGNMENT)
-    return malloc_check (bytes, NULL);
+    return malloc_check (bytes);
 
   if (alignment < MINSIZE)
     alignment = MINSIZE;
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 75ca6ec3f0..60753446a1 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1124,13 +1124,6 @@  static void munmap_chunk(mchunkptr p);
 static mchunkptr mremap_chunk(mchunkptr p, size_t new_size);
 #endif
 
-static void*   malloc_check(size_t sz, const void *caller);
-static void      free_check(void* mem, const void *caller);
-static void*   realloc_check(void* oldmem, size_t bytes,
-			       const void *caller);
-static void*   memalign_check(size_t alignment, size_t bytes,
-				const void *caller);
-
 /* ------------------ MMAP support ------------------  */
 
 
@@ -5078,7 +5071,7 @@  musable (void *mem)
 
       p = mem2chunk (mem);
 
-      if (__builtin_expect (using_malloc_checking == 1, 0))
+      if (__glibc_unlikely (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)))
 	return malloc_check_get_size (p);
 
       if (chunk_is_mmapped (p))