[PATCH] malloc: Simplify implementation of __malloc_assert

[PATCH] malloc: Simplify implementation of __malloc_assert

[Florian Weimer]

It is prudent not to run too much code after detecting heap
corruption, and __fxprintf is really complex.  The line number
and file name do not carry much information, so it is not included
in the error message.  (__libc_message only supports %s formatting.)
The function name and assertion should provide some context.

Tested on i686-linux-gnu and x86_64-linux-gnu.  Also emulated the
__libc_message call using GDB, and it produced the expected output.

Thanks,
Florian
---
 malloc/malloc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 12908b8f97..bd3c76ed31 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -292,19 +292,14 @@
 # define __assert_fail(assertion, file, line, function)			\
 	 __malloc_assert(assertion, file, line, function)
 
-extern const char *__progname;
-
-static void
+_Noreturn static void
 __malloc_assert (const char *assertion, const char *file, unsigned int line,
 		 const char *function)
 {
-  (void) __fxprintf (NULL, "%s%s%s:%u: %s%sAssertion `%s' failed.\n",
-		     __progname, __progname[0] ? ": " : "",
-		     file, line,
-		     function ? function : "", function ? ": " : "",
-		     assertion);
-  fflush (stderr);
-  abort ();
+  __libc_message (do_abort, "\
+Fatal glibc error: malloc assertion failure in %s: %s\n",
+		  function, assertion);
+  __builtin_unreachable ();
 }
 #endif
 #endif

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Siddhesh Poyarekar]

On 2022-07-21 06:34, Florian Weimer via Libc-alpha wrote:
> It is prudent not to run too much code after detecting heap
> corruption, and __fxprintf is really complex.  The line number
> and file name do not carry much information, so it is not included
> in the error message.  (__libc_message only supports %s formatting.)
> The function name and assertion should provide some context.
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.  Also emulated the
> __libc_message call using GDB, and it produced the expected output.
> 
> Thanks,
> Florian
> ---

LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

>   malloc/malloc.c | 15 +++++----------
>   1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 12908b8f97..bd3c76ed31 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -292,19 +292,14 @@
>   # define __assert_fail(assertion, file, line, function)			\
>   	 __malloc_assert(assertion, file, line, function)
>   
> -extern const char *__progname;
> -
> -static void
> +_Noreturn static void
>   __malloc_assert (const char *assertion, const char *file, unsigned int line,
>   		 const char *function)
>   {
> -  (void) __fxprintf (NULL, "%s%s%s:%u: %s%sAssertion `%s' failed.\n",
> -		     __progname, __progname[0] ? ": " : "",
> -		     file, line,
> -		     function ? function : "", function ? ": " : "",
> -		     assertion);
> -  fflush (stderr);
> -  abort ();
> +  __libc_message (do_abort, "\
> +Fatal glibc error: malloc assertion failure in %s: %s\n",
> +		  function, assertion);
> +  __builtin_unreachable ();
>   }
>   #endif
>   #endif
> 

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Carlos O'Donell]

On 7/21/22 07:43, Siddhesh Poyarekar wrote:
> On 2022-07-21 06:34, Florian Weimer via Libc-alpha wrote:
>> It is prudent not to run too much code after detecting heap
>> corruption, and __fxprintf is really complex.  The line number
>> and file name do not carry much information, so it is not included
>> in the error message.  (__libc_message only supports %s formatting.)
>> The function name and assertion should provide some context.
>>
>> Tested on i686-linux-gnu and x86_64-linux-gnu.  Also emulated the
>> __libc_message call using GDB, and it produced the expected output.
>>
>> Thanks,
>> Florian
>> ---
> 
> LGTM.
> 
> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
 
Approved as RM for glibc 2.36.

This avoids us calling malloc down the failing path.

>   malloc/malloc.c | 15 +++++----------
>>   1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>> index 12908b8f97..bd3c76ed31 100644
>> --- a/malloc/malloc.c
>> +++ b/malloc/malloc.c
>> @@ -292,19 +292,14 @@
>>   # define __assert_fail(assertion, file, line, function)            \
>>        __malloc_assert(assertion, file, line, function)
>>   -extern const char *__progname;
>> -
>> -static void
>> +_Noreturn static void
>>   __malloc_assert (const char *assertion, const char *file, unsigned int line,
>>            const char *function)
>>   {
>> -  (void) __fxprintf (NULL, "%s%s%s:%u: %s%sAssertion `%s' failed.\n",
>> -             __progname, __progname[0] ? ": " : "",
>> -             file, line,
>> -             function ? function : "", function ? ": " : "",
>> -             assertion);
>> -  fflush (stderr);
>> -  abort ();
>> +  __libc_message (do_abort, "\
>> +Fatal glibc error: malloc assertion failure in %s: %s\n",
>> +          function, assertion);
>> +  __builtin_unreachable ();
>>   }
>>   #endif
>>   #endif
>>
> 


-- 
Cheers,
Carlos.

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Adhemerval Zanella Netto]

On 21/07/22 07:34, Florian Weimer via Libc-alpha wrote:
> It is prudent not to run too much code after detecting heap
> corruption, and __fxprintf is really complex.  The line number
> and file name do not carry much information, so it is not included
> in the error message.  (__libc_message only supports %s formatting.)
> The function name and assertion should provide some context.
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.  Also emulated the
> __libc_message call using GDB, and it produced the expected output.
> 
> Thanks,
> Florian
> ---
>  malloc/malloc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 12908b8f97..bd3c76ed31 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -292,19 +292,14 @@
>  # define __assert_fail(assertion, file, line, function)			\
>  	 __malloc_assert(assertion, file, line, function)
>  
> -extern const char *__progname;
> -
> -static void
> +_Noreturn static void
>  __malloc_assert (const char *assertion, const char *file, unsigned int line,
>  		 const char *function)
>  {
> -  (void) __fxprintf (NULL, "%s%s%s:%u: %s%sAssertion `%s' failed.\n",
> -		     __progname, __progname[0] ? ": " : "",
> -		     file, line,
> -		     function ? function : "", function ? ": " : "",
> -		     assertion);
> -  fflush (stderr);
> -  abort ();
> +  __libc_message (do_abort, "\
> +Fatal glibc error: malloc assertion failure in %s: %s\n",
> +		  function, assertion);
> +  __builtin_unreachable ();
>  }
>  #endif
>  #endif
> 

Would be better to keep the the file and line information?  

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Adhemerval Zanella Netto]

On 21/07/22 10:10, Adhemerval Zanella Netto wrote:
> 
> 
> On 21/07/22 07:34, Florian Weimer via Libc-alpha wrote:
>> It is prudent not to run too much code after detecting heap
>> corruption, and __fxprintf is really complex.  The line number
>> and file name do not carry much information, so it is not included
>> in the error message.  (__libc_message only supports %s formatting.)
>> The function name and assertion should provide some context.
>>
>> Tested on i686-linux-gnu and x86_64-linux-gnu.  Also emulated the
>> __libc_message call using GDB, and it produced the expected output.
>>
>> Thanks,
>> Florian
>> ---
>>  malloc/malloc.c | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>> index 12908b8f97..bd3c76ed31 100644
>> --- a/malloc/malloc.c
>> +++ b/malloc/malloc.c
>> @@ -292,19 +292,14 @@
>>  # define __assert_fail(assertion, file, line, function)			\
>>  	 __malloc_assert(assertion, file, line, function)
>>  
>> -extern const char *__progname;
>> -
>> -static void
>> +_Noreturn static void
>>  __malloc_assert (const char *assertion, const char *file, unsigned int line,
>>  		 const char *function)
>>  {
>> -  (void) __fxprintf (NULL, "%s%s%s:%u: %s%sAssertion `%s' failed.\n",
>> -		     __progname, __progname[0] ? ": " : "",
>> -		     file, line,
>> -		     function ? function : "", function ? ": " : "",
>> -		     assertion);
>> -  fflush (stderr);
>> -  abort ();
>> +  __libc_message (do_abort, "\
>> +Fatal glibc error: malloc assertion failure in %s: %s\n",
>> +		  function, assertion);
>> +  __builtin_unreachable ();
>>  }
>>  #endif
>>  #endif
>>
> 
> Would be better to keep the the file and line information?  

And since the idea here is to avoid the malloc from __asprintf, would be better
to make __assert_fail_base call __libc_message (which will either call alloca
or mmap) ? Then there is no need to reimplement it for malloc.

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Florian Weimer]

* Adhemerval Zanella Netto:

> And since the idea here is to avoid the malloc from __asprintf, would
> be better to make __assert_fail_base call __libc_message (which will
> either call alloca or mmap) ? Then there is no need to reimplement it
> for malloc.

No, assert is required to print to stderr (the stdio stream) by POSIX
and ISO C.  But we don't want to call into libio after detecting heap
corruption.

Thanks,
Florian

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Florian Weimer]

* Adhemerval Zanella Netto:

>> +  __libc_message (do_abort, "\
>> +Fatal glibc error: malloc assertion failure in %s: %s\n",
>> +		  function, assertion);
>> +  __builtin_unreachable ();
>>  }
>>  #endif
>>  #endif
>> 
>
> Would be better to keep the the file and line information?  

I would have to figure out how to use _itoa correctly. 8-/

I think we converted almost all asserts that are reachable through heap
corruption into malloc_printerr a couple of years ago, so the diagnostic
value of the line number will be really low.  That's why I had to resort
to GDB to test this.

Do you insist that I add file and line number information?

Thanks,
Florian

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Adhemerval Zanella Netto]

On 21/07/22 10:26, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> +  __libc_message (do_abort, "\
>>> +Fatal glibc error: malloc assertion failure in %s: %s\n",
>>> +		  function, assertion);
>>> +  __builtin_unreachable ();
>>>  }
>>>  #endif
>>>  #endif
>>>
>>
>> Would be better to keep the the file and line information?  
> 
> I would have to figure out how to use _itoa correctly. 8-/

Yeah, it is not the best interface indeed.

> 
> I think we converted almost all asserts that are reachable through heap
> corruption into malloc_printerr a couple of years ago, so the diagnostic
> value of the line number will be really low.  That's why I had to resort
> to GDB to test this.
> 
> Do you insist that I add file and line number information?

I think it should be ok, what bothers me is we need to reimplement the
assert macro for malloc usage.

Re: [PATCH] malloc: Simplify implementation of __malloc_assert

[Florian Weimer]

* Adhemerval Zanella Netto:

>> I think we converted almost all asserts that are reachable through heap
>> corruption into malloc_printerr a couple of years ago, so the diagnostic
>> value of the line number will be really low.  That's why I had to resort
>> to GDB to test this.
>> 
>> Do you insist that I add file and line number information?
>
> I think it should be ok, what bothers me is we need to reimplement the
> assert macro for malloc usage.

It occurred to me that we do not need to use stderr for internal assert
calls.  We can use a different implementation based on __libc_message.
Then the malloc code does not need its own version anymore.  We can also
change __libc_message to make it non-catchable (in process), and reset
SIGABRT to SIG_DFL earlier.  But maybe the last step goes too far.

Thanks,
Florian