[PATCH] libc_fatal: Get rid of alloca

[PATCH] libc_fatal: Get rid of alloca

[Joe Simmons-Talbott]

Use scratch_buffers in place of alloca to avoid potential stack overflow.
---
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.

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

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 ();
 }
-- 
2.39.2

Re: [PATCH] libc_fatal: Get rid of alloca

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] libc_fatal: Get rid of alloca

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] libc_fatal: Get rid of alloca

[Joe Simmons-Talbott]

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

Re: [PATCH] libc_fatal: Get rid of alloca

[Andreas Schwab]

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?

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] libc_fatal: Get rid of alloca

[Joe Simmons-Talbott]

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

Re: [PATCH] libc_fatal: Get rid of alloca

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] libc_fatal: Get rid of alloca

[Adhemerval Zanella Netto]

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.

Re: [PATCH] libc_fatal: Get rid of alloca

[Florian Weimer]

* 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.

Re: [PATCH] libc_fatal: Get rid of alloca

[Adhemerval Zanella Netto]

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.