[PATCH v2] stdlib: Fix __libc_message_impl iovec size (BZ 32947)
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Wed May 28 18:54:12 GMT 2025
On 20/05/25 09:40, Carlos O'Donell wrote:
> 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.
It is indeed by __libc_assert_fail.c:31. With my ubsan branch, the tst-bz20544 shows:
(gdb) bt
#0 __pthread_kill_implementation (threadid=<optimized out>, signo=6, no_tid=0) at pthread_kill.c:44
#1 0x00007ffff7f9dbb2 in raise (sig=6) at ../sysdeps/posix/raise.c:26
#2 0x00007ffff7f0d6fb in ubsan_abort () at ubsan_error.c:32
#3 __ubsan_error (source=source@entry=0x7ffff7c175a0,
fmt=fmt@entry=0x7ffff7fa98e8 <error: Cannot access memory at address 0x7ffff7fa98e8>) at ubsan_error.c:56
#4 0x00007ffff7f08999 in __GI___ubsan_handle_out_of_bounds (_data=_data@entry=0x7ffff7c175a0,
index=<error reading variable: Cannot access memory at address 0x7fffffffd468>)
at ubsan_handle_out_of_bounds.c:29
#5 0x00007ffff77bada6 in __libc_message_impl (
fmt=fmt@entry=0x7ffff7b03be0 <error: Cannot access memory at address 0x7ffff7b03be0>)
at ../sysdeps/posix/libc_fatal.c:98
#6 0x00007ffff768e7e1 in __libc_assert_fail (
assertion=assertion@entry=0x7ffff7afcb3e <error: Cannot access memory at address 0x7ffff7afcb3e>,
file=file@entry=0x7ffff7afcb31 <error: Cannot access memory at address 0x7ffff7afcb31>,
line=line@entry=41,
function=function@entry=0x7ffff7b0abd0 <__PRETTY_FUNCTION__.0> <error: Cannot access memory at address 0x7ffff7b0abd0>) at __libc_assert_fail.c:31
#7 0x00007ffff76b3abb in __internal_atexit (func=func@entry=0x0, arg=arg@entry=0x0, d=<optimized out>,
listp=listp@entry=0x7ffff7b99d78 <__exit_funcs>) at cxa_atexit.c:41
#8 0x00007ffff76b3b04 in __GI___cxa_atexit (func=func@entry=0x0, arg=arg@entry=0x0, d=<optimized out>)
at cxa_atexit.c:68
#9 0x000055555555dd26 in atexit (func=func@entry=0x0) at atexit.c:46
#10 0x0000555555557ce9 in test_bz20544_atexit (closure=closure@entry=0x0) at tst-bz20544.c:46
#11 0x000055555555a261 in support_subprocess (callback=callback@entry=0x555555557cd0 <test_bz20544_atexit>,
closure=closure@entry=0x0) at support_subprocess.c:62
#12 0x0000555555558608 in support_capture_subprocess (
callback=callback@entry=0x555555557cd0 <test_bz20544_atexit>, closure=closure@entry=0x0)
at support_capture_subprocess.c:94
#13 0x0000555555557d4e in test_one_fn (test_fn=test_fn@entry=0x555555557cd0 <test_bz20544_atexit>)
at tst-bz20544.c:83
#14 0x0000555555557e2e in do_test () at tst-bz20544.c:106
#15 0x000055555555bc52 in support_test_main (argc=1, argv=0x7fffffffda48, config=config@entry=0x7fffffffd8c0)
at support_test_main.c:441
#16 0x0000555555557a79 in main (argc=<optimized out>, argv=<optimized out>) at ../support/test-driver.c:171
>
>> 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?
We don't need this because invalid usage should be catch at *build* time
due how the __libc_message macro is implemented. On include/stdio.h, it
only supports calls up to LIBC_MESSAGE_MAX_ARGS because of the
__libc_message_disp expansion and the __libc_messageN definitions.
Issuing a __libc_message with a number of arguments larger than
LIBC_MESSAGE_MAX_ARGS with a bogus changes like:
diff --git a/assert/__libc_assert_fail.c b/assert/__libc_assert_fail.c
index b50637a893..9e15ac9ceb 100644
--- a/assert/__libc_assert_fail.c
+++ b/assert/__libc_assert_fail.c
@@ -28,6 +28,6 @@ __libc_assert_fail (const char *assertion, const char *file, unsigned int line,
char linebuf[INT_BUFSIZE_BOUND (unsigned int)];
array_end (linebuf)[-1] = '\0';
char *linestr = _itoa_word (line, array_end (linebuf) - 1, 10, 0);
- __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n",
- file, linestr, function, assertion);
+ __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s %s\n",
+ file, linestr, function, assertion, "testing");
}
triggers the build issue:
In file included from __libc_assert_fail.c:22:
__libc_assert_fail.c: In function ‘__libc_assert_fail’:
../include/stdio.h:199:25: error: implicit declaration of function ‘__libc_message5’; did you mean ‘__libc_message_impl’? [-Wimplicit-function-declaration]
199 | __libc_message_disp (__libc_message, __VA_ARGS__)
| ^~~~~~~~~~~~~~
../include/stdio.h:190:39: note: in definition of macro ‘__libc_message_concat_x’
190 | #define __libc_message_concat_x(a,b) a##b
| ^
../include/stdio.h:197:4: note: in expansion of macro ‘__libc_message_concat’
197 | __libc_message_concat (b, __libc_message_nargs (__VA_ARGS__))(__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~~~
../include/stdio.h:199:4: note: in expansion of macro ‘__libc_message_disp’
199 | __libc_message_disp (__libc_message, __VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~
__libc_assert_fail.c:31:3: note: in expansion of macro ‘__libc_message’
31 | __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s %s\n",
| ^~~~~~~~~~~~~~
>
> 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.
>
> Can we write a tests-internal test case that is compiled statically and
> can call __libc_message with >4 %s and trigger this message?
We can't really code this test because it won't build unless we start to
add larger __libc_messageN, which defeats the macro itself.
More information about the Libc-alpha
mailing list