* [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort
2022-08-01 10:45 [PATCH 0/3] Internal asserts cleanups Florian Weimer
@ 2022-08-01 10:45 ` Florian Weimer
2022-08-01 19:59 ` Adhemerval Zanella Netto
2022-08-01 10:45 ` [PATCH 2/3] nptl: Remove uses of assert_perror Florian Weimer
2022-08-01 10:45 ` [PATCH 3/3] assert: Do not use stderr in libc-internal assert Florian Weimer
2 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2022-08-01 10:45 UTC (permalink / raw)
To: libc-alpha
Since commit ec2c1fcefb200c6cb7e09553f3c6af8815013d83 ("malloc:
Abort on heap corruption, without a backtrace [BZ #21754]"),
__libc_message always terminates the process. Since commit
a289ea09ea843ced6e5277c2f2e63c357bc7f9a3 ("Do not print backtraces
on fatal glibc errors"), the backtrace facility has been removed.
Therefore, remove enum __libc_message_action and the action
argument of __libc_message, and mark __libc_message as _No_return.
---
debug/fortify_fail.c | 4 +---
include/stdio.h | 9 +-------
malloc/malloc.c | 5 ++--
sysdeps/posix/libc_fatal.c | 47 +++++++++++++++++---------------------
4 files changed, 25 insertions(+), 40 deletions(-)
diff --git a/debug/fortify_fail.c b/debug/fortify_fail.c
index b1c51662b7..b880f82cb6 100644
--- a/debug/fortify_fail.c
+++ b/debug/fortify_fail.c
@@ -21,8 +21,6 @@ void
__attribute__ ((noreturn))
__fortify_fail (const char *msg)
{
- /* The loop is added only to keep gcc happy. */
- while (1)
- __libc_message (do_abort, "*** %s ***: terminated\n", msg);
+ __libc_message ("*** %s ***: terminated\n", msg);
}
libc_hidden_def (__fortify_fail)
diff --git a/include/stdio.h b/include/stdio.h
index a6f7fd43cb..c3e772ad9a 100644
--- a/include/stdio.h
+++ b/include/stdio.h
@@ -143,18 +143,11 @@ extern int __gen_tempname (char *__tmpl, int __suffixlen, int __flags,
# define __GT_DIR 1 /* create a directory */
# define __GT_NOCREATE 2 /* just find a name not currently in use */
-enum __libc_message_action
-{
- do_message = 0, /* Print message. */
- do_abort = 1 << 0, /* Abort. */
-};
-
/* Print out MESSAGE (which should end with a newline) on the error output
and abort. */
extern void __libc_fatal (const char *__message)
__attribute__ ((__noreturn__));
-extern void __libc_message (enum __libc_message_action action,
- const char *__fnt, ...) attribute_hidden;
+_Noreturn void __libc_message (const char *__fnt, ...) attribute_hidden;
extern void __fortify_fail (const char *msg) __attribute__ ((__noreturn__));
libc_hidden_proto (__fortify_fail)
diff --git a/malloc/malloc.c b/malloc/malloc.c
index bd3c76ed31..f3320d2663 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -296,8 +296,7 @@ _Noreturn static void
__malloc_assert (const char *assertion, const char *file, unsigned int line,
const char *function)
{
- __libc_message (do_abort, "\
-Fatal glibc error: malloc assertion failure in %s: %s\n",
+ __libc_message ("Fatal glibc error: malloc assertion failure in %s: %s\n",
function, assertion);
__builtin_unreachable ();
}
@@ -5657,7 +5656,7 @@ static void
malloc_printerr (const char *str)
{
#if IS_IN (libc)
- __libc_message (do_abort, "%s\n", str);
+ __libc_message ("%s\n", str);
#else
__libc_fatal (str);
#endif
diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
index 2ee0010b8d..270238495d 100644
--- a/sysdeps/posix/libc_fatal.c
+++ b/sysdeps/posix/libc_fatal.c
@@ -54,7 +54,7 @@ struct str_list
/* Abort with an error message. */
void
-__libc_message (enum __libc_message_action action, const char *fmt, ...)
+__libc_message (const char *fmt, ...)
{
va_list ap;
int fd = -1;
@@ -123,36 +123,31 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...)
WRITEV_FOR_FATAL (fd, iov, nlist, total);
- if ((action & do_abort))
+ total = (total + 1 + GLRO(dl_pagesize) - 1) & ~(GLRO(dl_pagesize) - 1);
+ struct abort_msg_s *buf = __mmap (NULL, total,
+ PROT_READ | PROT_WRITE,
+ MAP_ANON | MAP_PRIVATE, -1, 0);
+ if (__glibc_likely (buf != MAP_FAILED))
{
- total = ((total + 1 + GLRO(dl_pagesize) - 1)
- & ~(GLRO(dl_pagesize) - 1));
- struct abort_msg_s *buf = __mmap (NULL, total,
- PROT_READ | PROT_WRITE,
- MAP_ANON | MAP_PRIVATE, -1, 0);
- if (__glibc_likely (buf != MAP_FAILED))
- {
- buf->size = total;
- char *wp = buf->msg;
- for (int cnt = 0; cnt < nlist; ++cnt)
- wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len);
- *wp = '\0';
-
- /* We have to free the old buffer since the application might
- catch the SIGABRT signal. */
- struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg,
- buf);
- if (old != NULL)
- __munmap (old, old->size);
- }
+ buf->size = total;
+ char *wp = buf->msg;
+ for (int cnt = 0; cnt < nlist; ++cnt)
+ wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len);
+ *wp = '\0';
+
+ /* We have to free the old buffer since the application might
+ catch the SIGABRT signal. */
+ struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg,
+ buf);
+ if (old != NULL)
+ __munmap (old, old->size);
}
}
va_end (ap);
- if ((action & do_abort))
- /* Kill the application. */
- abort ();
+ /* Kill the application. */
+ abort ();
}
@@ -161,6 +156,6 @@ __libc_fatal (const char *message)
{
/* The loop is added only to keep gcc happy. */
while (1)
- __libc_message (do_abort, "%s", message);
+ __libc_message ("%s", message);
}
libc_hidden_def (__libc_fatal)
--
2.35.3
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort
2022-08-01 10:45 ` [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort Florian Weimer
@ 2022-08-01 19:59 ` Adhemerval Zanella Netto
2022-08-02 8:03 ` Florian Weimer
0 siblings, 1 reply; 9+ messages in thread
From: Adhemerval Zanella Netto @ 2022-08-01 19:59 UTC (permalink / raw)
To: libc-alpha, Florian Weimer
On 01/08/22 07:45, Florian Weimer via Libc-alpha wrote:
> Since commit ec2c1fcefb200c6cb7e09553f3c6af8815013d83 ("malloc:
> Abort on heap corruption, without a backtrace [BZ #21754]"),
> __libc_message always terminates the process. Since commit
> a289ea09ea843ced6e5277c2f2e63c357bc7f9a3 ("Do not print backtraces
> on fatal glibc errors"), the backtrace facility has been removed.
> Therefore, remove enum __libc_message_action and the action
> argument of __libc_message, and mark __libc_message as _No_return.
LGTM, just a comment below.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> debug/fortify_fail.c | 4 +---
> include/stdio.h | 9 +-------
> malloc/malloc.c | 5 ++--
> sysdeps/posix/libc_fatal.c | 47 +++++++++++++++++---------------------
> 4 files changed, 25 insertions(+), 40 deletions(-)
>
> diff --git a/debug/fortify_fail.c b/debug/fortify_fail.c
> index b1c51662b7..b880f82cb6 100644
> --- a/debug/fortify_fail.c
> +++ b/debug/fortify_fail.c
> @@ -21,8 +21,6 @@ void
> __attribute__ ((noreturn))
> __fortify_fail (const char *msg)
> {
> - /* The loop is added only to keep gcc happy. */
> - while (1)
> - __libc_message (do_abort, "*** %s ***: terminated\n", msg);
> + __libc_message ("*** %s ***: terminated\n", msg);
> }
> libc_hidden_def (__fortify_fail)
> diff --git a/include/stdio.h b/include/stdio.h
> index a6f7fd43cb..c3e772ad9a 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -143,18 +143,11 @@ extern int __gen_tempname (char *__tmpl, int __suffixlen, int __flags,
> # define __GT_DIR 1 /* create a directory */
> # define __GT_NOCREATE 2 /* just find a name not currently in use */
>
> -enum __libc_message_action
> -{
> - do_message = 0, /* Print message. */
> - do_abort = 1 << 0, /* Abort. */
> -};
> -
> /* Print out MESSAGE (which should end with a newline) on the error output
> and abort. */
> extern void __libc_fatal (const char *__message)
> __attribute__ ((__noreturn__));
> -extern void __libc_message (enum __libc_message_action action,
> - const char *__fnt, ...) attribute_hidden;
> +_Noreturn void __libc_message (const char *__fnt, ...) attribute_hidden;
> extern void __fortify_fail (const char *msg) __attribute__ ((__noreturn__));
> libc_hidden_proto (__fortify_fail)
>
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index bd3c76ed31..f3320d2663 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -296,8 +296,7 @@ _Noreturn static void
> __malloc_assert (const char *assertion, const char *file, unsigned int line,
> const char *function)
> {
> - __libc_message (do_abort, "\
> -Fatal glibc error: malloc assertion failure in %s: %s\n",
> + __libc_message ("Fatal glibc error: malloc assertion failure in %s: %s\n",
> function, assertion);
> __builtin_unreachable ();
> }
> @@ -5657,7 +5656,7 @@ static void
> malloc_printerr (const char *str)
> {
> #if IS_IN (libc)
> - __libc_message (do_abort, "%s\n", str);
> + __libc_message ("%s\n", str);
> #else
> __libc_fatal (str);
> #endif
> diff --git a/sysdeps/posix/libc_fatal.c b/sysdeps/posix/libc_fatal.c
> index 2ee0010b8d..270238495d 100644
> --- a/sysdeps/posix/libc_fatal.c
> +++ b/sysdeps/posix/libc_fatal.c
> @@ -54,7 +54,7 @@ struct str_list
>
> /* Abort with an error message. */
> void
> -__libc_message (enum __libc_message_action action, const char *fmt, ...)
> +__libc_message (const char *fmt, ...)
> {
> va_list ap;
> int fd = -1;
> @@ -123,36 +123,31 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...)
>
> WRITEV_FOR_FATAL (fd, iov, nlist, total);
>
> - if ((action & do_abort))
> + total = (total + 1 + GLRO(dl_pagesize) - 1) & ~(GLRO(dl_pagesize) - 1);
> + struct abort_msg_s *buf = __mmap (NULL, total,
> + PROT_READ | PROT_WRITE,
> + MAP_ANON | MAP_PRIVATE, -1, 0);
> + if (__glibc_likely (buf != MAP_FAILED))
> {
> - total = ((total + 1 + GLRO(dl_pagesize) - 1)
> - & ~(GLRO(dl_pagesize) - 1));
> - struct abort_msg_s *buf = __mmap (NULL, total,
> - PROT_READ | PROT_WRITE,
> - MAP_ANON | MAP_PRIVATE, -1, 0);
> - if (__glibc_likely (buf != MAP_FAILED))
> - {
> - buf->size = total;
> - char *wp = buf->msg;
> - for (int cnt = 0; cnt < nlist; ++cnt)
> - wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len);
> - *wp = '\0';
> -
> - /* We have to free the old buffer since the application might
> - catch the SIGABRT signal. */
> - struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg,
> - buf);
> - if (old != NULL)
> - __munmap (old, old->size);
> - }
> + buf->size = total;
> + char *wp = buf->msg;
> + for (int cnt = 0; cnt < nlist; ++cnt)
> + wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len);
> + *wp = '\0';
> +
> + /* We have to free the old buffer since the application might
> + catch the SIGABRT signal. */
> + struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg,
> + buf);
> + if (old != NULL)
> + __munmap (old, old->size);
I am aware that this is just replicating the code already in place, but maybe
replace the old atomic with atomic_exchange_acquire here.
> }
> }
>
> va_end (ap);
>
> - if ((action & do_abort))
> - /* Kill the application. */
> - abort ();
> + /* Kill the application. */
> + abort ();
> }
>
>
> @@ -161,6 +156,6 @@ __libc_fatal (const char *message)
> {
> /* The loop is added only to keep gcc happy. */
> while (1)
> - __libc_message (do_abort, "%s", message);
> + __libc_message ("%s", message);
> }
> libc_hidden_def (__libc_fatal)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort
2022-08-01 19:59 ` Adhemerval Zanella Netto
@ 2022-08-02 8:03 ` Florian Weimer
2022-08-02 13:43 ` Adhemerval Zanella Netto
0 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2022-08-02 8:03 UTC (permalink / raw)
To: Adhemerval Zanella Netto via Libc-alpha
* Adhemerval Zanella Netto via Libc-alpha:
>> + buf->size = total;
>> + char *wp = buf->msg;
>> + for (int cnt = 0; cnt < nlist; ++cnt)
>> + wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len);
>> + *wp = '\0';
>> +
>> + /* We have to free the old buffer since the application might
>> + catch the SIGABRT signal. */
>> + struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg,
>> + buf);
>> + if (old != NULL)
>> + __munmap (old, old->size);
>
> I am aware that this is just replicating the code already in place, but maybe
> replace the old atomic with atomic_exchange_acquire here.
We should use a CAS here anyway because the earlier message is likely
more valuable for debugging purposes. The exchange was needed because
when __libc_message could continue execution in the old days, we had to
somehow avoid the memory leak. That's no longer a concern with the
current code. I will try to clean this up further.
Thanks,
Florian
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort
2022-08-02 8:03 ` Florian Weimer
@ 2022-08-02 13:43 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella Netto @ 2022-08-02 13:43 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella Netto via Libc-alpha
On 02/08/22 05:03, Florian Weimer wrote:
> * Adhemerval Zanella Netto via Libc-alpha:
>
>>> + buf->size = total;
>>> + char *wp = buf->msg;
>>> + for (int cnt = 0; cnt < nlist; ++cnt)
>>> + wp = mempcpy (wp, iov[cnt].iov_base, iov[cnt].iov_len);
>>> + *wp = '\0';
>>> +
>>> + /* We have to free the old buffer since the application might
>>> + catch the SIGABRT signal. */
>>> + struct abort_msg_s *old = atomic_exchange_acq (&__abort_msg,
>>> + buf);
>>> + if (old != NULL)
>>> + __munmap (old, old->size);
>>
>> I am aware that this is just replicating the code already in place, but maybe
>> replace the old atomic with atomic_exchange_acquire here.
>
> We should use a CAS here anyway because the earlier message is likely
> more valuable for debugging purposes. The exchange was needed because
> when __libc_message could continue execution in the old days, we had to
> somehow avoid the memory leak. That's no longer a concern with the
> current code. I will try to clean this up further.
My intention was more to move away from old atomics macros and use compiler
builtins when possible.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] nptl: Remove uses of assert_perror
2022-08-01 10:45 [PATCH 0/3] Internal asserts cleanups Florian Weimer
2022-08-01 10:45 ` [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort Florian Weimer
@ 2022-08-01 10:45 ` Florian Weimer
2022-08-01 20:01 ` Adhemerval Zanella Netto
2022-08-01 10:45 ` [PATCH 3/3] assert: Do not use stderr in libc-internal assert Florian Weimer
2 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2022-08-01 10:45 UTC (permalink / raw)
To: libc-alpha
__pthread_sigmask cannot actually fail with valid pointer arguments
(it would need a really broken seccomp filter), and we do not check
for errors elsewhere.
---
sysdeps/nptl/gai_misc.h | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/sysdeps/nptl/gai_misc.h b/sysdeps/nptl/gai_misc.h
index c09350c2ed..4196eac6e6 100644
--- a/sysdeps/nptl/gai_misc.h
+++ b/sysdeps/nptl/gai_misc.h
@@ -81,9 +81,7 @@ __gai_start_notify_thread (void)
{
sigset_t ss;
sigemptyset (&ss);
- int sigerr __attribute__ ((unused));
- sigerr = __pthread_sigmask (SIG_SETMASK, &ss, NULL);
- assert_perror (sigerr);
+ (void) __pthread_sigmask (SIG_SETMASK, &ss, NULL);
}
extern inline int
@@ -106,15 +104,12 @@ __gai_create_helper_thread (pthread_t *threadp, void *(*tf) (void *),
sigset_t ss;
sigset_t oss;
sigfillset (&ss);
- int sigerr __attribute__ ((unused));
- sigerr = __pthread_sigmask (SIG_SETMASK, &ss, &oss);
- assert_perror (sigerr);
+ (void) __pthread_sigmask (SIG_SETMASK, &ss, &oss);
int ret = __pthread_create (threadp, &attr, tf, arg);
/* Restore the signal mask. */
- sigerr = __pthread_sigmask (SIG_SETMASK, &oss, NULL);
- assert_perror (sigerr);
+ (void) __pthread_sigmask (SIG_SETMASK, &oss, NULL);
(void) __pthread_attr_destroy (&attr);
return ret;
--
2.35.3
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] nptl: Remove uses of assert_perror
2022-08-01 10:45 ` [PATCH 2/3] nptl: Remove uses of assert_perror Florian Weimer
@ 2022-08-01 20:01 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella Netto @ 2022-08-01 20:01 UTC (permalink / raw)
To: libc-alpha
On 01/08/22 07:45, Florian Weimer via Libc-alpha wrote:
> __pthread_sigmask cannot actually fail with valid pointer arguments
> (it would need a really broken seccomp filter), and we do not check
> for errors elsewhere.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> sysdeps/nptl/gai_misc.h | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/sysdeps/nptl/gai_misc.h b/sysdeps/nptl/gai_misc.h
> index c09350c2ed..4196eac6e6 100644
> --- a/sysdeps/nptl/gai_misc.h
> +++ b/sysdeps/nptl/gai_misc.h
> @@ -81,9 +81,7 @@ __gai_start_notify_thread (void)
> {
> sigset_t ss;
> sigemptyset (&ss);
> - int sigerr __attribute__ ((unused));
> - sigerr = __pthread_sigmask (SIG_SETMASK, &ss, NULL);
> - assert_perror (sigerr);
> + (void) __pthread_sigmask (SIG_SETMASK, &ss, NULL);
> }
>
> extern inline int
> @@ -106,15 +104,12 @@ __gai_create_helper_thread (pthread_t *threadp, void *(*tf) (void *),
> sigset_t ss;
> sigset_t oss;
> sigfillset (&ss);
> - int sigerr __attribute__ ((unused));
> - sigerr = __pthread_sigmask (SIG_SETMASK, &ss, &oss);
> - assert_perror (sigerr);
> + (void) __pthread_sigmask (SIG_SETMASK, &ss, &oss);
>
> int ret = __pthread_create (threadp, &attr, tf, arg);
>
> /* Restore the signal mask. */
> - sigerr = __pthread_sigmask (SIG_SETMASK, &oss, NULL);
> - assert_perror (sigerr);
> + (void) __pthread_sigmask (SIG_SETMASK, &oss, NULL);
>
> (void) __pthread_attr_destroy (&attr);
> return ret;
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] assert: Do not use stderr in libc-internal assert
2022-08-01 10:45 [PATCH 0/3] Internal asserts cleanups Florian Weimer
2022-08-01 10:45 ` [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort Florian Weimer
2022-08-01 10:45 ` [PATCH 2/3] nptl: Remove uses of assert_perror Florian Weimer
@ 2022-08-01 10:45 ` Florian Weimer
2022-08-01 20:04 ` Adhemerval Zanella Netto
2 siblings, 1 reply; 9+ messages in thread
From: Florian Weimer @ 2022-08-01 10:45 UTC (permalink / raw)
To: libc-alpha
Redirect internal assertion failures to __libc_assert_fail, based on
based on __libc_message, which writes directly to STDERR_FILENO
and calls abort. Also disable message translation and reword the
error message slightly (adjusting stdlib/tst-bz20544 accordingly).
As a result of these changes, malloc no longer needs its own
redefinition of __assert_fail.
__libc_assert_fail needs to be stubbed out during rtld dependency
analysis because the rtld rebuilds turn __libc_assert_fail into
__assert_fail, which is unconditionally provided by elf/dl-minimal.c.
This change is not possible for the public assert macro and its
__assert_fail function because POSIX requires that the diagnostic
is written to stderr.
---
assert/Makefile | 7 ++++++-
assert/__libc_assert_fail.c | 33 +++++++++++++++++++++++++++++++++
assert/assert.c | 1 -
elf/Makefile | 1 +
include/assert.h | 12 +++++++++---
malloc/malloc.c | 16 ----------------
stdlib/tst-bz20544.c | 2 +-
7 files changed, 50 insertions(+), 22 deletions(-)
create mode 100644 assert/__libc_assert_fail.c
diff --git a/assert/Makefile b/assert/Makefile
index f7cf8e9b77..c905845a43 100644
--- a/assert/Makefile
+++ b/assert/Makefile
@@ -24,7 +24,12 @@ include ../Makeconfig
headers := assert.h
-routines := assert assert-perr __assert
+routines := \
+ __assert \
+ __libc_assert_fail \
+ assert \
+ assert-perr \
+ # routines
tests := test-assert test-assert-perr tst-assert-c++ tst-assert-g++
ifeq ($(have-cxx-thread_local),yes)
diff --git a/assert/__libc_assert_fail.c b/assert/__libc_assert_fail.c
new file mode 100644
index 0000000000..149d5feae1
--- /dev/null
+++ b/assert/__libc_assert_fail.c
@@ -0,0 +1,33 @@
+/* libc-internal assert that calls __libc_message.
+ Copyright (C) 2022 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <_itoa.h>
+#include <array_length.h>
+#include <intprops.h>
+#include <stdio.h>
+
+void
+__libc_assert_fail (const char *assertion, const char *file, unsigned int line,
+ const char *function)
+{
+ char linebuf[INT_BUFSIZE_BOUND (unsigned int)];
+ array_end (linebuf)[-1] = '\0';
+ char *linestr = _itoa_word (line, array_end (linebuf) - 1, 10, 0);
+ __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n",
+ file, linestr, function, assertion);
+}
diff --git a/assert/assert.c b/assert/assert.c
index 133a183bc3..564ae28a2b 100644
--- a/assert/assert.c
+++ b/assert/assert.c
@@ -101,4 +101,3 @@ __assert_fail (const char *assertion, const char *file, unsigned int line,
__assert_fail_base (_("%s%s%s:%u: %s%sAssertion `%s' failed.\n%n"),
assertion, file, line, function);
}
-hidden_def(__assert_fail)
diff --git a/elf/Makefile b/elf/Makefile
index fd77d0c7c8..3386f0ce77 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1283,6 +1283,7 @@ $(objpfx)dl-allobjs.os: $(all-rtld-routines:%=$(objpfx)%.os)
rtld-stubbed-symbols = \
__GI___pthread_disable_asynccancel \
__GI___pthread_enable_asynccancel \
+ __libc_assert_fail \
__pthread_disable_asynccancel \
__pthread_enable_asynccancel \
calloc \
diff --git a/include/assert.h b/include/assert.h
index 61cc8aa22f..c812808f9b 100644
--- a/include/assert.h
+++ b/include/assert.h
@@ -20,8 +20,14 @@ extern void __assert_fail_base (const char *fmt, const char *assertion,
const char *function)
__THROW __attribute__ ((__noreturn__)) attribute_hidden;
-# if IS_IN (libc) || (IS_IN (rtld) && !defined NO_RTLD_HIDDEN)
-hidden_proto (__assert_fail)
-hidden_proto (__assert_perror_fail)
+rtld_hidden_proto (__assert_fail)
+rtld_hidden_proto (__assert_perror_fail)
+libc_hidden_proto (__assert_perror_fail)
+
+
+# if IS_IN (libc)
+/* Redirect to the internal version which does not use stderr. */
+extern _Noreturn __typeof (__assert_fail) __libc_assert_fail attribute_hidden;
+# define __assert_fail __libc_assert_fail
# endif
#endif
diff --git a/malloc/malloc.c b/malloc/malloc.c
index f3320d2663..6f9c3b59b9 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -287,22 +287,6 @@
#define MALLOC_DEBUG 0
#endif
-#if IS_IN (libc)
-#ifndef NDEBUG
-# define __assert_fail(assertion, file, line, function) \
- __malloc_assert(assertion, file, line, function)
-
-_Noreturn static void
-__malloc_assert (const char *assertion, const char *file, unsigned int line,
- const char *function)
-{
- __libc_message ("Fatal glibc error: malloc assertion failure in %s: %s\n",
- function, assertion);
- __builtin_unreachable ();
-}
-#endif
-#endif
-
#if USE_TCACHE
/* We want 64 entries. This is an arbitrary limit, which tunables can reduce. */
# define TCACHE_MAX_BINS 64
diff --git a/stdlib/tst-bz20544.c b/stdlib/tst-bz20544.c
index 411cd3f3ba..7cc236a1b1 100644
--- a/stdlib/tst-bz20544.c
+++ b/stdlib/tst-bz20544.c
@@ -78,7 +78,7 @@ test_bz20544_cxa_at_quick_exit (void *closure)
static void
test_one_fn (void (*test_fn) (void *))
{
- const char expected_error[] = "Assertion `func != NULL' failed.\n";
+ const char expected_error[] = "assertion failed: func != NULL\n";
struct support_capture_subprocess result;
result = support_capture_subprocess (test_fn, NULL);
support_capture_subprocess_check (&result, "bz20544", -SIGABRT,
--
2.35.3
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] assert: Do not use stderr in libc-internal assert
2022-08-01 10:45 ` [PATCH 3/3] assert: Do not use stderr in libc-internal assert Florian Weimer
@ 2022-08-01 20:04 ` Adhemerval Zanella Netto
0 siblings, 0 replies; 9+ messages in thread
From: Adhemerval Zanella Netto @ 2022-08-01 20:04 UTC (permalink / raw)
To: Florian Weimer, libc-alpha
On 01/08/22 07:45, Florian Weimer via Libc-alpha wrote:
> Redirect internal assertion failures to __libc_assert_fail, based on
> based on __libc_message, which writes directly to STDERR_FILENO
> and calls abort. Also disable message translation and reword the
> error message slightly (adjusting stdlib/tst-bz20544 accordingly).
>
> As a result of these changes, malloc no longer needs its own
> redefinition of __assert_fail.
>
> __libc_assert_fail needs to be stubbed out during rtld dependency
> analysis because the rtld rebuilds turn __libc_assert_fail into
> __assert_fail, which is unconditionally provided by elf/dl-minimal.c.
>
> This change is not possible for the public assert macro and its
> __assert_fail function because POSIX requires that the diagnostic
> is written to stderr.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> assert/Makefile | 7 ++++++-
> assert/__libc_assert_fail.c | 33 +++++++++++++++++++++++++++++++++
> assert/assert.c | 1 -
> elf/Makefile | 1 +
> include/assert.h | 12 +++++++++---
> malloc/malloc.c | 16 ----------------
> stdlib/tst-bz20544.c | 2 +-
> 7 files changed, 50 insertions(+), 22 deletions(-)
> create mode 100644 assert/__libc_assert_fail.c
>
> diff --git a/assert/Makefile b/assert/Makefile
> index f7cf8e9b77..c905845a43 100644
> --- a/assert/Makefile
> +++ b/assert/Makefile
> @@ -24,7 +24,12 @@ include ../Makeconfig
>
> headers := assert.h
>
> -routines := assert assert-perr __assert
> +routines := \
> + __assert \
> + __libc_assert_fail \
> + assert \
> + assert-perr \
> + # routines
> tests := test-assert test-assert-perr tst-assert-c++ tst-assert-g++
>
> ifeq ($(have-cxx-thread_local),yes)
Ok.
> diff --git a/assert/__libc_assert_fail.c b/assert/__libc_assert_fail.c
> new file mode 100644
> index 0000000000..149d5feae1
> --- /dev/null
> +++ b/assert/__libc_assert_fail.c
> @@ -0,0 +1,33 @@
> +/* libc-internal assert that calls __libc_message.
> + Copyright (C) 2022 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <_itoa.h>
> +#include <array_length.h>
> +#include <intprops.h>
> +#include <stdio.h>
> +
> +void
> +__libc_assert_fail (const char *assertion, const char *file, unsigned int line,
> + const char *function)
> +{
> + char linebuf[INT_BUFSIZE_BOUND (unsigned int)];
> + array_end (linebuf)[-1] = '\0';
> + char *linestr = _itoa_word (line, array_end (linebuf) - 1, 10, 0);
> + __libc_message ("Fatal glibc error: %s:%s (%s): assertion failed: %s\n",
> + file, linestr, function, assertion);
> +}
Ok.
> diff --git a/assert/assert.c b/assert/assert.c
> index 133a183bc3..564ae28a2b 100644
> --- a/assert/assert.c
> +++ b/assert/assert.c
> @@ -101,4 +101,3 @@ __assert_fail (const char *assertion, const char *file, unsigned int line,
> __assert_fail_base (_("%s%s%s:%u: %s%sAssertion `%s' failed.\n%n"),
> assertion, file, line, function);
> }
> -hidden_def(__assert_fail)
Ok.
> diff --git a/elf/Makefile b/elf/Makefile
> index fd77d0c7c8..3386f0ce77 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1283,6 +1283,7 @@ $(objpfx)dl-allobjs.os: $(all-rtld-routines:%=$(objpfx)%.os)
> rtld-stubbed-symbols = \
> __GI___pthread_disable_asynccancel \
> __GI___pthread_enable_asynccancel \
> + __libc_assert_fail \
> __pthread_disable_asynccancel \
> __pthread_enable_asynccancel \
> calloc \
Ok.
> diff --git a/include/assert.h b/include/assert.h
> index 61cc8aa22f..c812808f9b 100644
> --- a/include/assert.h
> +++ b/include/assert.h
> @@ -20,8 +20,14 @@ extern void __assert_fail_base (const char *fmt, const char *assertion,
> const char *function)
> __THROW __attribute__ ((__noreturn__)) attribute_hidden;
>
> -# if IS_IN (libc) || (IS_IN (rtld) && !defined NO_RTLD_HIDDEN)
> -hidden_proto (__assert_fail)
> -hidden_proto (__assert_perror_fail)
> +rtld_hidden_proto (__assert_fail)
> +rtld_hidden_proto (__assert_perror_fail)
> +libc_hidden_proto (__assert_perror_fail)
> +
> +
> +# if IS_IN (libc)
> +/* Redirect to the internal version which does not use stderr. */
> +extern _Noreturn __typeof (__assert_fail) __libc_assert_fail attribute_hidden;
> +# define __assert_fail __libc_assert_fail
> # endif
> #endif
Ok.
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index f3320d2663..6f9c3b59b9 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -287,22 +287,6 @@
> #define MALLOC_DEBUG 0
> #endif
>
> -#if IS_IN (libc)
> -#ifndef NDEBUG
> -# define __assert_fail(assertion, file, line, function) \
> - __malloc_assert(assertion, file, line, function)
> -
> -_Noreturn static void
> -__malloc_assert (const char *assertion, const char *file, unsigned int line,
> - const char *function)
> -{
> - __libc_message ("Fatal glibc error: malloc assertion failure in %s: %s\n",
> - function, assertion);
> - __builtin_unreachable ();
> -}
> -#endif
> -#endif
> -
> #if USE_TCACHE
> /* We want 64 entries. This is an arbitrary limit, which tunables can reduce. */
> # define TCACHE_MAX_BINS 64
Ok.
> diff --git a/stdlib/tst-bz20544.c b/stdlib/tst-bz20544.c
> index 411cd3f3ba..7cc236a1b1 100644
> --- a/stdlib/tst-bz20544.c
> +++ b/stdlib/tst-bz20544.c
> @@ -78,7 +78,7 @@ test_bz20544_cxa_at_quick_exit (void *closure)
> static void
> test_one_fn (void (*test_fn) (void *))
> {
> - const char expected_error[] = "Assertion `func != NULL' failed.\n";
> + const char expected_error[] = "assertion failed: func != NULL\n";
> struct support_capture_subprocess result;
> result = support_capture_subprocess (test_fn, NULL);
> support_capture_subprocess_check (&result, "bz20544", -SIGABRT,
Ok.
^ permalink raw reply [flat|nested] 9+ messages in thread