public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] fxprintf: Get rid of alloca
@ 2023-07-07 17:53 Joe Simmons-Talbott
  2023-07-19 18:12 ` Adhemerval Zanella Netto
  0 siblings, 1 reply; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-07-07 17:53 UTC (permalink / raw)
  To: libc-alpha; +Cc: Joe Simmons-Talbott

Use a scratch_buffer rather than alloca/malloc to avoid potential stack
overflow.
---
 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;
     }
-  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;
 }
-- 
2.39.2


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fxprintf: Get rid of alloca
  2023-07-07 17:53 [PATCH] fxprintf: Get rid of alloca Joe Simmons-Talbott
@ 2023-07-19 18:12 ` Adhemerval Zanella Netto
  2023-08-10 13:34   ` Joe Simmons-Talbott
  0 siblings, 1 reply; 4+ messages in thread
From: Adhemerval Zanella Netto @ 2023-07-19 18:12 UTC (permalink / raw)
  To: Joe Simmons-Talbott, libc-alpha



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fxprintf: Get rid of alloca
  2023-07-19 18:12 ` Adhemerval Zanella Netto
@ 2023-08-10 13:34   ` Joe Simmons-Talbott
  2023-09-06 16:56     ` Adhemerval Zanella Netto
  0 siblings, 1 reply; 4+ messages in thread
From: Joe Simmons-Talbott @ 2023-08-10 13:34 UTC (permalink / raw)
  To: Adhemerval Zanella Netto; +Cc: libc-alpha

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] fxprintf: Get rid of alloca
  2023-08-10 13:34   ` Joe Simmons-Talbott
@ 2023-09-06 16:56     ` Adhemerval Zanella Netto
  0 siblings, 0 replies; 4+ messages in thread
From: Adhemerval Zanella Netto @ 2023-09-06 16:56 UTC (permalink / raw)
  To: Joe Simmons-Talbott; +Cc: libc-alpha



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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2023-09-06 16:56 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-07 17:53 [PATCH] fxprintf: Get rid of alloca Joe Simmons-Talbott
2023-07-19 18:12 ` Adhemerval Zanella Netto
2023-08-10 13:34   ` Joe Simmons-Talbott
2023-09-06 16:56     ` Adhemerval Zanella Netto

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).