Re: [PATCH 4/8] mcheck: Wean away from malloc hooks

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

On 6/25/21 4:21 AM, DJ Delorie via Libc-alpha wrote:
> 
> Siddhesh Poyarekar <siddhesh@sourceware.org> 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.

It won't be; it is likely that a ctor preceding the DSO that was linked 
with -lmcheck could call malloc and hence result in ptmalloc_init being 
called before the ctor mallopt is called.

I reckon it has to be an ELF hack.  An exported symbol like 
__libc_lmcheck seems like the cleanest way to do it.

We could add an ELF note, but typically notes don't impact execution so 
that seems like a bad idea.

We could add a specially named segment (e.g. .glibc.lmcheck) with no 
contents (or a single nop word) that ld.so reads to decide if mcheck 
should be enabled.  However that's not too different from __libc_lmcheck 
in terms of ABI impact (we don't add a symbol version but its absence 
will have the same effect) with the added complexity in the dynamic linker.

>> -  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.

Added comment.

>> +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?

All paths initialize victim, but I've initialized it anyway and left it 
to the compiler to optimize it away.

Thanks,
Siddhesh