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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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.