[v2] 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_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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 ();
}
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
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
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
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
@@ -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 ();
}