diff mbox series

[v4] libio: asprintf should write NULL upon failure

Message ID 871q2udpx6.fsf@oldenburg3.str.redhat.com (mailing list archive)
State Committed
Delegated to: Carlos O'Donell
Headers
Series [v4] libio: asprintf should write NULL upon failure |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

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
Sam James Sept. 20, 2024, 9:50 p.m. UTC | #3 
Solar Designer <solar@openwall.com> writes:

> 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.

It came up in
https://inbox.sourceware.org/libc-alpha/07aa5632-7db8-4962-a7ab-eb4cbb7c0157@redhat.com/.

Carlos is assigned to review. If we approach Feb (really, the freeze
before that) and it looks like there's no movement, please scream
loudly, as I'd really like it in for the next release.

>
> 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