From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 70564385EC56 for ; Thu, 24 Jun 2021 22:51:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 70564385EC56 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-411-HcZy-2PkPnKA42_GK_AkSQ-1; Thu, 24 Jun 2021 18:51:50 -0400 X-MC-Unique: HcZy-2PkPnKA42_GK_AkSQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id F3037804143; Thu, 24 Jun 2021 22:51:48 +0000 (UTC) Received: from greed.delorie.com (ovpn-112-111.rdu2.redhat.com [10.10.112.111]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6CDE060CCC; Thu, 24 Jun 2021 22:51:44 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.15.2/8.15.2) with ESMTP id 15OMpg1o448357; Thu, 24 Jun 2021 18:51:43 -0400 From: DJ Delorie To: Siddhesh Poyarekar Cc: libc-alpha@sourceware.org, carlos@redhat.com, fweimer@redhat.com Subject: Re: [PATCH 4/8] mcheck: Wean away from malloc hooks In-Reply-To: <20210624182312.236596-5-siddhesh@sourceware.org> Date: Thu, 24 Jun 2021 18:51:42 -0400 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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-List-Received-Date: Thu, 24 Jun 2021 22:51:54 -0000 Siddhesh Poyarekar 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 > +# include > +# include > + > +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 > # include > # include > -# include > # include > #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.