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 714BA3984034 for ; Fri, 2 Jul 2021 19:06:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 714BA3984034 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-339-VszBW3IMO0eGYHvFucUCqA-1; Fri, 02 Jul 2021 15:06:45 -0400 X-MC-Unique: VszBW3IMO0eGYHvFucUCqA-1 Received: by mail-qk1-f199.google.com with SMTP id a6-20020a37b1060000b02903b488f9d348so1964228qkf.20 for ; Fri, 02 Jul 2021 12:06:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=m7Xi7v89bO1OidpzCQHM9POtxgUIEgbNYJLDjefmW6g=; b=PM9s9R1U4L8IvmQZXRqQtscwtlZGibyN87jlapd7ww3RwONkOouE1EKR2xIchn5aP7 dspDXxf8pEQgb3xN4uILeeq7gHt/iso+fIBIIWi7E1R14l0lseQv/qQUNCSVdxydXWUc oqFNxhlZ5OsXbIJEYO0Ih9zjo40178Xu7ACs5itvOC7zar9RuxWOnbvZqZ/VA4jUNX3v +kJgaFFyW3dDbGJrW27eoUFx6aSEbgkz0TBQi9lBRKa3WBTAHhax2ffHurp1h2Qfmr77 n5r0JTh7dwLAg8Pig5ROqFj/ns2/jh7L4w3Wxq8bciSznE3/NhbNPqc4jrk2GB7JAK4n ZPZg== X-Gm-Message-State: AOAM533U75pLZHy1NDf+7+tw5e9dCc712ek7JKHQaA/6lcYgCGR9S6Fg 2QgP4bncJpEFqcukBRM6dgzE5F+s2qSiEYSVgG6C3crF60arBryTyMWTbA188jFXvAnS9BhvY8m hGm79T7aaO+RMe7N0fSlT X-Received: by 2002:ac8:4e86:: with SMTP id 6mr1169211qtp.159.1625252804409; Fri, 02 Jul 2021 12:06:44 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzOqCgGiTICtwXwwsIoSTA8/RNZ6PSUZ8fkwt9OrbmKscxpYqLDFcSsOF20BdoK+cFIi0BPbg== X-Received: by 2002:ac8:4e86:: with SMTP id 6mr1169190qtp.159.1625252804194; Fri, 02 Jul 2021 12:06:44 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id e10sm1721423qkg.18.2021.07.02.12.06.43 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 02 Jul 2021 12:06:43 -0700 (PDT) Subject: Re: [PATCH v4 05/10] glibc.malloc.check: Wean away from malloc hooks To: Siddhesh Poyarekar , libc-alpha@sourceware.org Cc: dj@redhat.com, fweimer@redhat.com References: <20210702113845.3367306-1-siddhesh@sourceware.org> <20210702113845.3367306-6-siddhesh@sourceware.org> From: Carlos O'Donell Organization: Red Hat Message-ID: Date: Fri, 2 Jul 2021 15:06:42 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210702113845.3367306-6-siddhesh@sourceware.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable 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-List-Received-Date: Fri, 02 Jul 2021 19:06:48 -0000 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 > Reviewed-by: DJ Delorie > --- > 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 > + > /* 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 . */ > > > -/* 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)) > -- Cheers, Carlos.