| Message ID | 875zmy2o8a.fsf@oldenburg2.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Do not print backtraces on fatal glibc errors | expand |
On 8/15/19 4:00 PM, 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. > Great work! > 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. LGTM. Confirmed locations of do_backtrace and that they were all removed. Reviewed-by: Carlos O'Donell <carlos@redhat.com> > 2019-08-15 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). OK. > + > 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>")); > -} > OK. > 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); OK. > } > - > libc_hidden_def (__fortify_fail) > -libc_hidden_def (__fortify_fail_abort) OK. > 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"); OK. > } > > 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. */ OK. > }; > > /* 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) OK. > > /* 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 > - OK. > 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); > - } > - OK. > 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 (); OK. > } > > > @@ -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> OK. > #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 > - OK. > #include <sysdeps/posix/libc_fatal.c> >
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. :) 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. Thanks, 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. s/particular useful/particularly useful/
* 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. 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>
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> >
* Carlos O'Donell: > Still looks good to me. > > Reviewed-by: Carlos O'Donell <carlos@redhat.com> Thanks. I've realized that the NEWS entry is incorrect (this is not a user-visible change), so I will drop it. Thanks, Florian
On 8/19/19 9:23 AM, Florian Weimer wrote: > * Carlos O'Donell: > >> Still looks good to me. >> >> Reviewed-by: Carlos O'Donell <carlos@redhat.com> > > Thanks. I've realized that the NEWS entry is incorrect (this is not a > user-visible change), so I will drop it. Your call. I like the NEWS entry, and it's informative even if factually it changed earlier or differently for different arches.
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>