Message ID | 20230831124953.2090492-1-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | libc_fatal: Get rid of alloca | expand |
On Aug 31 2023, Joe Simmons-Talbott via Libc-alpha wrote: > @@ -100,17 +107,30 @@ __libc_message (const char *fmt, ...) > cp = next; > } > > - struct str_list *newp = alloca (sizeof (struct str_list)); > + struct str_list *newp = _newp + newp_idx; > + if ((void *) newp > sbuf.data + sbuf.length) > + { > + if (!scratch_buffer_grow_preserve (&sbuf)) > + goto fail_out; > + _newp = sbuf.data; > + newp = _newp + newp_idx; > + } > newp->str = str; > newp->len = len; > newp->next = list; > list = newp; > ++nlist; > + ++newp_idx; It looks like newp_idx is always the same as nlist.
On Aug 31 2023, Joe Simmons-Talbott via Libc-alpha wrote: > NOTE: I'm not certain that this is the best way to handle allocation > failures by jumping straight to the abort and am open to other > suggestions. The problem with this is that __libc_message and __libc_fatal can be called in situations when malloc cannot be used. It should only use async signal safe functions.
On Thu, Aug 31, 2023 at 03:21:23PM +0200, Andreas Schwab wrote: > On Aug 31 2023, Joe Simmons-Talbott via Libc-alpha wrote: > > > NOTE: I'm not certain that this is the best way to handle allocation > > failures by jumping straight to the abort and am open to other > > suggestions. > > The problem with this is that __libc_message and __libc_fatal can be > called in situations when malloc cannot be used. It should only use > async signal safe functions. > I guess an option would be to use a fixed sized array with a judiciously selected size to store the structs. I think this would work since there are not a lot of calls to __libc_message and none with more than a few '%s'. Thoughts? Thanks, Joe
On Aug 31 2023, Joe Simmons-Talbott wrote: > I guess an option would be to use a fixed sized array with a judiciously > selected size to store the structs. I think this would work since there > are not a lot of calls to __libc_message and none with more than a few > '%s'. Thoughts? How does that differ from using alloca?
On Thu, Aug 31, 2023 at 04:00:34PM +0200, Andreas Schwab wrote: > On Aug 31 2023, Joe Simmons-Talbott wrote: > > > I guess an option would be to use a fixed sized array with a judiciously > > selected size to store the structs. I think this would work since there > > are not a lot of calls to __libc_message and none with more than a few > > '%s'. Thoughts? > > How does that differ from using alloca? I guess it really doesn't other than avoiding possible stack overflow which seems unlikely. Thanks, Joe
On Aug 31 2023, Joe Simmons-Talbott wrote: > I guess it really doesn't other than avoiding possible stack overflow > which seems unlikely. If alloca overflows then your fixed size buffer will overflow even faster.
On 31/08/23 11:04, Joe Simmons-Talbott via Libc-alpha wrote: > On Thu, Aug 31, 2023 at 04:00:34PM +0200, Andreas Schwab wrote: >> On Aug 31 2023, Joe Simmons-Talbott wrote: >> >>> I guess an option would be to use a fixed sized array with a judiciously >>> selected size to store the structs. I think this would work since there >>> are not a lot of calls to __libc_message and none with more than a few >>> '%s'. Thoughts? >> >> How does that differ from using alloca? > > I guess it really doesn't other than avoiding possible stack overflow > which seems unlikely. > > Thanks, > Joe > Maybe a better option would to evaluate each '%s' and call multiple writes instead of one single writev. You can maybe add support for %d, so we can simplify __libc_assert_fail to be tail call to __libc_message.
* Adhemerval Zanella Netto via Libc-alpha: > On 31/08/23 11:04, Joe Simmons-Talbott via Libc-alpha wrote: >> On Thu, Aug 31, 2023 at 04:00:34PM +0200, Andreas Schwab wrote: >>> On Aug 31 2023, Joe Simmons-Talbott wrote: >>> >>>> I guess an option would be to use a fixed sized array with a judiciously >>>> selected size to store the structs. I think this would work since there >>>> are not a lot of calls to __libc_message and none with more than a few >>>> '%s'. Thoughts? >>> >>> How does that differ from using alloca? >> >> I guess it really doesn't other than avoiding possible stack overflow >> which seems unlikely. >> >> Thanks, >> Joe >> > > Maybe a better option would to evaluate each '%s' and call multiple writes > instead of one single writev. You can maybe add support for %d, so we can > simplify __libc_assert_fail to be tail call to __libc_message. I think it should be a single write or writev, so that the message doesn't get split.
On 31/08/23 14:29, Florian Weimer wrote: > * Adhemerval Zanella Netto via Libc-alpha: > >> On 31/08/23 11:04, Joe Simmons-Talbott via Libc-alpha wrote: >>> On Thu, Aug 31, 2023 at 04:00:34PM +0200, Andreas Schwab wrote: >>>> On Aug 31 2023, Joe Simmons-Talbott wrote: >>>> >>>>> I guess an option would be to use a fixed sized array with a judiciously >>>>> selected size to store the structs. I think this would work since there >>>>> are not a lot of calls to __libc_message and none with more than a few >>>>> '%s'. Thoughts? >>>> >>>> How does that differ from using alloca? >>> >>> I guess it really doesn't other than avoiding possible stack overflow >>> which seems unlikely. >>> >>> Thanks, >>> Joe >>> >> >> Maybe a better option would to evaluate each '%s' and call multiple writes >> instead of one single writev. You can maybe add support for %d, so we can >> simplify __libc_assert_fail to be tail call to __libc_message. > > I think it should be a single write or writev, so that the message > doesn't get split. So either we continue to use alloca or limit the maximum number of varargs. Since it is an internal function, I think later should be feasible.
diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c index 70edcc10c1..f8e2df4157 100644 --- a/sysdeps/posix/libc_fatal.c +++ b/sysdeps/posix/libc_fatal.c @@ -21,6 +21,7 @@ #include <fcntl.h> #include <ldsodefs.h> #include <paths.h> +#include <scratch_buffer.h> #include <stdarg.h> #include <stdbool.h> #include <stdio.h> @@ -58,6 +59,10 @@ __libc_message (const char *fmt, ...) { va_list ap; int fd = -1; + struct scratch_buffer sbuf; + scratch_buffer_init (&sbuf); + int newp_idx = 0; + struct str_list *_newp = sbuf.data; va_start (ap, fmt); @@ -70,6 +75,8 @@ __libc_message (const char *fmt, ...) struct str_list *list = NULL; int nlist = 0; + struct scratch_buffer iovec_buf; + scratch_buffer_init (&iovec_buf); const char *cp = fmt; while (*cp != '\0') @@ -100,17 +107,30 @@ __libc_message (const char *fmt, ...) cp = next; } - struct str_list *newp = alloca (sizeof (struct str_list)); + struct str_list *newp = _newp + newp_idx; + if ((void *) newp > sbuf.data + sbuf.length) + { + if (!scratch_buffer_grow_preserve (&sbuf)) + goto fail_out; + _newp = sbuf.data; + newp = _newp + newp_idx; + } newp->str = str; newp->len = len; newp->next = list; list = newp; ++nlist; + ++newp_idx; } if (nlist > 0) { - struct iovec *iov = alloca (nlist * sizeof (struct iovec)); + if (!scratch_buffer_set_array_size (&iovec_buf, + nlist, sizeof (struct iovec))) + goto fail_out; + + struct iovec *iov = iovec_buf.data; + ssize_t total = 0; for (int cnt = nlist - 1; cnt >= 0; --cnt) @@ -146,6 +166,9 @@ __libc_message (const char *fmt, ...) va_end (ap); +fail_out: + scratch_buffer_free (&sbuf); + scratch_buffer_free (&iovec_buf); /* Kill the application. */ abort (); }