[v6,5/8] glibc.malloc.check: Wean away from malloc hooks

Message ID 20210706180924.95047-6-siddhesh@sourceware.org
State Superseded
Headers
Series malloc hooks removal |

Checks

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

Commit Message

Siddhesh Poyarekar July 6, 2021, 6:09 p.m. UTC
  The malloc-check debugging feature is tightly integrated into glibc
malloc because of which the implementation needs to stay in libc.so
for now.  The flags to control its execution however has been moved to
libc_malloc_debug.so so that it is called only when the library is
preloaded.  To achieve this, the checking functions have been exported
into the GLIBC_PRIVATE namespace to allow libc_malloc_debug.so to call
it.

Further, a special totem variable __malloc_debug_totem@GLIBC_PRIVATE
is set in libc malloc initialization, which is a hint to
libc_malloc_debug.so that it is running with a compatible DSO.  This
is necessary to skip malloc check tests with older glibc, which
inadvertently happens as the LD_PRELOAD environment variable sometimes
needs through pass through multiple programs, e.g. with testrun.sh.

A long term solution here would be to tweeze malloc-check away from
the core malloc so that it can be embedded into libc_malloc_debug.so.
---
 malloc/Versions       |  9 +++++
 malloc/arena.c        | 11 ------
 malloc/hooks.c        | 10 ++++-
 malloc/malloc-check.c | 86 ++++++++++++++++++++++++-------------------
 malloc/malloc-check.h | 32 ++++++++++++++++
 malloc/malloc-debug.c | 84 +++++++++++++++++++++++++++++++++++-------
 malloc/malloc.c       | 10 -----
 7 files changed, 168 insertions(+), 74 deletions(-)
 create mode 100644 malloc/malloc-check.h
  

Patch

diff --git a/malloc/Versions b/malloc/Versions
index 62e4698a08..5e84bd5d05 100644
--- a/malloc/Versions
+++ b/malloc/Versions
@@ -96,5 +96,14 @@  libc {
     __libc_alloc_buffer_copy_bytes;
     __libc_alloc_buffer_copy_string;
     __libc_alloc_buffer_create_failure;
+
+    # Malloc debugging support
+    __malloc_debug_totem;
+    __malloc_check_malloc_usable_size;
+    __malloc_check_free;
+    __malloc_check_malloc;
+    __malloc_check_realloc;
+    __malloc_check_memalign;
+    __malloc_usable_size;
   }
 }
diff --git a/malloc/arena.c b/malloc/arena.c
index cae3387db0..1ae57f43d5 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -207,14 +207,6 @@  __malloc_fork_unlock_child (void)
 }
 
 #if HAVE_TUNABLES
-static void
-TUNABLE_CALLBACK (set_mallopt_check) (tunable_val_t *valp)
-{
-  int32_t value = (int32_t) valp->numval;
-  if (value != 0)
-    __malloc_check_init ();
-}
-
 # define TUNABLE_CALLBACK_FNDECL(__name, __type) \
 static inline int do_ ## __name (__type value);				      \
 static void									      \
@@ -323,7 +315,6 @@  ptmalloc_init (void)
   malloc_init_state (&main_arena);
 
 #if HAVE_TUNABLES
-  TUNABLE_GET (check, int32_t, TUNABLE_CALLBACK (set_mallopt_check));
   TUNABLE_GET (top_pad, size_t, TUNABLE_CALLBACK (set_top_pad));
   TUNABLE_GET (perturb, int32_t, TUNABLE_CALLBACK (set_perturb_byte));
   TUNABLE_GET (mmap_threshold, size_t, TUNABLE_CALLBACK (set_mmap_threshold));
@@ -401,8 +392,6 @@  ptmalloc_init (void)
             }
         }
     }
-  if (s && s[0] != '\0' && s[0] != '0')
-    __malloc_check_init ();
 #endif
 }
 
diff --git a/malloc/hooks.c b/malloc/hooks.c
index 04a909b52a..3bc08773af 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -35,6 +35,11 @@  void *weak_variable (*__realloc_hook)
 void *weak_variable (*__memalign_hook)
   (size_t, size_t, const void *) = memalign_hook_ini;
 
+/* This is interposed by libc_malloc_debug.so to match with a compatible libc.
+   We don't use dlsym or equivalent because the dlsym symbol version got bumped
+   in 2.34 and is hence unusable in libc_malloc_debug.so.  */
+unsigned __malloc_debug_totem = 0;
+
 /* Hooks for debugging versions.  The initial hooks just call the
    initialization routine, then do the normal work. */
 
@@ -59,6 +64,7 @@  generic_hook_ini (void)
   if (hook != NULL)
     (*hook)();
 #endif
+  __malloc_debug_totem = 1;
 }
 
 static void *
@@ -82,6 +88,8 @@  memalign_hook_ini (size_t alignment, size_t sz, const void *caller)
   return memalign (alignment, sz);
 }
 
+static bool force_malloc_check_off = false;
+
 #include "malloc-check.c"
 
 #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_25)
@@ -156,7 +164,7 @@  malloc_set_state (void *msptr)
   __realloc_hook = NULL;
   __free_hook = NULL;
   __memalign_hook = NULL;
-  using_malloc_checking = 0;
+  force_malloc_check_off = true;
 
   /* 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..54d77fb926 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
@@ -62,15 +48,14 @@  magicbyte (const void *p)
    memory.  Our magic byte is right at the end of the requested size, so we
    must reach it with this iteration, otherwise we have witnessed a memory
    corruption.  */
-static size_t
-malloc_check_get_size (mchunkptr p)
+size_t
+__malloc_check_malloc_usable_size (void *mem)
 {
   size_t size;
   unsigned char c;
+  mchunkptr p = mem2chunk (mem);
   unsigned char magic = magicbyte (p);
 
-  assert (using_malloc_checking == 1);
-
   for (size = CHUNK_HDR_SZ + memsize (p) - 1;
        (c = *SAFE_CHAR_OFFSET (p, size)) != magic;
        size -= c)
@@ -202,32 +187,42 @@  top_check (void)
   malloc_printerr ("malloc: top chunk is corrupt");
 }
 
-static void *
-malloc_check (size_t sz, const void *caller)
+static bool
+malloc_check (size_t sz, void **victimp)
 {
   void *victim;
   size_t nb;
 
+  if (force_malloc_check_off)
+    return false;
+
   if (__builtin_add_overflow (sz, 1, &nb))
     {
       __set_errno (ENOMEM);
-      return NULL;
+      *victimp = NULL;
+      return true;
     }
 
   __libc_lock_lock (main_arena.mutex);
   top_check ();
   victim = _int_malloc (&main_arena, nb);
   __libc_lock_unlock (main_arena.mutex);
-  return mem2mem_check (tag_new_usable (victim), sz);
+  *victimp = mem2mem_check (tag_new_usable (victim), sz);
+
+  return true;
 }
+strong_alias (malloc_check, __malloc_check_malloc)
 
-static void
-free_check (void *mem, const void *caller)
+static bool
+free_check (void *mem)
 {
   mchunkptr p;
 
+  if (force_malloc_check_off)
+    return false;
+
   if (!mem)
-    return;
+    return true;
 
   int err = errno;
 
@@ -253,28 +248,36 @@  free_check (void *mem, const void *caller)
       __libc_lock_unlock (main_arena.mutex);
     }
   __set_errno (err);
+
+  return true;
 }
+strong_alias (free_check, __malloc_check_free)
 
-static void *
-realloc_check (void *oldmem, size_t bytes, const void *caller)
+bool
+__malloc_check_realloc (void *oldmem, size_t bytes, void **victimp)
 {
   INTERNAL_SIZE_T chnb;
   void *newmem = 0;
   unsigned char *magic_p;
   size_t rb;
 
+  if (force_malloc_check_off)
+    return false;
+
   if (__builtin_add_overflow (bytes, 1, &rb))
     {
       __set_errno (ENOMEM);
-      return NULL;
+      *victimp = NULL;
+      return true;
     }
   if (oldmem == 0)
-    return malloc_check (bytes, NULL);
+    return malloc_check (bytes, victimp);
 
   if (bytes == 0)
     {
-      free_check (oldmem, NULL);
-      return NULL;
+      free_check (oldmem);
+      *victimp = NULL;
+      return true;
     }
 
   /* Quickly check that the freed pointer matches the tag for the memory.
@@ -344,16 +347,20 @@  invert:
 
   __libc_lock_unlock (main_arena.mutex);
 
-  return mem2mem_check (tag_new_usable (newmem), bytes);
+  *victimp = mem2mem_check (tag_new_usable (newmem), bytes);
+  return true;
 }
 
-static void *
-memalign_check (size_t alignment, size_t bytes, const void *caller)
+bool
+__malloc_check_memalign (size_t alignment, size_t bytes, void **victimp)
 {
   void *mem;
 
+  if (force_malloc_check_off)
+    return false;
+
   if (alignment <= MALLOC_ALIGNMENT)
-    return malloc_check (bytes, NULL);
+    return malloc_check (bytes, victimp);
 
   if (alignment < MINSIZE)
     alignment = MINSIZE;
@@ -363,14 +370,16 @@  memalign_check (size_t alignment, size_t bytes, const void *caller)
   if (alignment > SIZE_MAX / 2 + 1)
     {
       __set_errno (EINVAL);
-      return 0;
+      *victimp = NULL;
+      return true;
     }
 
   /* Check for overflow.  */
   if (bytes > SIZE_MAX - alignment - MINSIZE)
     {
       __set_errno (ENOMEM);
-      return 0;
+      *victimp = NULL;
+      return true;
     }
 
   /* Make sure alignment is power of 2.  */
@@ -386,5 +395,6 @@  memalign_check (size_t alignment, size_t bytes, const void *caller)
   top_check ();
   mem = _int_memalign (&main_arena, alignment, bytes + 1);
   __libc_lock_unlock (main_arena.mutex);
-  return mem2mem_check (tag_new_usable (mem), bytes);
+  *victimp = mem2mem_check (tag_new_usable (mem), bytes);
+  return true;
 }
diff --git a/malloc/malloc-check.h b/malloc/malloc-check.h
new file mode 100644
index 0000000000..6fae260b8b
--- /dev/null
+++ b/malloc/malloc-check.h
@@ -0,0 +1,32 @@ 
+/* glibc.malloc.check function interface for libc_malloc_debug.so.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <https://www.gnu.org/licenses/>.  */
+
+#ifndef _MALLOC_CHECK_H_
+# define _MALLOC_CHECK_H_
+
+# if !IS_IN (libc_malloc_debug)
+#  error "These functions must only be used in libc_malloc_debug.so"
+# else
+extern size_t __malloc_usable_size (void *);
+extern size_t __malloc_check_malloc_usable_size (void *);
+extern bool __malloc_check_malloc (size_t, void **);
+extern bool __malloc_check_free (void *);
+extern bool __malloc_check_realloc (void *, size_t, void **);
+extern bool __malloc_check_memalign (size_t, size_t, void **);
+# endif
+#endif
diff --git a/malloc/malloc-debug.c b/malloc/malloc-debug.c
index e810d47107..87048f8536 100644
--- a/malloc/malloc-debug.c
+++ b/malloc/malloc-debug.c
@@ -23,6 +23,14 @@ 
 #include <unistd.h>
 #include <sys/param.h>
 
+#define TUNABLE_NAMESPACE malloc
+#include <elf/dl-tunables.h>
+
+/* A compatible libc will set this totem to a non-zero value.  This is
+   needed for __malloc_check at the moment because the new __malloc_check_*
+   functions are not available in older libc.  */
+unsigned __malloc_debug_totem = 0;
+
 /* Support only the glibc allocators.  */
 extern void *__libc_malloc (size_t);
 extern void __libc_free (void *);
@@ -50,6 +58,7 @@  enum malloc_debug_hooks
   MALLOC_NONE_HOOK = 0,
   MALLOC_MCHECK_HOOK = 1 << 0, /* mcheck()  */
   MALLOC_MTRACE_HOOK = 1 << 1, /* mtrace()  */
+  MALLOC_CHECK_HOOK = 1 << 2,  /* MALLOC_CHECK_ or glibc.malloc.check.  */
 };
 static unsigned __malloc_debugging_hooks;
 
@@ -73,6 +82,7 @@  __malloc_debug_disable (enum malloc_debug_hooks flag)
 
 #include "mcheck.c"
 #include "mtrace.c"
+#include "malloc-check.h"
 
 extern void (*__free_hook) (void *, const void *);
 compat_symbol_reference (libc, __free_hook, __free_hook, GLIBC_2_0);
@@ -85,6 +95,32 @@  compat_symbol_reference (libc, __memalign_hook, __memalign_hook, GLIBC_2_0);
 
 static size_t pagesize;
 
+static void
+TUNABLE_CALLBACK (set_mallopt_check) (tunable_val_t *valp)
+{
+  int32_t value = (int32_t) valp->numval;
+  if (value != 0 && __malloc_debug_totem)
+    __malloc_debug_enable (MALLOC_CHECK_HOOK);
+}
+
+static __always_inline void
+maybe_initialize (void)
+{
+  if (!malloc_called)
+    {
+#if HAVE_TUNABLES
+      TUNABLE_GET (check, int32_t, TUNABLE_CALLBACK (set_mallopt_check));
+#else
+      const char *s = secure_getenv ("MALLOC_CHECK_");
+      if (s && s[0] != '\0' && s[0] != '0' && __malloc_debug_totem)
+	__malloc_debug_enable (MALLOC_CHECK_HOOK);
+#endif
+      /* The mcheck initializer runs before this through the initializer
+	 hook so it is safe to set this here.  */
+      malloc_called = true;
+    }
+}
+
 /* The allocator functions.  */
 
 static void *
@@ -94,12 +130,14 @@  __debug_malloc (size_t bytes)
   if (__builtin_expect (hook != NULL, 0))
     return (*hook)(bytes, RETURN_ADDRESS (0));
 
-  malloc_called = true;
+  maybe_initialize ();
 
   void *victim = NULL;
   size_t orig_bytes = bytes;
-  if (!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
-      || !malloc_mcheck_before (&bytes, &victim))
+  if ((!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
+       || !malloc_mcheck_before (&bytes, &victim))
+      && (!__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)
+	  || !__malloc_check_malloc (bytes, &victim)))
     {
       victim = __libc_malloc (bytes);
     }
@@ -124,10 +162,13 @@  __debug_free (void *mem)
 
   if (__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK))
     mem = free_mcheck (mem);
+  if (!__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)
+      || !__malloc_check_free (mem))
+    {
+      __libc_free (mem);
+    }
   if (__is_malloc_debug_enabled (MALLOC_MTRACE_HOOK))
     free_mtrace (mem, RETURN_ADDRESS (0));
-
-  __libc_free (mem);
 }
 strong_alias (__debug_free, free)
 
@@ -139,13 +180,15 @@  __debug_realloc (void *oldmem, size_t bytes)
   if (__builtin_expect (hook != NULL, 0))
     return (*hook)(oldmem, bytes, RETURN_ADDRESS (0));
 
-  malloc_called = true;
+  maybe_initialize ();
 
   size_t orig_bytes = bytes, oldsize = 0;
   void *victim = NULL;
 
-  if (!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
-      || !realloc_mcheck_before (&oldmem, &bytes, &oldsize, &victim))
+  if ((!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
+       || !realloc_mcheck_before (&oldmem, &bytes, &oldsize, &victim))
+      && (!__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)
+	  || !__malloc_check_realloc (oldmem, bytes, &victim)))
     {
       victim = __libc_realloc (oldmem, bytes);
     }
@@ -167,13 +210,15 @@  _mid_memalign (size_t alignment, size_t bytes, const void *address)
   if (__builtin_expect (hook != NULL, 0))
     return (*hook)(alignment, bytes, address);
 
-  malloc_called = true;
+  maybe_initialize ();
 
   void *victim = NULL;
   size_t orig_bytes = bytes;
 
-  if (!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
-      || !memalign_mcheck_before (alignment, &bytes, &victim))
+  if ((!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
+       || !memalign_mcheck_before (alignment, &bytes, &victim))
+      && (!__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)
+	  || !__malloc_check_memalign (alignment, bytes, &victim)))
     {
       victim = __libc_memalign (alignment, bytes);
     }
@@ -266,13 +311,15 @@  __debug_calloc (size_t nmemb, size_t size)
       return mem;
     }
 
-  malloc_called = true;
+  maybe_initialize ();
 
   size_t orig_bytes = bytes;
   void *victim = NULL;
 
-  if (!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
-      || !malloc_mcheck_before (&bytes, &victim))
+  if ((!__is_malloc_debug_enabled (MALLOC_MCHECK_HOOK)
+       || !malloc_mcheck_before (&bytes, &victim))
+      && (!__is_malloc_debug_enabled (MALLOC_CHECK_HOOK)
+	  || !__malloc_check_malloc (bytes, &victim)))
     {
       victim = __libc_malloc (bytes);
     }
@@ -288,3 +335,12 @@  __debug_calloc (size_t nmemb, size_t size)
   return victim;
 }
 strong_alias (__debug_calloc, calloc)
+
+size_t
+malloc_usable_size (void *mem)
+{
+  if (__is_malloc_debug_enabled (MALLOC_CHECK_HOOK))
+    return __malloc_check_malloc_usable_size (mem);
+
+  return __malloc_usable_size (mem);
+}
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 595dd8bbdb..1c1e1ab60b 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1114,13 +1114,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 ------------------  */
 
 
@@ -5054,9 +5047,6 @@  musable (void *mem)
 
       p = mem2chunk (mem);
 
-      if (__builtin_expect (using_malloc_checking == 1, 0))
-	return malloc_check_get_size (p);
-
       if (chunk_is_mmapped (p))
 	{
 	  if (DUMPED_MAIN_ARENA_CHUNK (p))