Re: [PATCH] Do not print backtraces on fatal glibc errors

[Carlos O'Donell <carlos at redhat dot com>]

On 8/19/19 6:03 AM, Florian Weimer wrote:
> * Mao Han:
> 
>> On Thu, Aug 15, 2019 at 10:00:21PM +0200, Florian Weimer wrote:
>>> If the process is in a bad state, we used to print backtraces in
>>> many cases.  This is problematic because doing so could involve
>>> a lot of work, like loading libgcc_s using the dynamic linker,
>>> and this could itself be targeted by exploit writers.  For example,
>>> if the crashing process was forked from a long-lived process, the
>>> addresses in the error message could be used to bypass ASLR.
>>>
>>> This commit completes the removal of backtrace printing from glibc.
>>> With the prevalence of external crash handlers, this functionality
>>> does not appear to be particular useful.  The crash handler may also
>>> destroy useful information for debugging if coredumps are enabled.
>>
>> Thanks for this simplification. Dose the previous code(without this
>> cleanup) intentionally designed to be optimized not to print the
>> backtrace? My colleague says that the optimization for enum is generic
>> to all the arch, I've tried to verified that on x86(compiled with
>> gcc 9.0) and got similar optimization. Our customer complained about
>> the crash backtrace printing difference between C-SKY and x86. Now I
>> can tell them it is removed for security and reliability with this
>> patch. :)
> 
> Thanks for reminding me that this still needs investigation.  I found
> out what is going on and updated the commit message.
> 
>> You'v said that this backtrace printing is not particular useful. So
>> how should I suggest our customer to debug their user program(memory
>> corruption/segmentation fault)? Coredump, hook the crash handlers by
>> themself or some other method? I normally do not use tools other
>> than gdb and don't know much about these.
> 
> Coredumps or external crash handlers (kernel.core_pattern starting with
> "|") are very helpful.  In-process crash handlers do not work all that
> well for C or C++ programs in our experience.
> 
> * Dmitry V. Levin:
> 
>>> This commit completes the removal of backtrace printing from glibc.
>>> With the prevalence of external crash handlers, this functionality
>>> does not appear to be particular useful.  The crash handler may also
>>> destroy useful information for debugging if coredumps are enabled.
>>
>> s/particular useful/particularly useful/
> 
> Fixed below.
> 
> Thanks,
> Florian
> 
> 
> Do not print backtraces on fatal glibc errors
> 
> If the process is in a bad state, we used to print backtraces in
> many cases.  This is problematic because doing so could involve
> a lot of work, like loading libgcc_s using the dynamic linker,
> and this could itself be targeted by exploit writers.  For example,
> if the crashing process was forked from a long-lived process, the
> addresses in the error message could be used to bypass ASLR.
> 
> Commit ed421fca42fd9b4cab7c66e77894b8dd7ca57ed0 ("Avoid backtrace from
> __stack_chk_fail [BZ #12189]"), backtraces where no longer printed
> because backtrace_and_maps was always called with do_abort == 1.
> 
> Rather than fixing this logic error, this change removes the backtrace
> functionality from the sources.  With the prevalence of external crash
> handlers, it does not appear to be particularly useful.  The crash
> handler may also destroy useful information for debugging.

Still looks good to me.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2019-08-19  Florian Weimer  <fweimer@redhat.com>
> 
> 	Do not print backtraces on fatal errors.
> 	* debug/fortify_fail.c (__libc_argv): Remove declaration.
> 	(__fortify_fail_abort): Remove definition.
> 	(__fortify_fail): Call __libc_message directly.
> 	* debug/stack_chk_fail.c (__libc_argv): Remove declaration.
> 	(__stack_chk_fail): Call __fortify_fail instead of
> 	__fortify_fail_abort.
> 	* include/stdio.h (__fortify_fail_abort): Remove declaration.
> 	* sysdeps/posix/libc_fatal.c (BEFORE_ABORT, before_abort): Remove
> 	definitions.
> 	(__libc_message): Do not handle do_backtrace.  Do not call
> 	BEFORE_ABORT.
> 	(__libc_fatal): Do not pass do_backtrace to __libc_message.
> 	* sysdeps/unix/sysv/linux/libc_fatal.c (BEFORE_ABORT)
> 	(before_abort): Remove definitions.
> 
> diff --git a/NEWS b/NEWS
> index 045720b3fb..c44aa7148a 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -25,6 +25,9 @@ Deprecated and removed features, and other changes affecting compatibility:
>    Request 25 to TS 18661-1, as applied for C2X.  Existing binaries that pass
>    floating-point arguments directly will continue to work.
>  
> +* glibc no longer prints a backtrace and process map on certain fatal errors
> +  (such as constraint violations detected in _FORTIFY_SOURCE mode).
> +
>  Changes to build and runtime requirements:
>  
>    [Add changes to build and runtime requirements here]
> diff --git a/debug/fortify_fail.c b/debug/fortify_fail.c
> index 16549d6dbc..272a829fd4 100644
> --- a/debug/fortify_fail.c
> +++ b/debug/fortify_fail.c
> @@ -16,33 +16,13 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <stdio.h>
> -#include <stdlib.h>
> -#include <stdbool.h>
> -
> -
> -extern char **__libc_argv attribute_hidden;
> -
> -void
> -__attribute__ ((noreturn))
> -__fortify_fail_abort (_Bool need_backtrace, const char *msg)
> -{
> -  /* The loop is added only to keep gcc happy.  Don't pass down
> -     __libc_argv[0] if we aren't doing backtrace since __libc_argv[0]
> -     may point to the corrupted stack.  */
> -  while (1)
> -    __libc_message (need_backtrace ? (do_abort | do_backtrace) : do_abort,
> -		    "*** %s ***: %s terminated\n",
> -		    msg,
> -		    (need_backtrace && __libc_argv[0] != NULL
> -		     ? __libc_argv[0] : "<unknown>"));
> -}
>  
>  void
>  __attribute__ ((noreturn))
>  __fortify_fail (const char *msg)
>  {
> -  __fortify_fail_abort (true, msg);
> +  /* The loop is added only to keep gcc happy.  */
> +  while (1)
> +    __libc_message (do_abort, "*** %s ***: terminated\n", msg);
>  }
> -
>  libc_hidden_def (__fortify_fail)
> -libc_hidden_def (__fortify_fail_abort)
> diff --git a/debug/stack_chk_fail.c b/debug/stack_chk_fail.c
> index 4485655599..d4381dfa53 100644
> --- a/debug/stack_chk_fail.c
> +++ b/debug/stack_chk_fail.c
> @@ -16,17 +16,12 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <stdio.h>
> -#include <stdlib.h>
> -#include <stdbool.h>
> -
> -
> -extern char **__libc_argv attribute_hidden;
>  
>  void
>  __attribute__ ((noreturn))
>  __stack_chk_fail (void)
>  {
> -  __fortify_fail_abort (false, "stack smashing detected");
> +  __fortify_fail ("stack smashing detected");
>  }
>  
>  strong_alias (__stack_chk_fail, __stack_chk_fail_local)
> diff --git a/include/stdio.h b/include/stdio.h
> index 5302e61024..bea2066cd1 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -102,7 +102,6 @@ enum __libc_message_action
>  {
>    do_message	= 0,		/* Print message.  */
>    do_abort	= 1 << 0,	/* Abort.  */
> -  do_backtrace	= 1 << 1	/* Backtrace.  */
>  };
>  
>  /* Print out MESSAGE (which should end with a newline) on the error output
> @@ -112,10 +111,7 @@ extern void __libc_fatal (const char *__message)
>  extern void __libc_message (enum __libc_message_action action,
>  			    const char *__fnt, ...) attribute_hidden;
>  extern void __fortify_fail (const char *msg) __attribute__ ((__noreturn__));
> -extern void __fortify_fail_abort (_Bool, const char *msg)
> -  __attribute__ ((__noreturn__)) attribute_hidden;
>  libc_hidden_proto (__fortify_fail)
> -libc_hidden_proto (__fortify_fail_abort)
>  
>  /* 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 3906af5ee7..9ddbfa7314 100644
> --- a/sysdeps/posix/libc_fatal.c
> +++ b/sysdeps/posix/libc_fatal.c
> @@ -45,16 +45,6 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
>  }
>  #endif
>  
> -#ifndef BEFORE_ABORT
> -# define BEFORE_ABORT		before_abort
> -static void
> -before_abort (int do_abort __attribute__ ((unused)),
> -              bool written __attribute__ ((unused)),
> -              int fd __attribute__ ((unused)))
> -{
> -}
> -#endif
> -
>  struct str_list
>  {
>    const char *str;
> @@ -75,17 +65,6 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...)
>    FATAL_PREPARE;
>  #endif
>  
> -  /* Don't call __libc_secure_getenv if we aren't doing backtrace, which
> -     may access the corrupted stack.  */
> -  if ((action & do_backtrace))
> -    {
> -      /* Open a descriptor for /dev/tty unless the user explicitly
> -	 requests errors on standard error.  */
> -      const char *on_2 = __libc_secure_getenv ("LIBC_FATAL_STDERR_");
> -      if (on_2 == NULL || *on_2 == '\0')
> -	fd = __open_nocancel (_PATH_TTY, O_RDWR | O_NOCTTY | O_NDELAY);
> -    }
> -
>    if (fd == -1)
>      fd = STDERR_FILENO;
>  
> @@ -129,7 +108,6 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...)
>        ++nlist;
>      }
>  
> -  bool written = false;
>    if (nlist > 0)
>      {
>        struct iovec *iov = alloca (nlist * sizeof (struct iovec));
> @@ -143,7 +121,7 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...)
>  	  list = list->next;
>  	}
>  
> -      written = WRITEV_FOR_FATAL (fd, iov, nlist, total);
> +      WRITEV_FOR_FATAL (fd, iov, nlist, total);
>  
>        if ((action & do_abort))
>  	{
> @@ -173,13 +151,8 @@ __libc_message (enum __libc_message_action action, const char *fmt, ...)
>    va_end (ap);
>  
>    if ((action & do_abort))
> -    {
> -      if ((action & do_backtrace))
> -	BEFORE_ABORT (do_abort, written, fd);
> -
> -      /* Kill the application.  */
> -      abort ();
> -    }
> +    /* Kill the application.  */
> +    abort ();
>  }
>  
>  
> @@ -188,6 +161,6 @@ __libc_fatal (const char *message)
>  {
>    /* The loop is added only to keep gcc happy.  */
>    while (1)
> -    __libc_message (do_abort | do_backtrace, "%s", message);
> +    __libc_message (do_abort, "%s", message);
>  }
>  libc_hidden_def (__libc_fatal)
> diff --git a/sysdeps/unix/sysv/linux/libc_fatal.c b/sysdeps/unix/sysv/linux/libc_fatal.c
> index 56c626339f..50a613e31f 100644
> --- a/sysdeps/unix/sysv/linux/libc_fatal.c
> +++ b/sysdeps/unix/sysv/linux/libc_fatal.c
> @@ -17,11 +17,6 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> -#include <execinfo.h>
> -#include <fcntl.h>
> -#include <not-cancel.h>
> -#include <string.h>
> -#include <sys/mman.h>
>  #include <sys/uio.h>
>  
>  static bool
> @@ -37,32 +32,4 @@ writev_for_fatal (int fd, const struct iovec *iov, size_t niov, size_t total)
>  }
>  #define WRITEV_FOR_FATAL	writev_for_fatal
>  
> -static void
> -backtrace_and_maps (int do_abort, bool written, int fd)
> -{
> -  if (do_abort > 1 && written)
> -    {
> -      void *addrs[64];
> -#define naddrs (sizeof (addrs) / sizeof (addrs[0]))
> -      int n = __backtrace (addrs, naddrs);
> -      if (n > 2)
> -        {
> -#define strnsize(str) str, strlen (str)
> -#define writestr(str) __write_nocancel (fd, str)
> -          writestr (strnsize ("======= Backtrace: =========\n"));
> -          __backtrace_symbols_fd (addrs + 1, n - 1, fd);
> -
> -          writestr (strnsize ("======= Memory map: ========\n"));
> -          int fd2 = __open_nocancel ("/proc/self/maps", O_RDONLY);
> -          char buf[1024];
> -          ssize_t n2;
> -          while ((n2 = __read_nocancel (fd2, buf, sizeof (buf))) > 0)
> -            if (__write_nocancel (fd, buf, n2) != n2)
> -              break;
> -          __close_nocancel_nostatus (fd2);
> -        }
> -    }
> -}
> -#define BEFORE_ABORT		backtrace_and_maps
> -
>  #include <sysdeps/posix/libc_fatal.c>
> 


-- 
Cheers,
Carlos.