[PATCH] assert: Remove the use of %n from __assert_fail_base (BZ #32456)

[Adhemerval Zanella Netto]

On 30/12/24 14:06, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> The require size for mmap can be inferred from __vasprintf return
>> value.  It also fixes tst-assert-2 when building with --enable-fortify,
>> where even if the format is not translated, __readonly_area fails
>> because malloc can not be used.
>>
>> Checked on aarch64-linux-gnu.
>> ---
>>  assert/assert-perr.c |  2 +-
>>  assert/assert.c      | 27 ++++++++++-----------------
>>  2 files changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/assert/assert-perr.c b/assert/assert-perr.c
>> index 0010cbc278..8a03620b4c 100644
>> --- a/assert/assert-perr.c
>> +++ b/assert/assert-perr.c
>> @@ -32,7 +32,7 @@ __assert_perror_fail (int errnum,
>>    char errbuf[1024];
>>  
>>    char *e = __strerror_r (errnum, errbuf, sizeof errbuf);
>> -  __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n%n"),
>> +  __assert_fail_base (_("%s%s%s:%u: %s%sUnexpected error: %s.\n"),
>>  		      e, file, line, function);
>>  }
>>  libc_hidden_def (__assert_perror_fail)
> 
> I think you have to update the translations in the same commit,
> otherwise we'll end up with crashes due to %n leading to a write through
> an uninitialized pointer.

Right, I will do it on next version.

> 
>> @@ -56,12 +49,12 @@ __assert_fail_base (const char *fmt, const char *assertion, const char *file,
>>    FATAL_PREPARE;
>>  #endif
>>  
>> -  int total;
>> -  if (__asprintf (&str, fmt,
>> -		  __progname, __progname[0] ? ": " : "",
>> -		  file, line,
>> -		  function ? function : "", function ? ": " : "",
>> -		  assertion, &total) >= 0)
>> +  int total = __asprintf (&str, fmt,
>> +			  __progname, __progname[0] ? ": " : "",
>> +			  file, line,
>> +			  function ? function : "", function ? ": " : "",
>> +			  assertion);
>> +  if (total >= 0)
>>      {
>>        /* Print the message.  */
>>        (void) __fxprintf (NULL, "%s", str);
> 
> Somewhat unrelated: The code is rather odd.  Why would we use mmap here
> in addition to malloc?  It doesn't matter that much anymore because
> malloc's own asserts no longer use this code.  Still I suppose we could
> allocate a single patch and use __snprintf there.  Anyway, this can wait
> for a later cleanup.  We should remove the use of __fxprintf, too,
> although it won't be 100% backwards-compatible (for example, write the
> message only if there is a file descriptor associated with the stderr
> stream, so that we can write bytes directly without going through the
> wide stream state).

The patch that added mmap (f8a3b5bf8fa1d0c43d2458e03cc109a04fdef194) was
not clear about the rationale, but my wild guess would be to have a 
direct way to get the abort message in a post-mortem analysis (instead
to try dig it from malloc metadata).

But it is not really clear to me the why we need to keep the __abort_msg,
and if this is really useful.  Also, the assert implementation now requires
multiple allocation (the translation, the asprintf, and the mmap), which
adds a lot of overhead and multiple point of failures that makes the interface
moot (although mot likely memory allocation won't fail).