From patchwork Mon Jul 5 17:08:11 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 44150 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7BD16383F405 for ; Mon, 5 Jul 2021 17:13:28 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7BD16383F405 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1625505208; bh=Bf/0gKcbmcrhgUNS5dNWhHHlgFjFCp1MBS6g7dW+RlY=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ouHihjgZkpOHS7DpdFVJ5OS2viz+ZowZNHwd70t9kGNYZgTpfD2/WOMtL5fOSZr4O N+k4zpljxBK5N5RJJ3ijDL953tvT2C8wvfRh0cMyiVUQL6KqyKZl0rQz3mP63iuyK5 7kZvSHS+OomV/IsM5Vq6n9DQcrjUeeZ9M/CorBp0= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from antelope.elm.relay.mailchannels.net (antelope.elm.relay.mailchannels.net [23.83.212.4]) by sourceware.org (Postfix) with ESMTPS id 24E31383F404 for ; Mon, 5 Jul 2021 17:09:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 24E31383F404 X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id A353D5A100B; Mon, 5 Jul 2021 17:09:14 +0000 (UTC) Received: from pdx1-sub0-mail-a29.g.dreamhost.com (100-101-162-64.trex-nlb.outbound.svc.cluster.local [100.101.162.64]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 4326E5A118C; Mon, 5 Jul 2021 17:09:14 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a29.g.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384) by 100.101.162.64 (trex/6.3.3); Mon, 05 Jul 2021 17:09:14 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Celery-Madly: 481df92404e15396_1625504954541_2288557881 X-MC-Loop-Signature: 1625504954541:209302621 X-MC-Ingress-Time: 1625504954541 Received: from pdx1-sub0-mail-a29.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a29.g.dreamhost.com (Postfix) with ESMTP id EF5DF7E509; Mon, 5 Jul 2021 10:09:13 -0700 (PDT) Received: from rhbox.intra.reserved-bit.com (unknown [1.186.101.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a29.g.dreamhost.com (Postfix) with ESMTPSA id 2DA247E4A5; Mon, 5 Jul 2021 10:09:10 -0700 (PDT) X-DH-BACKEND: pdx1-sub0-mail-a29 To: libc-alpha@sourceware.org Subject: [PATCH v5 5/8] glibc.malloc.check: Wean away from malloc hooks Date: Mon, 5 Jul 2021 22:38:11 +0530 Message-Id: <20210705170814.4132997-6-siddhesh@sourceware.org> X-Mailer: git-send-email 2.31.1 In-Reply-To: <20210705170814.4132997-1-siddhesh@sourceware.org> References: <20210705170814.4132997-1-siddhesh@sourceware.org> MIME-Version: 1.0 X-Spam-Status: No, score=-3493.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NEUTRAL, TXREP, URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Siddhesh Poyarekar via Libc-alpha From: Siddhesh Poyarekar Reply-To: Siddhesh Poyarekar Cc: fweimer@redhat.com Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" 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 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 d924e35e6f..a9aebdd057 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 . */ -/* 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 . */ + +#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 #include +#define TUNABLE_NAMESPACE malloc +#include + +/* 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))