[v2] malloc: Set and reset all hooks for tracing (Bug 16573)

DJ noticed I accidentally cargo-culted the old __ptr_t usage and should
just use void * as we use today. The code is still not MT-safe, but we
won't get there until we finish the hook deprecation followed by the
new preloadable tracer replacement.

Still build and passes on x86_64.

v2
- Remove the old __ptr_t usage and replace with newer void * usage

8< --- 8< --- 8<
If an error occurs during the tracing operation, particularly during a
call to lock_and_info() which calls _dl_addr, we may end up calling back
into the malloc-subsystem and relock the loader lock and deadlock. For
all intents and purposes the call to _dl_addr can call any of the malloc
family API functions and so we should disable all tracing before calling
such loader functions.  This is similar to the strategy that the new
malloc tracer takes when calling the real malloc, namely that all
tracing ceases at the boundary to the real function and any faults at
that point are the purvue of the library (though the new tracer does
this on a per-thread basis in an MT-safe fashion). Since the new tracer
and the hook deprecation are not yet complete we must fix these issues
where we can.

Tested on x86_64 with no regressions.

Co-authored-by: Kwok Cheung Yeung <kcy@codesourcery.com>
---
  ChangeLog       | 15 +++++++++++
  malloc/mtrace.c | 72 +++++++++++++++++++++++++++++++------------------
  2 files changed, 61 insertions(+), 26 deletions(-)

Hello Carlos

Thank you for looking at this old issue! :-) It has been a while, but 
the patch looks reasonable to me too.

Regards

Kwok Yeung

On 09/04/2019 1:52 am, Carlos O'Donell wrote:
> DJ noticed I accidentally cargo-culted the old __ptr_t usage and should
> just use void * as we use today. The code is still not MT-safe, but we
> won't get there until we finish the hook deprecation followed by the
> new preloadable tracer replacement.
> 
> Still build and passes on x86_64.
> 
> v2
> - Remove the old __ptr_t usage and replace with newer void * usage
> 
> 8< --- 8< --- 8<
> If an error occurs during the tracing operation, particularly during a
> call to lock_and_info() which calls _dl_addr, we may end up calling back
> into the malloc-subsystem and relock the loader lock and deadlock. For
> all intents and purposes the call to _dl_addr can call any of the malloc
> family API functions and so we should disable all tracing before calling
> such loader functions.  This is similar to the strategy that the new
> malloc tracer takes when calling the real malloc, namely that all
> tracing ceases at the boundary to the real function and any faults at
> that point are the purvue of the library (though the new tracer does
> this on a per-thread basis in an MT-safe fashion). Since the new tracer
> and the hook deprecation are not yet complete we must fix these issues
> where we can.
> 
> Tested on x86_64 with no regressions.
> 
> Co-authored-by: Kwok Cheung Yeung <kcy@codesourcery.com>
> ---
>   ChangeLog       | 15 +++++++++++
>   malloc/mtrace.c | 72 +++++++++++++++++++++++++++++++------------------
>   2 files changed, 61 insertions(+), 26 deletions(-)
> 
> diff --git a/ChangeLog b/ChangeLog
> index 6b7f19a1f3..786bb52400 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,18 @@
> +2019-04-08  Carlos O'Donell  <carlos@redhat.com>
> +        Kwok Cheung Yeung  <kcy@codesourcery.com>
> +
> +    [BZ #16573]
> +    * malloc/mtrace.c: Define prototypes for all hooks.
> +    (set_default_hooks): New function.
> +    (set_trace_hooks): Likewise.
> +    (save_default_hooks): Likewise.
> +    (tr_freehook): Use new s*_hooks functions.
> +    (tr_mallochook): Likewise.
> +    (tr_reallochook): Likewise.
> +    (tr_memalignhook): Likewise.
> +    (mtrace): Likewise.
> +    (muntrace): Likewise.
> +
>   2019-04-08  Florian Weimer  <fweimer@redhat.com>
> 
>       * resolv/resolv.h (RES_INSECURE1, RES_INSECURE2): Remove
> diff --git a/malloc/mtrace.c b/malloc/mtrace.c
> index a2facf65ea..2fda262508 100644
> --- a/malloc/mtrace.c
> +++ b/malloc/mtrace.c
> @@ -121,6 +121,41 @@ lock_and_info (const void *caller, Dl_info *mem)
>     return res;
>   }
> 
> +static void tr_freehook (void *, const void *);
> +static void * tr_mallochook (size_t, const void *);
> +static void * tr_reallochook (void *, size_t, const void *);
> +static void * tr_memalignhook (size_t, size_t, const void *);
> +
> +/* Set all the default non-trace hooks.  */
> +static __always_inline void
> +set_default_hooks (void)
> +{
> +  __free_hook = tr_old_free_hook;
> +  __malloc_hook = tr_old_malloc_hook;
> +  __realloc_hook = tr_old_realloc_hook;
> +  __memalign_hook = tr_old_memalign_hook;
> +}
> +
> +/* Set all of the tracing hooks used for mtrace.  */
> +static __always_inline void
> +set_trace_hooks (void)
> +{
> +  __free_hook = tr_freehook;
> +  __malloc_hook = tr_mallochook;
> +  __realloc_hook = tr_reallochook;
> +  __memalign_hook = tr_memalignhook;
> +}
> +
> +/* Save the current set of hooks as the default hooks.  */
> +static __always_inline void
> +save_default_hooks (void)
> +{
> +  tr_old_free_hook = __free_hook;
> +  tr_old_malloc_hook = __malloc_hook;
> +  tr_old_realloc_hook = __realloc_hook;
> +  tr_old_memalign_hook = __memalign_hook;
> +}
> +
>   static void
>   tr_freehook (void *ptr, const void *caller)
>   {
> @@ -138,12 +173,12 @@ tr_freehook (void *ptr, const void *caller)
>         tr_break ();
>         __libc_lock_lock (lock);
>       }
> -  __free_hook = tr_old_free_hook;
> +  set_default_hooks ();
>     if (tr_old_free_hook != NULL)
>       (*tr_old_free_hook)(ptr, caller);
>     else
>       free (ptr);
> -  __free_hook = tr_freehook;
> +  set_trace_hooks ();
>     __libc_lock_unlock (lock);
>   }
> 
> @@ -155,12 +190,12 @@ tr_mallochook (size_t size, const void *caller)
>     Dl_info mem;
>     Dl_info *info = lock_and_info (caller, &mem);
> 
> -  __malloc_hook = tr_old_malloc_hook;
> +  set_default_hooks ();
>     if (tr_old_malloc_hook != NULL)
>       hdr = (void *) (*tr_old_malloc_hook)(size, caller);
>     else
>       hdr = (void *) malloc (size);
> -  __malloc_hook = tr_mallochook;
> +  set_trace_hooks ();
> 
>     tr_where (caller, info);
>     /* We could be printing a NULL here; that's OK.  */
> @@ -185,16 +220,12 @@ tr_reallochook (void *ptr, size_t size, const void 
> *caller)
>     Dl_info mem;
>     Dl_info *info = lock_and_info (caller, &mem);
> 
> -  __free_hook = tr_old_free_hook;
> -  __malloc_hook = tr_old_malloc_hook;
> -  __realloc_hook = tr_old_realloc_hook;
> +  set_default_hooks ();
>     if (tr_old_realloc_hook != NULL)
>       hdr = (void *) (*tr_old_realloc_hook)(ptr, size, caller);
>     else
>       hdr = (void *) realloc (ptr, size);
> -  __free_hook = tr_freehook;
> -  __malloc_hook = tr_mallochook;
> -  __realloc_hook = tr_reallochook;
> +  set_trace_hooks ();
> 
>     tr_where (caller, info);
>     if (hdr == NULL)
> @@ -230,14 +261,12 @@ tr_memalignhook (size_t alignment, size_t size, 
> const void *caller)
>     Dl_info mem;
>     Dl_info *info = lock_and_info (caller, &mem);
> 
> -  __memalign_hook = tr_old_memalign_hook;
> -  __malloc_hook = tr_old_malloc_hook;
> +  set_default_hooks ();
>     if (tr_old_memalign_hook != NULL)
>       hdr = (void *) (*tr_old_memalign_hook)(alignment, size, caller);
>     else
>       hdr = (void *) memalign (alignment, size);
> -  __memalign_hook = tr_memalignhook;
> -  __malloc_hook = tr_mallochook;
> +  set_trace_hooks ();
> 
>     tr_where (caller, info);
>     /* We could be printing a NULL here; that's OK.  */
> @@ -305,14 +334,8 @@ mtrace (void)
>             malloc_trace_buffer = mtb;
>             setvbuf (mallstream, malloc_trace_buffer, _IOFBF, 
> TRACE_BUFFER_SIZE);
>             fprintf (mallstream, "= Start\n");
> -          tr_old_free_hook = __free_hook;
> -          __free_hook = tr_freehook;
> -          tr_old_malloc_hook = __malloc_hook;
> -          __malloc_hook = tr_mallochook;
> -          tr_old_realloc_hook = __realloc_hook;
> -          __realloc_hook = tr_reallochook;
> -          tr_old_memalign_hook = __memalign_hook;
> -          __memalign_hook = tr_memalignhook;
> +      save_default_hooks ();
> +      set_trace_hooks ();
>   #ifdef _LIBC
>             if (!added_atexit_handler)
>               {
> @@ -338,10 +361,7 @@ muntrace (void)
>        file.  */
>     FILE *f = mallstream;
>     mallstream = NULL;
> -  __free_hook = tr_old_free_hook;
> -  __malloc_hook = tr_old_malloc_hook;
> -  __realloc_hook = tr_old_realloc_hook;
> -  __memalign_hook = tr_old_memalign_hook;
> +  set_default_hooks ();
> 
>     fprintf (f, "= End\n");
>     fclose (f);