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

[Carlos O'Donell]

On 6/25/25 3:33 PM, Adhemerval Zanella wrote:
> The iovec size should account for all substrings between each conversion
> specification.  For the format:
> 
>    "abc %s efg"
> 
> The list of substrings are:
> 
>    ["abc ", arg, " efg]
> 
> which is 2 times the number of maximum arguments *plus* one.
> 
> This issue triggered 'out of bounds' errors by stdlib/tst-bz20544 when
> glibc is built with experimental UBSAN support [1].
> 
> Besides adjusting the iovec size, a new runtime and check is added to
> avoid wrong __libc_message_impl usage.
> 
> Checked on x86_64-linux-gnu.

This version includes a runtime check to catch a defect in the macro
implementation, and a regression test to look for that check.

LGTM. This resolves bug 32947 completely.

Verified on x86_64 and i686 builds, and verified the test case triggers
when the change is reverted (A/B check):
tst-libc-message.c:41: error: string comparison failed
   left string: 10 bytes
   right string: 70 bytes
   left (evaluated from result.err.buffer):
       "1 2 3 4 5\n"
       31 20 32 20 33 20 34 20 35 0A
   right (evaluated from IOVEC_MAX_ERR_MSG):
       "Fatal glibc error: Internal __libc_message error. Too many arguments.\n"
...

Tested-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> [1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/ubsan-undef
> 
> Co-authored-by: Carlos O'Donell <carlos@redhat.com>
> --
> Changes from v3:
> * Fixed lint-makefile regression.
> Changes from v2:
> * Add runtime check and regression testcase.
> Changes from v1:
> * Add bug report.
> ---
>   include/stdio.h            |  4 ++++
>   posix/Makefile             |  2 ++
>   posix/tst-libc-message.c   | 48 ++++++++++++++++++++++++++++++++++++++
>   sysdeps/posix/libc_fatal.c | 32 ++++++++++++++-----------
>   4 files changed, 72 insertions(+), 14 deletions(-)
>   create mode 100644 posix/tst-libc-message.c
> 
> diff --git a/include/stdio.h b/include/stdio.h
> index e48d709919..3d917dba5b 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -173,6 +173,10 @@ libc_hidden_proto (__fortify_fail)
>   /* The maximum number of varargs allowed in a __libc_message format string */
>   #define LIBC_MESSAGE_MAX_ARGS 4
>   
> +#define IOVEC_MAX_ERR_MSG "Fatal glibc error: Internal " \
> +			  "__libc_message error. Too many arguments.\n"
> +#define IOVEC_MAX_ERR_MSG_LEN (sizeof (IOVEC_MAX_ERR_MSG) - 1)

OK. Using sizeof yields the size including the null terminator, so -1.

> +
>   _Noreturn void __libc_message_impl (const char *__fnt, ...) attribute_hidden
>        __attribute__ ((__format__ (__printf__, 1, 2)));
>   
> diff --git a/posix/Makefile b/posix/Makefile
> index c0e224236a..36b8b14c46 100644
> --- a/posix/Makefile
> +++ b/posix/Makefile
> @@ -348,6 +348,7 @@ tests-internal := \
>     bug-regex5 \
>     bug-regex20 \
>     bug-regex33 \
> +  tst-libc-message \

OK. Internal to access the function prototype.

>     # tests-internal
>   
>   tests-container := \
> @@ -391,6 +392,7 @@ endif
>   
>   tests-static = \
>     tst-exec-static \
> +  tst-libc-message \

OK. Static to access the function.

>     tst-spawn-static \
>     # tests-static
>   
> diff --git a/posix/tst-libc-message.c b/posix/tst-libc-message.c
> new file mode 100644
> index 0000000000..b85195e338
> --- /dev/null
> +++ b/posix/tst-libc-message.c
> @@ -0,0 +1,48 @@
> +/* Internal test to verify __libc_fatal.

OK.

> +   Copyright (C) 2025 Free Software Foundation, Inc.

OK.

> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <signal.h>
> +#include <stdio.h>
> +
> +#include <support/check.h>
> +#include <support/capture_subprocess.h>
> +
> +static _Noreturn void
> +run_libc_message (void *closure)
> +{
> +  /* We only support 4 arguments.  Call with 5 to trigger failure.  */
> +  __libc_message_impl ("%s %s %s %s %s\n", "1", "2", "3", "4", "5");

OK. Will trigger because this is 5 plus the \n.

> +  __builtin_unreachable ();
> +}
> +
> +static int
> +do_test (void)
> +{
> +  struct support_capture_subprocess result
> +    = support_capture_subprocess (run_libc_message, NULL);
> +  support_capture_subprocess_check (&result, "libc_message", -SIGABRT,
> +				    sc_allow_stderr);

OK. Sun as subprocess.

> +
> +  TEST_COMPARE_STRING (result.err.buffer, IOVEC_MAX_ERR_MSG);

OK. Compare the results.

> +
> +  support_capture_subprocess_free (&result);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> index d90cc6c681..6f75197e96 100644
> --- a/sysdeps/posix/libc_fatal.c
> +++ b/sysdeps/posix/libc_fatal.c
> @@ -16,23 +16,13 @@
>      License along with the GNU C Library; if not, see
>      <https://www.gnu.org/licenses/>.  */
>   
> -#include <atomic.h>
> -#include <errno.h>
> -#include <fcntl.h>
> +#include <assert.h>
>   #include <ldsodefs.h>
> -#include <libc-pointer-arith.h>
> -#include <paths.h>
> +#include <setvmaname.h>
>   #include <stdarg.h>
> -#include <stdbool.h>
>   #include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <sysdep.h>
> -#include <unistd.h>
> -#include <sys/mman.h>
>   #include <sys/uio.h>
> -#include <not-cancel.h>
> -#include <setvmaname.h>
> +#include <unistd.h>

OK. Nice cleanup.

>   
>   #ifdef FATAL_PREPARE_INCLUDE
>   #include FATAL_PREPARE_INCLUDE
> @@ -47,6 +37,10 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
>   }
>   #endif
>   
> +/* At most a substring before each conversion specification and the
> +   trailing substring (the plus one).  */
> +#define IOVEC_MAX (LIBC_MESSAGE_MAX_ARGS * 2 + 1)

OK.

> +
>   /* Abort with an error message.  */
>   void
>   __libc_message_impl (const char *fmt, ...)
> @@ -61,7 +55,7 @@ __libc_message_impl (const char *fmt, ...)
>     if (fd == -1)
>       fd = STDERR_FILENO;
>   
> -  struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 - 1];
> +  struct iovec iov[IOVEC_MAX];

OK.

>     int iovcnt = 0;
>     ssize_t total = 0;
>   
> @@ -99,6 +93,16 @@ __libc_message_impl (const char *fmt, ...)
>         iov[iovcnt].iov_len = len;
>         total += len;
>         iovcnt++;
> +
> +      if (__glibc_unlikely (iovcnt > IOVEC_MAX))
> +	{
> +	  len = IOVEC_MAX_ERR_MSG_LEN;

OK. Length of string without null terminator (matching strlen).

> +	  iov[0].iov_base = (char *) IOVEC_MAX_ERR_MSG;

OK. Message.

> +	  iov[0].iov_len = len;
> +	  total = len;
> +	  iovcnt = 1;

OK. One string.

> +	  break;
> +	}
>       }
>     va_end (ap);
>   


-- 
Cheers,
Carlos.