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_check--master-aarch64 |
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
|
Commit Message
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(-)
Comments
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.
@@ -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 ();
}