| Message ID | 871q3cqy0j.fsf@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | libio: asprintf should write NULL upon failure | expand |
Florian Weimer <fweimer@redhat.com> writes: > And update the manual to mention that function is now part > of POSIX. I'd still prefer it as a separate commit given one part is obviously fine to backport and one isn't. (Just to 2.40 would be nice because I can see people filing a bug over the docs being stale.) > > 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> > > --- > v2: Commit message updated as suggested by Andreas Schwab. > libio/Makefile | 1 + > libio/tst-asprintf-null.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++ > libio/vasprintf.c | 17 ++++++++-------- > manual/stdio.texi | 9 +++++++-- > 4 files changed, 67 insertions(+), 11 deletions(-) > > 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 f5e289d58a..0d63897b74 100644 > --- a/manual/stdio.texi > +++ b/manual/stdio.texi > @@ -2516,7 +2516,7 @@ The functions in this section do formatted output and place the results > in dynamically allocated memory. > > @deftypefun int asprintf (char **@var{ptr}, const char *@var{template}, @dots{}) > -@standards{GNU, stdio.h} > +@standards{POSIX, stdio.h} > @safety{@prelim{}@mtsafe{@mtslocale{}}@asunsafe{@ascuheap{}}@acunsafe{@acsmem{}}} > This function is similar to @code{sprintf}, except that it dynamically > allocates a string (as with @code{malloc}; @pxref{Unconstrained > @@ -2524,7 +2524,9 @@ 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 versions of @theglibc{} write a null pointer to > +@samp{*@var{ptr}} upon failure, but this is not required by the > +standard, and previous versions did not modify the pointer value. > > 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 > @@ -2545,6 +2547,9 @@ make_message (char *name, char *value) > return result; > @} > @end smallexample > + > +The @code{asprintf} function was a GNU extension initially, but is now > +part of POSIX. > @end deftypefun > > @deftypefun int obstack_printf (struct obstack *@var{obstack}, const char *@var{template}, @dots{}) > > base-commit: 32328a5a1461ff88c0b1e04954e9c68b3fa7f56d LGTM modulo my preference to split. Sorry about the race condition. Reviewed-by: Sam James <sam@gentoo.org>
* Sam James: > Florian Weimer <fweimer@redhat.com> writes: > >> And update the manual to mention that function is now part >> of POSIX. > > I'd still prefer it as a separate commit given one part is obviously > fine to backport and one isn't. (Just to 2.40 would be nice because I > can see people filing a bug over the docs being stale.) Sorry, which part is obviously fine to backport? Thanks, Florian
On Mon, 29 Jul 2024, Sam James wrote: > Florian Weimer <fweimer@redhat.com> writes: > > > And update the manual to mention that function is now part > > of POSIX. > > I'd still prefer it as a separate commit given one part is obviously > fine to backport and one isn't. (Just to 2.40 would be nice because I > can see people filing a bug over the docs being stale.) > > LGTM modulo my preference to split. Sorry about the race condition. > > Reviewed-by: Sam James <sam@gentoo.org> Did you see the previous asprintf thread? https://inbox.sourceware.org/libc-alpha/CANSoFxt-cdc-+C4u-rTENMtY4X9RpRSuv+axDswSPxbDgag8_Q@mail.gmail.com/T/ It is quite disappointing that when a new contributor tries to make that change it is bogged down in discussions about symbol versioning and then successfully forgotten. Is the systemd issue referenced there irrelevant now? Alexander
Alexander Monakov <amonakov@ispras.ru> writes: > On Mon, 29 Jul 2024, Sam James wrote: > >> Florian Weimer <fweimer@redhat.com> writes: >> >> > And update the manual to mention that function is now part >> > of POSIX. >> >> I'd still prefer it as a separate commit given one part is obviously >> fine to backport and one isn't. (Just to 2.40 would be nice because I >> can see people filing a bug over the docs being stale.) >> >> LGTM modulo my preference to split. Sorry about the race condition. >> >> Reviewed-by: Sam James <sam@gentoo.org> > > Did you see the previous asprintf thread? > > https://inbox.sourceware.org/libc-alpha/CANSoFxt-cdc-+C4u-rTENMtY4X9RpRSuv+axDswSPxbDgag8_Q@mail.gmail.com/T/ > No, I hadn't, although I've just read it now quickly. I don't agree with the need for a compat symbol but given the substantial debate on that at the time, I suppose we need to obtain broader consensus here. > It is quite disappointing that when a new contributor tries to make that change > it is bogged down in discussions about symbol versioning and then successfully > forgotten. I wasn't even involved in glibc develpment then, let alone being influential (which I'm not now, even). I agree it's rather unfortunate and quite sad to read. I think we've got better at this in the years I've been around to observe at least. GCC struggles with this quite a lot but glibc and binutils got better there. > > Is the systemd issue referenced there irrelevant now? > https://github.com/systemd/systemd/commit/305757d80819873c8918c6ea229f96fc10b77696 and kdbus is long gone. > Alexander thanks, sam
Florian Weimer <fweimer@redhat.com> writes: > * Sam James: > >> Florian Weimer <fweimer@redhat.com> writes: >> >>> And update the manual to mention that function is now part >>> of POSIX. >> >> I'd still prefer it as a separate commit given one part is obviously >> fine to backport and one isn't. (Just to 2.40 would be nice because I >> can see people filing a bug over the docs being stale.) > > Sorry, which part is obviously fine to backport? The POSIX documentation change, given glibc-2.40 was released after Issue 8 was ratified. The rest is going to need discussion, even though we might do it downstream. > > Thanks, > Florian thanks, sam
* Alexander Monakov: > On Mon, 29 Jul 2024, Sam James wrote: > >> Florian Weimer <fweimer@redhat.com> writes: >> >> > And update the manual to mention that function is now part >> > of POSIX. >> >> I'd still prefer it as a separate commit given one part is obviously >> fine to backport and one isn't. (Just to 2.40 would be nice because I >> can see people filing a bug over the docs being stale.) >> >> LGTM modulo my preference to split. Sorry about the race condition. >> >> Reviewed-by: Sam James <sam@gentoo.org> > > Did you see the previous asprintf thread? > > https://inbox.sourceware.org/libc-alpha/CANSoFxt-cdc-+C4u-rTENMtY4X9RpRSuv+axDswSPxbDgag8_Q@mail.gmail.com/T/ Obviously I saw it at the time, but I have forgotten about it. > It is quite disappointing that when a new contributor tries to make > that change it is bogged down in discussions about symbol versioning > and then successfully forgotten. Agreed. We try to avoid that nowadays. > Is the systemd issue referenced there irrelevant now? The systemd issue is this one: Bug 90017 - Improper use of asprintf() in login/pam_systemd.c <https://bugs.freedesktop.org/show_bug.cgi?id=90017> | #ifdef ENABLE_KDBUS | _cleanup_free_ char *s = NULL; | int r; | | /* skip export if kdbus is not active */ | if (access("/sys/fs/kdbus", F_OK) < 0) | return PAM_SUCCESS; | | if (asprintf(&s, KERNEL_USER_BUS_ADDRESS_FMT ";" UNIX_USER_BUS_ADDRESS_FMT, uid, runtime) < 0) { | pam_syslog(handle, LOG_ERR, "Failed to set bus variable."); | return PAM_BUF_ERR; | } The code is fine under both the old and new glibc behavior. It merely assumes that the pointer can be freed after a failed call, and that doesn't change. I no longer think we need to add a symbol version for this change. But if others think that we should do it, we can. (It's 14 new symbols on powerpc64le …) Thanks, Florian
On Mon, 29 Jul 2024, Florian Weimer wrote: > >> Reviewed-by: Sam James <sam@gentoo.org> > > > > Did you see the previous asprintf thread? > > > > https://inbox.sourceware.org/libc-alpha/CANSoFxt-cdc-+C4u-rTENMtY4X9RpRSuv+axDswSPxbDgag8_Q@mail.gmail.com/T/ > > Obviously I saw it at the time, but I have forgotten about it. Oh, how convenient. Obviously, my question was to Sam. > > It is quite disappointing that when a new contributor tries to make > > that change it is bogged down in discussions about symbol versioning > > and then successfully forgotten. > > Agreed. We try to avoid that nowadays. There's no 'try' in C. Also, not my impression from lurking here. > I no longer think we need to add a symbol version for this change. Then I think you should credit Archie Cobbs in your commit message. Alexander
On Mon, Jul 29, 2024 at 12:43:24PM +0200, Florian Weimer wrote: > And update the manual to mention that function is now part > of POSIX. > > 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> > > --- > v2: Commit message updated as suggested by Andreas Schwab. Florian, thanks for working on this issue. To be honest, I couldn't imagine back in 2001 that it would take so long to get the change in. Could you also add some references to the previous discussions, please? e.g. https://inbox.sourceware.org/libc-alpha/20011205185828.GA8376@ldv.office.alt-linux.org/T/#u https://inbox.sourceware.org/libc-alpha/CANSoFxt-cdc-+C4u-rTENMtY4X9RpRSuv+axDswSPxbDgag8_Q@mail.gmail.com/T/#u
Should a future revision of POSIX require that behaviour? According to man.freebsd.org the BSDs did that from the beginning.
On Mon, Jul 29, 2024 at 7:52 AM Florian Weimer <fweimer@redhat.com> wrote: > * Alexander Monakov: > > > On Mon, 29 Jul 2024, Sam James wrote: > > > >> Florian Weimer <fweimer@redhat.com> writes: > >> > >> > And update the manual to mention that function is now part > >> > of POSIX. > >> > >> I'd still prefer it as a separate commit given one part is obviously > >> fine to backport and one isn't. (Just to 2.40 would be nice because I > >> can see people filing a bug over the docs being stale.) > >> > >> LGTM modulo my preference to split. Sorry about the race condition. > >> > >> Reviewed-by: Sam James <sam@gentoo.org> > > > > Did you see the previous asprintf thread? > > > > > https://inbox.sourceware.org/libc-alpha/CANSoFxt-cdc-+C4u-rTENMtY4X9RpRSuv+axDswSPxbDgag8_Q@mail.gmail.com/T/ > > Obviously I saw it at the time, but I have forgotten about it. > > > It is quite disappointing that when a new contributor tries to make > > that change it is bogged down in discussions about symbol versioning > > and then successfully forgotten. > > Agreed. We try to avoid that nowadays. > > > Is the systemd issue referenced there irrelevant now? > > The systemd issue is this one: > > Bug 90017 - Improper use of asprintf() in login/pam_systemd.c > <https://bugs.freedesktop.org/show_bug.cgi?id=90017> > > | #ifdef ENABLE_KDBUS > | _cleanup_free_ char *s = NULL; > | int r; > | > | /* skip export if kdbus is not active */ > | if (access("/sys/fs/kdbus", F_OK) < 0) > | return PAM_SUCCESS; > | > | if (asprintf(&s, KERNEL_USER_BUS_ADDRESS_FMT ";" > UNIX_USER_BUS_ADDRESS_FMT, uid, runtime) < 0) { > | pam_syslog(handle, LOG_ERR, "Failed to set bus > variable."); > | return PAM_BUF_ERR; > | } > > > It does not matter anyways because kdbus left the building unfinished and never shipped into any distro.
On Mon, Jul 29, 2024 at 10:54 AM Andreas Schwab <schwab@suse.de> wrote: > Should a future revision of POSIX require that behaviour? According to > man.freebsd.org the BSDs did that from the beginning. > > > I think it is worth filing a posix bug then.. and this patch does not require new symbols, it has always worked that way elsewhere and previously it was unspecified what happened. We should IMHO avoid "unspecified" as much as possible.
* Sam James: > Florian Weimer <fweimer@redhat.com> writes: > >> * Sam James: >> >>> Florian Weimer <fweimer@redhat.com> writes: >>> >>>> And update the manual to mention that function is now part >>>> of POSIX. >>> >>> I'd still prefer it as a separate commit given one part is obviously >>> fine to backport and one isn't. (Just to 2.40 would be nice because I >>> can see people filing a bug over the docs being stale.) >> >> Sorry, which part is obviously fine to backport? > > The POSIX documentation change, given glibc-2.40 was released after > Issue 8 was ratified. Ahh, well, given that _POSIX_SOURCE etc. won't enable it, maybe even that part is controversial. I've submitted a v3 without this change. Thanks, Florian
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 f5e289d58a..0d63897b74 100644 --- a/manual/stdio.texi +++ b/manual/stdio.texi @@ -2516,7 +2516,7 @@ The functions in this section do formatted output and place the results in dynamically allocated memory. @deftypefun int asprintf (char **@var{ptr}, const char *@var{template}, @dots{}) -@standards{GNU, stdio.h} +@standards{POSIX, stdio.h} @safety{@prelim{}@mtsafe{@mtslocale{}}@asunsafe{@ascuheap{}}@acunsafe{@acsmem{}}} This function is similar to @code{sprintf}, except that it dynamically allocates a string (as with @code{malloc}; @pxref{Unconstrained @@ -2524,7 +2524,9 @@ 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 versions of @theglibc{} write a null pointer to +@samp{*@var{ptr}} upon failure, but this is not required by the +standard, and previous versions did not modify the pointer value. 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 @@ -2545,6 +2547,9 @@ make_message (char *name, char *value) return result; @} @end smallexample + +The @code{asprintf} function was a GNU extension initially, but is now +part of POSIX. @end deftypefun @deftypefun int obstack_printf (struct obstack *@var{obstack}, const char *@var{template}, @dots{})