[PATCH v2] stdlib: Fix __libc_message_impl iovec size (BZ 32947)

[Carlos O'Donell]

On 5/8/25 9:07 AM, Adhemerval Zanella wrote:
> The iovec size should account for all substrings between each conversion
> specification.  For the format:

Looking forward to a v3.

>    "abc %s efg"
> 
> The list of substrings are:
> 
>    ["abc ", arg, " efg]
> 
> which is 2 times the number of maximum arguments *plus* one.

Agreed, each conversion specifier creates at most a prefix and the specifier
and at most one suffix (the plus 1) for the whole string.

Your change is technically correct, but I don't see the use that ubsan
triggered on.

I audited all users and we don't have any %s conversion specifiers in place t
day.

$ grep -rl '__libc_fatal.*%s' *
$

Current list of single line __libc_fatal messages:
~~~
Can't allocate _hurd_ports\n
hurd: Can't allocate sigstate\n
hurd: Can't add reference on Mach thread\n
msg receive failed on signal thread exc\n
BUG: unexpected fault in signal thread\n
hurd: Can't allocate file descriptor table\n
hurd: Can't allocate initial file descriptors\n
Fatal error: glibc detected an invalid stdio handle\n
FATAL: exception not rethrown\n
Fatal glibc error: rseq registration failed\n
Illegal status in internal_getgrouplist.\n
Illegal status in internal_getgrouplist.\n
Illegal status in __nss_next.\n
fork handler counter overflow
*** invalid %N$ use detected ***\n
*** %n in writable segments detected ***\n
*** procfs could not open ***\n
Fatal glibc error: cannot get entropy for arc4random\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Invalid DWARF unwind data.\n
The futex facility returned an unexpected error code.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
Unexpected reloc type in static binary.\n
~~~

Two line __libc_fatal messages:
~~~
Fatal glibc error: failed to register TLS destructor:
                   out of memory\n
~~~
\
Fatal glibc error: gconv module reference counter overflow\n
~~~

And via assert we call __asprintf or directly write via writev.

However, via our own internal __libc_assert_fail, we do call __libc_message
with up to 4 parameters.

e.g.
  31   __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n",
  32                   file, linestr, function, assertion);

$ grep -r '__libc_message ("' *
assert/__libc_assert_fail.c:  __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n",
debug/fortify_fail.c:  __libc_message ("*** %s ***: terminated\n", msg);
malloc/malloc.c:  __libc_message ("%s\n", str);
sysdeps/posix/libc_fatal.c:    __libc_message ("%s", message);

And for __libc_assert_fail we indeed trigger UB in this case.

> This issue triggered 'out of bounds' errors by stdlib/tst-bz20544 when
> glibc is built with experimental UBSAN support [1].
> 
> Checked on x86_64-linux-gnu.



> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/ubsan-undef
> --
> Changes from v1:
> * Add bug report.
> ---
>   sysdeps/posix/libc_fatal.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> index d90cc6c681..25ef20cfc1 100644
> --- a/sysdeps/posix/libc_fatal.c
> +++ b/sysdeps/posix/libc_fatal.c
> @@ -61,7 +61,7 @@ __libc_message_impl (const char *fmt, ...)
>     if (fd == -1)
>       fd = STDERR_FILENO;
>   

Why is it enough? Please add a short comment.

   /* At most a substring before each conversion specification and the
      trailing substring (the plus one).  */

> -  struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 - 1];
> +  struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 + 1];
>     int iovcnt = 0;
>     ssize_t total = 0;
>   

We still get UB if any internal caller does this wrong.

We are failing here, so performance doesn't matter.

May we please add something like this?

	if (iovcnt > (LIBC_MESSAGE_MAX_ARGS * 2 + 1))
	  {
	    /* Set up a static message.  */
             iov[0].iov_base = "Fatal glibc error: Internal __libc_message error. Too many arguments.\n";
	    iov[0].iov_len = strlen ("Fatal glibc error: Internal error. Too many arguments.\n");
	    total = len;
	    iovcnt = 1;
	    break;
	  }

That way we're never UB?

Alternatively we could attempt to print as much of the error as possible,
but that seems dangerous from a security perspective if the internal
constraints have been violated.

-- 
Cheers,
Carlos.