| Message ID | 20230707175349.2096131-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | fxprintf: Get rid of alloca | expand |
On 07/07/23 14:53, Joe Simmons-Talbott via Libc-alpha wrote: > Use a scratch_buffer rather than alloca/malloc to avoid potential stack > overflow. LGTM, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > --- > stdio-common/fxprintf.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/stdio-common/fxprintf.c b/stdio-common/fxprintf.c > index f0ac9654ab..88501ab61f 100644 > --- a/stdio-common/fxprintf.c > +++ b/stdio-common/fxprintf.c > @@ -15,6 +15,7 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > +#include <scratch_buffer.h> > #include <stdarg.h> > #include <stdio.h> > #include <stdlib.h> > @@ -34,20 +35,18 @@ locked_vfxprintf (FILE *fp, const char *fmt, va_list ap, > wchar_t *wfmt; > mbstate_t mbstate; > int res; > - int used_malloc = 0; > size_t len = strlen (fmt) + 1; > + struct scratch_buffer buf; > + scratch_buffer_init (&buf); > > if (__glibc_unlikely (len > SIZE_MAX / sizeof (wchar_t))) > { > __set_errno (EOVERFLOW); > return -1; > } This check is redundant, but scratch_buffer_set_array_size would return ENOMEM in this case. I guess it should not change this for now. > - if (__libc_use_alloca (len * sizeof (wchar_t))) > - wfmt = alloca (len * sizeof (wchar_t)); > - else if ((wfmt = malloc (len * sizeof (wchar_t))) == NULL) > + if (!scratch_buffer_set_array_size (&buf, sizeof (wchar_t), len)) > return -1; > - else > - used_malloc = 1; > + wfmt = buf.data; > > memset (&mbstate, 0, sizeof mbstate); > res = __mbsrtowcs (wfmt, &fmt, len, &mbstate); > @@ -55,8 +54,7 @@ locked_vfxprintf (FILE *fp, const char *fmt, va_list ap, > if (res != -1) > res = __vfwprintf_internal (fp, wfmt, ap, mode_flags); > > - if (used_malloc) > - free (wfmt); > + scratch_buffer_free (&buf); > > return res; > }
On Wed, Jul 19, 2023 at 03:12:43PM -0300, Adhemerval Zanella Netto wrote: > > > On 07/07/23 14:53, Joe Simmons-Talbott via Libc-alpha wrote: > > Use a scratch_buffer rather than alloca/malloc to avoid potential stack > > overflow. > > LGTM, thanks. > > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> Should I apply this despite the seemingly unrelated test timeout[1] in CI? [1] https://patchwork.sourceware.org/project/glibc/patch/20230707175349.2096131-1-josimmon@redhat.com/ Thanks, Joe > > > --- > > stdio-common/fxprintf.c | 14 ++++++-------- > > 1 file changed, 6 insertions(+), 8 deletions(-) > > > > diff --git a/stdio-common/fxprintf.c b/stdio-common/fxprintf.c > > index f0ac9654ab..88501ab61f 100644 > > --- a/stdio-common/fxprintf.c > > +++ b/stdio-common/fxprintf.c > > @@ -15,6 +15,7 @@ > > License along with the GNU C Library; if not, see > > <https://www.gnu.org/licenses/>. */ > > > > +#include <scratch_buffer.h> > > #include <stdarg.h> > > #include <stdio.h> > > #include <stdlib.h> > > @@ -34,20 +35,18 @@ locked_vfxprintf (FILE *fp, const char *fmt, va_list ap, > > wchar_t *wfmt; > > mbstate_t mbstate; > > int res; > > - int used_malloc = 0; > > size_t len = strlen (fmt) + 1; > > + struct scratch_buffer buf; > > + scratch_buffer_init (&buf); > > > > if (__glibc_unlikely (len > SIZE_MAX / sizeof (wchar_t))) > > { > > __set_errno (EOVERFLOW); > > return -1; > > } > > This check is redundant, but scratch_buffer_set_array_size would return > ENOMEM in this case. I guess it should not change this for now. > > > - if (__libc_use_alloca (len * sizeof (wchar_t))) > > - wfmt = alloca (len * sizeof (wchar_t)); > > - else if ((wfmt = malloc (len * sizeof (wchar_t))) == NULL) > > + if (!scratch_buffer_set_array_size (&buf, sizeof (wchar_t), len)) > > return -1; > > - else > > - used_malloc = 1; > > + wfmt = buf.data; > > > > memset (&mbstate, 0, sizeof mbstate); > > res = __mbsrtowcs (wfmt, &fmt, len, &mbstate); > > @@ -55,8 +54,7 @@ locked_vfxprintf (FILE *fp, const char *fmt, va_list ap, > > if (res != -1) > > res = __vfwprintf_internal (fp, wfmt, ap, mode_flags); > > > > - if (used_malloc) > > - free (wfmt); > > + scratch_buffer_free (&buf); > > > > return res; > > } >
On 10/08/23 10:34, Joe Simmons-Talbott wrote: > On Wed, Jul 19, 2023 at 03:12:43PM -0300, Adhemerval Zanella Netto wrote: >> >> >> On 07/07/23 14:53, Joe Simmons-Talbott via Libc-alpha wrote: >>> Use a scratch_buffer rather than alloca/malloc to avoid potential stack >>> overflow. >> >> LGTM, thanks. >> >> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> > > Should I apply this despite the seemingly unrelated test timeout[1] in > CI? > > [1] > https://patchwork.sourceware.org/project/glibc/patch/20230707175349.2096131-1-josimmon@redhat.com/ Yes, I think this failure is transient.
diff --git a/stdio-common/fxprintf.c b/stdio-common/fxprintf.c index f0ac9654ab..88501ab61f 100644 --- a/stdio-common/fxprintf.c +++ b/stdio-common/fxprintf.c @@ -15,6 +15,7 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ +#include <scratch_buffer.h> #include <stdarg.h> #include <stdio.h> #include <stdlib.h> @@ -34,20 +35,18 @@ locked_vfxprintf (FILE *fp, const char *fmt, va_list ap, wchar_t *wfmt; mbstate_t mbstate; int res; - int used_malloc = 0; size_t len = strlen (fmt) + 1; + struct scratch_buffer buf; + scratch_buffer_init (&buf); if (__glibc_unlikely (len > SIZE_MAX / sizeof (wchar_t))) { __set_errno (EOVERFLOW); return -1; } - if (__libc_use_alloca (len * sizeof (wchar_t))) - wfmt = alloca (len * sizeof (wchar_t)); - else if ((wfmt = malloc (len * sizeof (wchar_t))) == NULL) + if (!scratch_buffer_set_array_size (&buf, sizeof (wchar_t), len)) return -1; - else - used_malloc = 1; + wfmt = buf.data; memset (&mbstate, 0, sizeof mbstate); res = __mbsrtowcs (wfmt, &fmt, len, &mbstate); @@ -55,8 +54,7 @@ locked_vfxprintf (FILE *fp, const char *fmt, va_list ap, if (res != -1) res = __vfwprintf_internal (fp, wfmt, ap, mode_flags); - if (used_malloc) - free (wfmt); + scratch_buffer_free (&buf); return res; }