diff mbox series

[v4] libio: asprintf should write NULL upon failure

Message ID 871q2udpx6.fsf@oldenburg3.str.redhat.com
State New
Headers show
Series [v4] libio: asprintf should write NULL upon failure | expand

Commit Message

Florian Weimer Aug. 12, 2024, 9:57 a.m. UTC 
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>

---
v4: Try to find a middle-ground in the manual description of the failure
    casae between actually encountered behavior and specified behavior.
 libio/Makefile            |  1 +
 libio/tst-asprintf-null.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++
 libio/vasprintf.c         | 17 ++++++++--------
 manual/stdio.texi         |  9 ++++++++-
 4 files changed, 68 insertions(+), 10 deletions(-)


base-commit: c2a474f4617ede7a8bf56b7257acb37dc757b2d1

Comments

Solar Designer Aug. 13, 2024, 6:27 p.m. UTC | #1 
On Mon, Aug 12, 2024 at 11:57:25AM +0200, Florian Weimer wrote:
> +++ b/manual/stdio.texi
> @@ -2517,7 +2517,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.)

Thanks.  I'm not happy that this doesn't suggest the only guaranteed
correct usage per POSIX (check the return value), but like I wrote
before I didn't mean to delay getting these changes in by commenting on
the documentation.  So let's proceed with the above.

Alexander
Solar Designer Sept. 13, 2024, 3:59 p.m. UTC | #2 
Hi Florian,

On Tue, Aug 13, 2024 at 08:27:26PM +0200, Solar Designer wrote:
> On Mon, Aug 12, 2024 at 11:57:25AM +0200, Florian Weimer wrote:
> > +++ b/manual/stdio.texi
> > @@ -2517,7 +2517,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.)
> 
> Thanks.  I'm not happy that this doesn't suggest the only guaranteed
> correct usage per POSIX (check the return value), but like I wrote
> before I didn't mean to delay getting these changes in by commenting on
> the documentation.  So let's proceed with the above.

What's the status on this?  I really did not mean to delay getting this
in.  If my comments have caused delay, then please disregard them.

Thanks,

Alexander
diff mbox series

Patch

diff --git a/libio/Makefile b/libio/Makefile
index 6a507b67ea..a2d1cde955 100644
--- a/libio/Makefile
+++ b/libio/Makefile
@@ -86,6 +86,7 @@  tests = \
   bug-wmemstream1 \
   bug-wsetpos \
   test-fmemopen \
+  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 8517653507..5ba6dfcf7a 100644
--- a/manual/stdio.texi
+++ b/manual/stdio.texi
@@ -2517,7 +2517,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