[PATCH 0/3] Internal asserts cleanups

[PATCH 0/3] Internal asserts cleanups

[Florian Weimer]

Internal asserts do not need to write to stderr (and call into libio).
These small series redefines __assert_fail inside libc itself, so that
it more reliably terminates the process.

Thanks,
Florian

Florian Weimer (3):
  stdio: Clean up __libc_message after unconditional abort
  nptl: Remove uses of assert_perror
  assert: Do not use stderr in libc-internal assert

 assert/Makefile             |  7 +++++-
 assert/__libc_assert_fail.c | 33 ++++++++++++++++++++++++++
 assert/assert.c             |  1 -
 debug/fortify_fail.c        |  4 +---
 elf/Makefile                |  1 +
 include/assert.h            | 12 +++++++---
 include/stdio.h             |  9 +------
 malloc/malloc.c             | 19 +--------------
 stdlib/tst-bz20544.c        |  2 +-
 sysdeps/nptl/gai_misc.h     | 11 +++------
 sysdeps/posix/libc_fatal.c  | 47 +++++++++++++++++--------------------
 11 files changed, 77 insertions(+), 69 deletions(-)
 create mode 100644 assert/__libc_assert_fail.c


base-commit: 5fded9c44537283593a7f839347f831e15726572
-- 
2.35.3

[PATCH 1/3] stdio: Clean up __libc_message after unconditional abort

[Florian Weimer]

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

[PATCH 2/3] nptl: Remove uses of assert_perror

[Florian Weimer]

__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

[PATCH 3/3] assert: Do not use stderr in libc-internal assert

[Florian Weimer]

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

Re: [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort

[Adhemerval Zanella Netto]

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)

Re: [PATCH 2/3] nptl: Remove uses of assert_perror

[Adhemerval Zanella Netto]

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;

Re: [PATCH 3/3] assert: Do not use stderr in libc-internal assert

[Adhemerval Zanella Netto]

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.

Re: [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort

[Florian Weimer]

* 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

Re: [PATCH 1/3] stdio: Clean up __libc_message after unconditional abort

[Adhemerval Zanella Netto]

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.