[v2] libc_fatal: Get rid of alloca

Message ID 20230831202122.2239619-1-josimmon@redhat.com
State Superseded
Headers
Series [v2] libc_fatal: Get rid of alloca |

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-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed

Commit Message

Joe Simmons-Talbott Aug. 31, 2023, 8:20 p.m. UTC
  Use fixed size arrays in place of alloca to avoid potential stack overflow.
Limit the number of varargs to __libc_message to 10.
---
Changes to v1:
 * Use a fixed size array rather than scratch_buffers since we can only
   call async signal safe functions.

 sysdeps/posix/libc_fatal.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
  

Comments

Adhemerval Zanella Netto Sept. 1, 2023, 2:23 p.m. UTC | #1
On 31/08/23 17:20, Joe Simmons-Talbott via Libc-alpha wrote:
> Use fixed size arrays in place of alloca to avoid potential stack overflow.
> Limit the number of varargs to __libc_message to 10.

I think we enforce the maximum number of arguments internally with some
macro tricks, so there is no need to bail out to abort without printing
the message.

> ---
> Changes to v1:
>  * Use a fixed size array rather than scratch_buffers since we can only
>    call async signal safe functions.
> 
>  sysdeps/posix/libc_fatal.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> index 70edcc10c1..16929addab 100644
> --- a/sysdeps/posix/libc_fatal.c
> +++ b/sysdeps/posix/libc_fatal.c
> @@ -45,6 +45,9 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
>  }
>  #endif
>  
> +/* The maximum number of varargs allowed in a __libc_message format string */
> +#define MAX_NLIST 10
> +
>  struct str_list
>  {
>    const char *str;
> @@ -58,6 +61,7 @@ __libc_message (const char *fmt, ...)
>  {
>    va_list ap;
>    int fd = -1;
> +  struct str_list _newp[MAX_NLIST];

There are no need to track the string list, it is used essentially to
construct the iovec struct to call writev.  You can construct the
iovec directly.

>  
>    va_start (ap, fmt);
>  
> @@ -70,6 +74,7 @@ __libc_message (const char *fmt, ...)
>  
>    struct str_list *list = NULL;
>    int nlist = 0;
> +  struct iovec iov[MAX_NLIST];
>  
>    const char *cp = fmt;
>    while (*cp != '\0')
> @@ -100,17 +105,18 @@ __libc_message (const char *fmt, ...)
>  	  cp = next;
>  	}
>  
> -      struct str_list *newp = alloca (sizeof (struct str_list));
> +      struct str_list *newp = &_newp[nlist];
>        newp->str = str;
>        newp->len = len;
>        newp->next = list;
>        list = newp;
>        ++nlist;
> +      if (nlist > MAX_NLIST)
> +        goto fail_out;
>      }
>  
>    if (nlist > 0)
>      {
> -      struct iovec *iov = alloca (nlist * sizeof (struct iovec));
>        ssize_t total = 0;
>  
>        for (int cnt = nlist - 1; cnt >= 0; --cnt)
> @@ -146,6 +152,7 @@ __libc_message (const char *fmt, ...)
>  
>    va_end (ap);
>  
> +fail_out:
>    /* Kill the application.  */
>    abort ();
>  }

Below is a patch on top of your which enforces the maximum number of 
supported variadic arguments with a similar trick I used for 
INLINE_SYSCALL_CALL.  One will need to explicit implement a new macro
for each __libc_message usage with a number of arguments larger than
LIBC_MESSAGE_MAX_ARGS, but it will fail at build time if you try to 
use __libc_message with 5 or more arguments.

I think this is best we can do without a compiler attribute to help us 
to enforce it.

--

diff --git a/include/stdio.h b/include/stdio.h
index 6755877911..e2100066ed 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -172,10 +172,36 @@ extern int __gen_tempname (char *__tmpl, int __suffixlen, int __flags,
    and abort.  */
 extern void __libc_fatal (const char *__message)
      __attribute__ ((__noreturn__));
-_Noreturn void __libc_message (const char *__fnt, ...) attribute_hidden;
 extern void __fortify_fail (const char *msg) __attribute__ ((__noreturn__));
 libc_hidden_proto (__fortify_fail)
 
+/* The maximum number of varargs allowed in a __libc_message format string */
+#define LIBC_MESSAGE_MAX_ARGS 4
+
+_Noreturn void __libc_message_impl (const char *__fnt, ...) attribute_hidden;
+
+#define __libc_message0(fmt) \
+   __libc_message_impl (fmt)
+#define __libc_message1(fmt, a1) \
+   __libc_message_impl (fmt, a1)
+#define __libc_message2(fmt, a1, a2) \
+   __libc_message_impl (fmt, a1, a2)
+#define __libc_message3(fmt, a1, a2, a3) \
+   __libc_message_impl (fmt, a1, a2, a3)
+#define __libc_message4(fmt, a1, a2, a3, a4) \
+   __libc_message_impl (fmt, a1, a2, a3, a4)
+
+#define __libc_message_concat_x(a,b)  a##b
+#define __libc_message_concat(a,b)    __libc_message_concat_x (a, b)
+
+#define __libc_message_nargs_x(a0,a1,a2,a3,a4,a5,a6,...) a6
+#define __libc_message_nargs(b, ...) \
+   __libc_message_nargs_x (__VA_ARGS__,6,5,4,3,2,1,0,)
+#define __libc_message_disp(b, ...) \
+   __libc_message_concat (b, __libc_message_nargs (__VA_ARGS__))(__VA_ARGS__)
+#define __libc_message(...) \
+   __libc_message_disp (__libc_message, __VA_ARGS__)
+
 /* Acquire ownership of STREAM.  */
 extern void __flockfile (FILE *__stream) attribute_hidden;
 
diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
index 16929addab..0816dd7752 100644
--- a/sysdeps/posix/libc_fatal.c
+++ b/sysdeps/posix/libc_fatal.c
@@ -45,25 +45,12 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
 }
 #endif
 
-/* The maximum number of varargs allowed in a __libc_message format string */
-#define MAX_NLIST 10
-
-struct str_list
-{
-  const char *str;
-  size_t len;
-  struct str_list *next;
-};
-
 /* Abort with an error message.  */
 void
-__libc_message (const char *fmt, ...)
+__libc_message_impl (const char *fmt, ...)
 {
   va_list ap;
   int fd = -1;
-  struct str_list _newp[MAX_NLIST];
-
-  va_start (ap, fmt);
 
 #ifdef FATAL_PREPARE
   FATAL_PREPARE;
@@ -72,10 +59,11 @@ __libc_message (const char *fmt, ...)
   if (fd == -1)
     fd = STDERR_FILENO;
 
-  struct str_list *list = NULL;
-  int nlist = 0;
-  struct iovec iov[MAX_NLIST];
+  struct iovec iov[LIBC_MESSAGE_MAX_ARGS];
+  int iovcnt = 0;
+  ssize_t total = 0;
 
+  va_start (ap, fmt);
   const char *cp = fmt;
   while (*cp != '\0')
     {
@@ -105,29 +93,16 @@ __libc_message (const char *fmt, ...)
 	  cp = next;
 	}
 
-      struct str_list *newp = &_newp[nlist];
-      newp->str = str;
-      newp->len = len;
-      newp->next = list;
-      list = newp;
-      ++nlist;
-      if (nlist > MAX_NLIST)
-        goto fail_out;
+      iov[iovcnt].iov_base = (char *) str;
+      iov[iovcnt].iov_len = len;
+      total += len;
+      iovcnt++;
     }
+  va_end (ap);
 
-  if (nlist > 0)
+  if (iovcnt > 0)
     {
-      ssize_t total = 0;
-
-      for (int cnt = nlist - 1; cnt >= 0; --cnt)
-	{
-	  iov[cnt].iov_base = (char *) list->str;
-	  iov[cnt].iov_len = list->len;
-	  total += list->len;
-	  list = list->next;
-	}
-
-      WRITEV_FOR_FATAL (fd, iov, nlist, total);
+      WRITEV_FOR_FATAL (fd, iov, iovcnt, total);
 
       total = (total + 1 + GLRO(dl_pagesize) - 1) & ~(GLRO(dl_pagesize) - 1);
       struct abort_msg_s *buf = __mmap (NULL, total,
@@ -137,7 +112,7 @@ __libc_message (const char *fmt, ...)
 	{
 	  buf->size = total;
 	  char *wp = buf->msg;
-	  for (int cnt = 0; cnt < nlist; ++cnt)
+	  for (int cnt = 0; cnt < iovcnt; ++cnt)
 	    wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len);
 	  *wp = '\0';
 
@@ -150,9 +125,6 @@ __libc_message (const char *fmt, ...)
 	}
     }
 
-  va_end (ap);
-
-fail_out:
   /* Kill the application.  */
   abort ();
 }
  
Joe Simmons-Talbott Sept. 6, 2023, 3:43 p.m. UTC | #2
On Fri, Sep 01, 2023 at 11:23:08AM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 31/08/23 17:20, Joe Simmons-Talbott via Libc-alpha wrote:
> > Use fixed size arrays in place of alloca to avoid potential stack overflow.
> > Limit the number of varargs to __libc_message to 10.
> 
> I think we enforce the maximum number of arguments internally with some
> macro tricks, so there is no need to bail out to abort without printing
> the message.
> 
> > ---
> > Changes to v1:
> >  * Use a fixed size array rather than scratch_buffers since we can only
> >    call async signal safe functions.
> > 
> >  sysdeps/posix/libc_fatal.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> > index 70edcc10c1..16929addab 100644
> > --- a/sysdeps/posix/libc_fatal.c
> > +++ b/sysdeps/posix/libc_fatal.c
> > @@ -45,6 +45,9 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
> >  }
> >  #endif
> >  
> > +/* The maximum number of varargs allowed in a __libc_message format string */
> > +#define MAX_NLIST 10
> > +
> >  struct str_list
> >  {
> >    const char *str;
> > @@ -58,6 +61,7 @@ __libc_message (const char *fmt, ...)
> >  {
> >    va_list ap;
> >    int fd = -1;
> > +  struct str_list _newp[MAX_NLIST];
> 
> There are no need to track the string list, it is used essentially to
> construct the iovec struct to call writev.  You can construct the
> iovec directly.
> 
> >  
> >    va_start (ap, fmt);
> >  
> > @@ -70,6 +74,7 @@ __libc_message (const char *fmt, ...)
> >  
> >    struct str_list *list = NULL;
> >    int nlist = 0;
> > +  struct iovec iov[MAX_NLIST];
> >  
> >    const char *cp = fmt;
> >    while (*cp != '\0')
> > @@ -100,17 +105,18 @@ __libc_message (const char *fmt, ...)
> >  	  cp = next;
> >  	}
> >  
> > -      struct str_list *newp = alloca (sizeof (struct str_list));
> > +      struct str_list *newp = &_newp[nlist];
> >        newp->str = str;
> >        newp->len = len;
> >        newp->next = list;
> >        list = newp;
> >        ++nlist;
> > +      if (nlist > MAX_NLIST)
> > +        goto fail_out;
> >      }
> >  
> >    if (nlist > 0)
> >      {
> > -      struct iovec *iov = alloca (nlist * sizeof (struct iovec));
> >        ssize_t total = 0;
> >  
> >        for (int cnt = nlist - 1; cnt >= 0; --cnt)
> > @@ -146,6 +152,7 @@ __libc_message (const char *fmt, ...)
> >  
> >    va_end (ap);
> >  
> > +fail_out:
> >    /* Kill the application.  */
> >    abort ();
> >  }
> 
> Below is a patch on top of your which enforces the maximum number of 
> supported variadic arguments with a similar trick I used for 
> INLINE_SYSCALL_CALL.  One will need to explicit implement a new macro
> for each __libc_message usage with a number of arguments larger than
> LIBC_MESSAGE_MAX_ARGS, but it will fail at build time if you try to 
> use __libc_message with 5 or more arguments.

Thanks for the patch.  I applied it and during testing see failures for
stdlib/tst-bz20544 with the following output:

<<<
Did not find expected string in error output:
  expected: >>>assertion failed: func != NULL
<<<
  actual:   >>>Fatal glibc error: on_exit.c:31 (__on_exit): assertion failed: ): assertion failed: %s

<<<
Did not find expected string in error output:
  expected: >>>assertion failed: func != NULL
<<<
  actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s

<<<
Did not find expected string in error output:
  expected: >>>assertion failed: func != NULL
<<<
  actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s

<<<

Thanks,
Joe
  
Adhemerval Zanella Netto Sept. 6, 2023, 4:51 p.m. UTC | #3
On 06/09/23 12:43, Joe Simmons-Talbott wrote:
> On Fri, Sep 01, 2023 at 11:23:08AM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>> On 31/08/23 17:20, Joe Simmons-Talbott via Libc-alpha wrote:
>>> Use fixed size arrays in place of alloca to avoid potential stack overflow.
>>> Limit the number of varargs to __libc_message to 10.
>>
>> I think we enforce the maximum number of arguments internally with some
>> macro tricks, so there is no need to bail out to abort without printing
>> the message.
>>
>>> ---
>>> Changes to v1:
>>>  * Use a fixed size array rather than scratch_buffers since we can only
>>>    call async signal safe functions.
>>>
>>>  sysdeps/posix/libc_fatal.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
>>> index 70edcc10c1..16929addab 100644
>>> --- a/sysdeps/posix/libc_fatal.c
>>> +++ b/sysdeps/posix/libc_fatal.c
>>> @@ -45,6 +45,9 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
>>>  }
>>>  #endif
>>>  
>>> +/* The maximum number of varargs allowed in a __libc_message format string */
>>> +#define MAX_NLIST 10
>>> +
>>>  struct str_list
>>>  {
>>>    const char *str;
>>> @@ -58,6 +61,7 @@ __libc_message (const char *fmt, ...)
>>>  {
>>>    va_list ap;
>>>    int fd = -1;
>>> +  struct str_list _newp[MAX_NLIST];
>>
>> There are no need to track the string list, it is used essentially to
>> construct the iovec struct to call writev.  You can construct the
>> iovec directly.
>>
>>>  
>>>    va_start (ap, fmt);
>>>  
>>> @@ -70,6 +74,7 @@ __libc_message (const char *fmt, ...)
>>>  
>>>    struct str_list *list = NULL;
>>>    int nlist = 0;
>>> +  struct iovec iov[MAX_NLIST];
>>>  
>>>    const char *cp = fmt;
>>>    while (*cp != '\0')
>>> @@ -100,17 +105,18 @@ __libc_message (const char *fmt, ...)
>>>  	  cp = next;
>>>  	}
>>>  
>>> -      struct str_list *newp = alloca (sizeof (struct str_list));
>>> +      struct str_list *newp = &_newp[nlist];
>>>        newp->str = str;
>>>        newp->len = len;
>>>        newp->next = list;
>>>        list = newp;
>>>        ++nlist;
>>> +      if (nlist > MAX_NLIST)
>>> +        goto fail_out;
>>>      }
>>>  
>>>    if (nlist > 0)
>>>      {
>>> -      struct iovec *iov = alloca (nlist * sizeof (struct iovec));
>>>        ssize_t total = 0;
>>>  
>>>        for (int cnt = nlist - 1; cnt >= 0; --cnt)
>>> @@ -146,6 +152,7 @@ __libc_message (const char *fmt, ...)
>>>  
>>>    va_end (ap);
>>>  
>>> +fail_out:
>>>    /* Kill the application.  */
>>>    abort ();
>>>  }
>>
>> Below is a patch on top of your which enforces the maximum number of 
>> supported variadic arguments with a similar trick I used for 
>> INLINE_SYSCALL_CALL.  One will need to explicit implement a new macro
>> for each __libc_message usage with a number of arguments larger than
>> LIBC_MESSAGE_MAX_ARGS, but it will fail at build time if you try to 
>> use __libc_message with 5 or more arguments.
> 
> Thanks for the patch.  I applied it and during testing see failures for
> stdlib/tst-bz20544 with the following output:
> 
> <<<
> Did not find expected string in error output:
>   expected: >>>assertion failed: func != NULL
> <<<
>   actual:   >>>Fatal glibc error: on_exit.c:31 (__on_exit): assertion failed: ): assertion failed: %s
> 
> <<<
> Did not find expected string in error output:
>   expected: >>>assertion failed: func != NULL
> <<<
>   actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s
> 
> <<<
> Did not find expected string in error output:
>   expected: >>>assertion failed: func != NULL
> <<<
>   actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s
> 
> <<<

That is unexpected, I have not see any regression testing here.  Could
you check a clean build with a branch I have pushed on sourceware [1]?

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=742b35228f3efa25d41d14f27c8911f308514b28
  
Joe Simmons-Talbott Sept. 6, 2023, 6:45 p.m. UTC | #4
On Wed, Sep 06, 2023 at 01:51:12PM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 06/09/23 12:43, Joe Simmons-Talbott wrote:
> > On Fri, Sep 01, 2023 at 11:23:08AM -0300, Adhemerval Zanella Netto wrote:
> >>
> >>
> >> On 31/08/23 17:20, Joe Simmons-Talbott via Libc-alpha wrote:
> >>> Use fixed size arrays in place of alloca to avoid potential stack overflow.
> >>> Limit the number of varargs to __libc_message to 10.
> >>
> >> I think we enforce the maximum number of arguments internally with some
> >> macro tricks, so there is no need to bail out to abort without printing
> >> the message.
> >>
> >>> ---
> >>> Changes to v1:
> >>>  * Use a fixed size array rather than scratch_buffers since we can only
> >>>    call async signal safe functions.
> >>>
> >>>  sysdeps/posix/libc_fatal.c | 11 +++++++++--
> >>>  1 file changed, 9 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> >>> index 70edcc10c1..16929addab 100644
> >>> --- a/sysdeps/posix/libc_fatal.c
> >>> +++ b/sysdeps/posix/libc_fatal.c
> >>> @@ -45,6 +45,9 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
> >>>  }
> >>>  #endif
> >>>  
> >>> +/* The maximum number of varargs allowed in a __libc_message format string */
> >>> +#define MAX_NLIST 10
> >>> +
> >>>  struct str_list
> >>>  {
> >>>    const char *str;
> >>> @@ -58,6 +61,7 @@ __libc_message (const char *fmt, ...)
> >>>  {
> >>>    va_list ap;
> >>>    int fd = -1;
> >>> +  struct str_list _newp[MAX_NLIST];
> >>
> >> There are no need to track the string list, it is used essentially to
> >> construct the iovec struct to call writev.  You can construct the
> >> iovec directly.
> >>
> >>>  
> >>>    va_start (ap, fmt);
> >>>  
> >>> @@ -70,6 +74,7 @@ __libc_message (const char *fmt, ...)
> >>>  
> >>>    struct str_list *list = NULL;
> >>>    int nlist = 0;
> >>> +  struct iovec iov[MAX_NLIST];
> >>>  
> >>>    const char *cp = fmt;
> >>>    while (*cp != '\0')
> >>> @@ -100,17 +105,18 @@ __libc_message (const char *fmt, ...)
> >>>  	  cp = next;
> >>>  	}
> >>>  
> >>> -      struct str_list *newp = alloca (sizeof (struct str_list));
> >>> +      struct str_list *newp = &_newp[nlist];
> >>>        newp->str = str;
> >>>        newp->len = len;
> >>>        newp->next = list;
> >>>        list = newp;
> >>>        ++nlist;
> >>> +      if (nlist > MAX_NLIST)
> >>> +        goto fail_out;
> >>>      }
> >>>  
> >>>    if (nlist > 0)
> >>>      {
> >>> -      struct iovec *iov = alloca (nlist * sizeof (struct iovec));
> >>>        ssize_t total = 0;
> >>>  
> >>>        for (int cnt = nlist - 1; cnt >= 0; --cnt)
> >>> @@ -146,6 +152,7 @@ __libc_message (const char *fmt, ...)
> >>>  
> >>>    va_end (ap);
> >>>  
> >>> +fail_out:
> >>>    /* Kill the application.  */
> >>>    abort ();
> >>>  }
> >>
> >> Below is a patch on top of your which enforces the maximum number of 
> >> supported variadic arguments with a similar trick I used for 
> >> INLINE_SYSCALL_CALL.  One will need to explicit implement a new macro
> >> for each __libc_message usage with a number of arguments larger than
> >> LIBC_MESSAGE_MAX_ARGS, but it will fail at build time if you try to 
> >> use __libc_message with 5 or more arguments.
> > 
> > Thanks for the patch.  I applied it and during testing see failures for
> > stdlib/tst-bz20544 with the following output:
> > 
> > <<<
> > Did not find expected string in error output:
> >   expected: >>>assertion failed: func != NULL
> > <<<
> >   actual:   >>>Fatal glibc error: on_exit.c:31 (__on_exit): assertion failed: ): assertion failed: %s
> > 
> > <<<
> > Did not find expected string in error output:
> >   expected: >>>assertion failed: func != NULL
> > <<<
> >   actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s
> > 
> > <<<
> > Did not find expected string in error output:
> >   expected: >>>assertion failed: func != NULL
> > <<<
> >   actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s
> > 
> > <<<
> 
> That is unexpected, I have not see any regression testing here.  Could
> you check a clean build with a branch I have pushed on sourceware [1]?
> 
> [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=742b35228f3efa25d41d14f27c8911f308514b28
> 

I saw the same error with your branch on a clean build.  I think the
issue is that iov needs to have space for the parts of the format
string that are not varargs too.  I replaced it with:

struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 - 1];

and that fixed the errors.

Thanks,
Joe
  
Joe Simmons-Talbott Sept. 6, 2023, 7:39 p.m. UTC | #5
On Wed, Sep 06, 2023 at 02:45:50PM -0400, Joe Simmons-Talbott wrote:
> On Wed, Sep 06, 2023 at 01:51:12PM -0300, Adhemerval Zanella Netto wrote:
> > 
> > 
> > On 06/09/23 12:43, Joe Simmons-Talbott wrote:
> > > On Fri, Sep 01, 2023 at 11:23:08AM -0300, Adhemerval Zanella Netto wrote:
> > >>
> > >>
> > >> On 31/08/23 17:20, Joe Simmons-Talbott via Libc-alpha wrote:
> > >>> Use fixed size arrays in place of alloca to avoid potential stack overflow.
> > >>> Limit the number of varargs to __libc_message to 10.
> > >>
> > >> I think we enforce the maximum number of arguments internally with some
> > >> macro tricks, so there is no need to bail out to abort without printing
> > >> the message.
> > >>
> > >>> ---
> > >>> Changes to v1:
> > >>>  * Use a fixed size array rather than scratch_buffers since we can only
> > >>>    call async signal safe functions.
> > >>>
> > >>>  sysdeps/posix/libc_fatal.c | 11 +++++++++--
> > >>>  1 file changed, 9 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> > >>> index 70edcc10c1..16929addab 100644
> > >>> --- a/sysdeps/posix/libc_fatal.c
> > >>> +++ b/sysdeps/posix/libc_fatal.c
> > >>> @@ -45,6 +45,9 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
> > >>>  }
> > >>>  #endif
> > >>>  
> > >>> +/* The maximum number of varargs allowed in a __libc_message format string */
> > >>> +#define MAX_NLIST 10
> > >>> +
> > >>>  struct str_list
> > >>>  {
> > >>>    const char *str;
> > >>> @@ -58,6 +61,7 @@ __libc_message (const char *fmt, ...)
> > >>>  {
> > >>>    va_list ap;
> > >>>    int fd = -1;
> > >>> +  struct str_list _newp[MAX_NLIST];
> > >>
> > >> There are no need to track the string list, it is used essentially to
> > >> construct the iovec struct to call writev.  You can construct the
> > >> iovec directly.
> > >>
> > >>>  
> > >>>    va_start (ap, fmt);
> > >>>  
> > >>> @@ -70,6 +74,7 @@ __libc_message (const char *fmt, ...)
> > >>>  
> > >>>    struct str_list *list = NULL;
> > >>>    int nlist = 0;
> > >>> +  struct iovec iov[MAX_NLIST];
> > >>>  
> > >>>    const char *cp = fmt;
> > >>>    while (*cp != '\0')
> > >>> @@ -100,17 +105,18 @@ __libc_message (const char *fmt, ...)
> > >>>  	  cp = next;
> > >>>  	}
> > >>>  
> > >>> -      struct str_list *newp = alloca (sizeof (struct str_list));
> > >>> +      struct str_list *newp = &_newp[nlist];
> > >>>        newp->str = str;
> > >>>        newp->len = len;
> > >>>        newp->next = list;
> > >>>        list = newp;
> > >>>        ++nlist;
> > >>> +      if (nlist > MAX_NLIST)
> > >>> +        goto fail_out;
> > >>>      }
> > >>>  
> > >>>    if (nlist > 0)
> > >>>      {
> > >>> -      struct iovec *iov = alloca (nlist * sizeof (struct iovec));
> > >>>        ssize_t total = 0;
> > >>>  
> > >>>        for (int cnt = nlist - 1; cnt >= 0; --cnt)
> > >>> @@ -146,6 +152,7 @@ __libc_message (const char *fmt, ...)
> > >>>  
> > >>>    va_end (ap);
> > >>>  
> > >>> +fail_out:
> > >>>    /* Kill the application.  */
> > >>>    abort ();
> > >>>  }
> > >>
> > >> Below is a patch on top of your which enforces the maximum number of 
> > >> supported variadic arguments with a similar trick I used for 
> > >> INLINE_SYSCALL_CALL.  One will need to explicit implement a new macro
> > >> for each __libc_message usage with a number of arguments larger than
> > >> LIBC_MESSAGE_MAX_ARGS, but it will fail at build time if you try to 
> > >> use __libc_message with 5 or more arguments.
> > > 
> > > Thanks for the patch.  I applied it and during testing see failures for
> > > stdlib/tst-bz20544 with the following output:
> > > 
> > > <<<
> > > Did not find expected string in error output:
> > >   expected: >>>assertion failed: func != NULL
> > > <<<
> > >   actual:   >>>Fatal glibc error: on_exit.c:31 (__on_exit): assertion failed: ): assertion failed: %s
> > > 
> > > <<<
> > > Did not find expected string in error output:
> > >   expected: >>>assertion failed: func != NULL
> > > <<<
> > >   actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s
> > > 
> > > <<<
> > > Did not find expected string in error output:
> > >   expected: >>>assertion failed: func != NULL
> > > <<<
> > >   actual:   >>>Fatal glibc error: cxa_atexit.c:41 (__internal_atexit): assertion failed: ): assertion failed: %s
> > > 
> > > <<<
> > 
> > That is unexpected, I have not see any regression testing here.  Could
> > you check a clean build with a branch I have pushed on sourceware [1]?
> > 
> > [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=742b35228f3efa25d41d14f27c8911f308514b28
> > 
> 
> I saw the same error with your branch on a clean build.  I think the
> issue is that iov needs to have space for the parts of the format
> string that are not varargs too.  I replaced it with:
> 
> struct iovec iov[LIBC_MESSAGE_MAX_ARGS * 2 - 1];
> 
> and that fixed the errors.
> 

I've posted the patch[1]

Thanks,
Joe

[1] https://sourceware.org/pipermail/libc-alpha/2023-September/151403.html
  

Patch

diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
index 70edcc10c1..16929addab 100644
--- a/sysdeps/posix/libc_fatal.c
+++ b/sysdeps/posix/libc_fatal.c
@@ -45,6 +45,9 @@  writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
 }
 #endif
 
+/* The maximum number of varargs allowed in a __libc_message format string */
+#define MAX_NLIST 10
+
 struct str_list
 {
   const char *str;
@@ -58,6 +61,7 @@  __libc_message (const char *fmt, ...)
 {
   va_list ap;
   int fd = -1;
+  struct str_list _newp[MAX_NLIST];
 
   va_start (ap, fmt);
 
@@ -70,6 +74,7 @@  __libc_message (const char *fmt, ...)
 
   struct str_list *list = NULL;
   int nlist = 0;
+  struct iovec iov[MAX_NLIST];
 
   const char *cp = fmt;
   while (*cp != '\0')
@@ -100,17 +105,18 @@  __libc_message (const char *fmt, ...)
 	  cp = next;
 	}
 
-      struct str_list *newp = alloca (sizeof (struct str_list));
+      struct str_list *newp = &_newp[nlist];
       newp->str = str;
       newp->len = len;
       newp->next = list;
       list = newp;
       ++nlist;
+      if (nlist > MAX_NLIST)
+        goto fail_out;
     }
 
   if (nlist > 0)
     {
-      struct iovec *iov = alloca (nlist * sizeof (struct iovec));
       ssize_t total = 0;
 
       for (int cnt = nlist - 1; cnt >= 0; --cnt)
@@ -146,6 +152,7 @@  __libc_message (const char *fmt, ...)
 
   va_end (ap);
 
+fail_out:
   /* Kill the application.  */
   abort ();
 }