* [PATCH v2] libc_fatal: Get rid of alloca @ 2023-08-31 20:20 Joe Simmons-Talbott 2023-09-01 14:23 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 6+ messages in thread From: Joe Simmons-Talbott @ 2023-08-31 20:20 UTC (permalink / raw) To: libc-alpha; +Cc: Joe Simmons-Talbott 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(-) 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 (); } -- 2.39.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libc_fatal: Get rid of alloca 2023-08-31 20:20 [PATCH v2] libc_fatal: Get rid of alloca Joe Simmons-Talbott @ 2023-09-01 14:23 ` Adhemerval Zanella Netto 2023-09-06 15:43 ` Joe Simmons-Talbott 0 siblings, 1 reply; 6+ messages in thread From: Adhemerval Zanella Netto @ 2023-09-01 14:23 UTC (permalink / raw) To: Joe Simmons-Talbott, libc-alpha 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 (); } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libc_fatal: Get rid of alloca 2023-09-01 14:23 ` Adhemerval Zanella Netto @ 2023-09-06 15:43 ` Joe Simmons-Talbott 2023-09-06 16:51 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 6+ messages in thread From: Joe Simmons-Talbott @ 2023-09-06 15:43 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libc_fatal: Get rid of alloca 2023-09-06 15:43 ` Joe Simmons-Talbott @ 2023-09-06 16:51 ` Adhemerval Zanella Netto 2023-09-06 18:45 ` Joe Simmons-Talbott 0 siblings, 1 reply; 6+ messages in thread From: Adhemerval Zanella Netto @ 2023-09-06 16:51 UTC (permalink / raw) To: Joe Simmons-Talbott; +Cc: libc-alpha 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libc_fatal: Get rid of alloca 2023-09-06 16:51 ` Adhemerval Zanella Netto @ 2023-09-06 18:45 ` Joe Simmons-Talbott 2023-09-06 19:39 ` Joe Simmons-Talbott 0 siblings, 1 reply; 6+ messages in thread From: Joe Simmons-Talbott @ 2023-09-06 18:45 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] libc_fatal: Get rid of alloca 2023-09-06 18:45 ` Joe Simmons-Talbott @ 2023-09-06 19:39 ` Joe Simmons-Talbott 0 siblings, 0 replies; 6+ messages in thread From: Joe Simmons-Talbott @ 2023-09-06 19:39 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-06 19:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-31 20:20 [PATCH v2] libc_fatal: Get rid of alloca Joe Simmons-Talbott 2023-09-01 14:23 ` Adhemerval Zanella Netto 2023-09-06 15:43 ` Joe Simmons-Talbott 2023-09-06 16:51 ` Adhemerval Zanella Netto 2023-09-06 18:45 ` Joe Simmons-Talbott 2023-09-06 19:39 ` Joe Simmons-Talbott
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).