[PATCH v5] libio: asprintf should write NULL upon failure

[Sam James]

Florian Weimer <fweimer@redhat.com> writes:

> This was suggested most recently by Solar Designer, noting
> that code replacing vsprintf with vasprintf in a security fix
> was subtly wrong:
>
>   Re: GStreamer Security Advisory 2024-0003: Orc compiler
>   stack-based buffer overflow
>   <https://www.openwall.com/lists/oss-security/2024/07/26/2>
>
> Previous libc-alpha discussions:
>
>   I: [PATCH] asprintf error handling fix
>   <https://inbox.sourceware.org/libc-alpha/20011205185828.GA8376@ldv.office.alt-linux.org/>
>
>   asprintf() issue
>   <https://inbox.sourceware.org/libc-alpha/CANSoFxt-cdc-+C4u-rTENMtY4X9RpRSuv+axDswSPxbDgag8_Q@mail.gmail.com/>
>
> I don't think we need a compatibility symbol for this.  As the
> most recent GStreamer example shows, this change is much more
> likely to fix bugs than cause compatibility issues.
>
> Suggested-by: Dmitry V. Levin <ldv@altlinux.org>
> Suggested-by: Archie Cobbs <archie.cobbs@gmail.com>
> Suggested-by: Solar Designer <solar@openwall.com>

Reviewed-by: Sam James <sam@gentoo.org>

Thanks for remembering to revive this.

>
> ---
> v5: Repost to address Makefile conflict.

Not that it was needed here, but I wonder if we should encourage people
posting range-diffs. It would help me sometimes at least.

>  libio/Makefile            |  1 +
>  libio/tst-asprintf-null.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>  libio/vasprintf.c         | 17 ++++++++--------
>  manual/stdio.texi         |  9 ++++++++-
>  4 files changed, 68 insertions(+), 10 deletions(-)
>
> diff --git a/libio/Makefile b/libio/Makefile
> index a879e8c2ba..3b5adff74d 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -88,6 +88,7 @@ tests = \
>    test-fmemopen \
>    test-fputs-unbuffered-full \
>    test-fputws-unbuffered-full \
> +  tst-asprintf-null \
>    tst-atime \
>    tst-bz22415 \
>    tst-bz24051 \
> diff --git a/libio/tst-asprintf-null.c b/libio/tst-asprintf-null.c
> new file mode 100644
> index 0000000000..1eebeb200f
> --- /dev/null
> +++ b/libio/tst-asprintf-null.c
> @@ -0,0 +1,51 @@
> +/* Test that asprintf sets the buffer pointer to NULL on failure.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   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 <errno.h>
> +#include <stdio.h>
> +#include <support/check.h>
> +#include <sys/resource.h>
> +
> +static int
> +do_test (void)
> +{
> +  static const char sentinel[] = "sentinel";
> +  char *buf = (char *) sentinel;
> +  {
> +    /* Avoid -Wformat-overflow warning.  */
> +    const char *volatile format = "%2000000000d %2000000000d";
> +    TEST_COMPARE (asprintf (&buf, format, 1, 2), -1);
> +  }
> +  if (errno != ENOMEM)
> +    TEST_COMPARE (errno, EOVERFLOW);
> +  TEST_VERIFY (buf == NULL);
> +
> +  /* Force ENOMEM in the test below.  */
> +  struct rlimit rl;
> +  TEST_COMPARE (getrlimit (RLIMIT_AS, &rl), 0);
> +  rl.rlim_cur = 10 * 1024 * 1024;
> +  TEST_COMPARE (setrlimit (RLIMIT_AS, &rl), 0);
> +
> +  buf = (char *) sentinel;
> +  TEST_COMPARE (asprintf (&buf, "%20000000d", 1), -1);
> +  TEST_COMPARE (errno, ENOMEM);
> +  TEST_VERIFY (buf == NULL);
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/libio/vasprintf.c b/libio/vasprintf.c
> index 999ae526f4..24f2a2e175 100644
> --- a/libio/vasprintf.c
> +++ b/libio/vasprintf.c
> @@ -92,7 +92,7 @@ __printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
>  
>  
>  int
> -__vasprintf_internal (char **result_ptr, const char *format, va_list args,
> +__vasprintf_internal (char **result, const char *format, va_list args,
>  		      unsigned int mode_flags)
>  {
>    struct __printf_buffer_asprintf buf;
> @@ -105,23 +105,23 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args,
>      {
>        if (buf.base.write_base != buf.direct)
>  	free (buf.base.write_base);
> +      *result = NULL;
>        return done;
>      }
>  
>    /* Transfer to the final buffer.  */
> -  char *result;
>    size_t size = buf.base.write_ptr - buf.base.write_base;
>    if (buf.base.write_base == buf.direct)
>      {
> -      result = malloc (size + 1);
> -      if (result == NULL)
> +      *result = malloc (size + 1);
> +      if (*result == NULL)
>  	return -1;
> -      memcpy (result, buf.direct, size);
> +      memcpy (*result, buf.direct, size);
>      }
>    else
>      {
> -      result = realloc (buf.base.write_base, size + 1);
> -      if (result == NULL)
> +      *result = realloc (buf.base.write_base, size + 1);
> +      if (*result == NULL)
>  	{
>  	  free (buf.base.write_base);
>  	  return -1;
> @@ -129,8 +129,7 @@ __vasprintf_internal (char **result_ptr, const char *format, va_list args,
>      }
>  
>    /* Add NUL termination.  */
> -  result[size] = '\0';
> -  *result_ptr = result;
> +  (*result)[size] = '\0';
>  
>    return done;
>  }
> diff --git a/manual/stdio.texi b/manual/stdio.texi
> index 83f4f92e3f..747b1abf45 100644
> --- a/manual/stdio.texi
> +++ b/manual/stdio.texi
> @@ -2578,7 +2578,14 @@ Allocation}) to hold the output, instead of putting the output in a
>  buffer you allocate in advance.  The @var{ptr} argument should be the
>  address of a @code{char *} object, and a successful call to
>  @code{asprintf} stores a pointer to the newly allocated string at that
> -location.
> +location.  Current and future versions of @theglibc{} write a null
> +pointer to @samp{*@var{ptr}} upon failure.  To achieve similar
> +behavior with previous versions, initialize @samp{*@var{ptr}} to a
> +null pointer before calling @code{asprintf}.  (Specifications for
> +@code{asprintf} only require a valid pointer value in
> +@samp{*@var{ptr}} if @code{asprintf} succeeds, but no implementations
> +are known which overwrite a null pointer with a pointer that cannot be
> +freed on failure.)
>  
>  The return value is the number of characters allocated for the buffer, or
>  less than zero if an error occurred.  Usually this means that the buffer
>
> base-commit: 7c22dcda27743658b6b8ea479283b384ad56bd5a