Re: [PATCH v2] libc_fatal: Get rid of alloca

[Joe Simmons-Talbott <josimmon@redhat.com>]

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