From 8c0fae46d6693fa34b0c102cc5872e02075678c8 Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Mon, 4 Mar 2019 15:12:04 -0700 Subject: Introduce thread-safe way to handle SIGSEGV The gdb demangler installs a SIGSEGV handler in order to protect gdb from demangler bugs. However, this is not thread-safe, as signal handlers are global to the process. This patch changes gdb to always install a global SIGSEGV handler, and then lets threads indicate their interest in handling the signal by setting a thread-local variable. This patch then arranges for the demangler code to use this; being sure to arrange for calls to warning and the like to be done on the main thread. One thing I wondered while writing this patch is if there are any systems that do not have "sigaction". If gdb could assume this, it would simplify this code. gdb/ChangeLog 2019-09-30 Tom Tromey * event-top.h (thread_local_segv_handler): Declare. * event-top.c (thread_local_segv_handler): New global. (install_handle_sigsegv, handle_sigsegv): New functions. (async_init_signals): Install SIGSEGV handler. * cp-support.c (gdb_demangle_jmp_buf): Change type. Now thread-local. (report_failed_demangle): New function. (gdb_demangle): Make core_dump_allowed atomic. Remove signal handler-setting code, instead use segv_handler. Run warning code on main thread. --- gdb/ChangeLog | 13 +++++ gdb/cp-support.c | 143 +++++++++++++++++++++++++++---------------------------- gdb/event-top.c | 41 ++++++++++++++++ gdb/event-top.h | 6 +++ 4 files changed, 130 insertions(+), 73 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index d0bd8af785c..402d5432425 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,16 @@ +2019-09-30 Tom Tromey + + * event-top.h (thread_local_segv_handler): Declare. + * event-top.c (thread_local_segv_handler): New global. + (install_handle_sigsegv, handle_sigsegv): New functions. + (async_init_signals): Install SIGSEGV handler. + * cp-support.c (gdb_demangle_jmp_buf): Change type. Now + thread-local. + (report_failed_demangle): New function. + (gdb_demangle): Make core_dump_allowed atomic. Remove signal + handler-setting code, instead use segv_handler. Run warning code + on main thread. + 2019-09-30 Tom Tromey * unittests/main-thread-selftests.c: New file. diff --git a/gdb/cp-support.c b/gdb/cp-support.c index cd732b60e7d..8fe9ea161f9 100644 --- a/gdb/cp-support.c +++ b/gdb/cp-support.c @@ -37,6 +37,9 @@ #include "gdbsupport/gdb_setjmp.h" #include "safe-ctype.h" #include "gdbsupport/selftest.h" +#include +#include "event-top.h" +#include "ser-event.h" #define d_left(dc) (dc)->u.s_binary.left #define d_right(dc) (dc)->u.s_binary.right @@ -1476,11 +1479,11 @@ static bool catch_demangler_crashes = true; /* Stack context and environment for demangler crash recovery. */ -static SIGJMP_BUF gdb_demangle_jmp_buf; +static thread_local SIGJMP_BUF *gdb_demangle_jmp_buf; /* If nonzero, attempt to dump core from the signal handler. */ -static int gdb_demangle_attempt_core_dump = 1; +static std::atomic gdb_demangle_attempt_core_dump; /* Signal handler for gdb_demangle. */ @@ -1492,10 +1495,46 @@ gdb_demangle_signal_handler (int signo) if (fork () == 0) dump_core (); - gdb_demangle_attempt_core_dump = 0; + gdb_demangle_attempt_core_dump = false; } - SIGLONGJMP (gdb_demangle_jmp_buf, signo); + SIGLONGJMP (*gdb_demangle_jmp_buf, signo); +} + +/* A helper for gdb_demangle that reports a demangling failure. */ + +static void +report_failed_demangle (const char *name, bool core_dump_allowed, + int crash_signal) +{ + static bool error_reported = false; + + if (!error_reported) + { + std::string short_msg + = string_printf (_("unable to demangle '%s' " + "(demangler failed with signal %d)"), + name, crash_signal); + + std::string long_msg + = string_printf ("%s:%d: %s: %s", __FILE__, __LINE__, + "demangler-warning", short_msg.c_str ()); + + target_terminal::scoped_restore_terminal_state term_state; + target_terminal::ours_for_output (); + + begin_line (); + if (core_dump_allowed) + fprintf_unfiltered (gdb_stderr, + _("%s\nAttempting to dump core.\n"), + long_msg.c_str ()); + else + warn_cant_dump_core (long_msg.c_str ()); + + demangler_warning (__FILE__, __LINE__, "%s", short_msg.c_str ()); + + error_reported = true; + } } #endif @@ -1509,38 +1548,18 @@ gdb_demangle (const char *name, int options) int crash_signal = 0; #ifdef HAVE_WORKING_FORK -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) - struct sigaction sa, old_sa; -#else - sighandler_t ofunc; -#endif - static int core_dump_allowed = -1; - - if (core_dump_allowed == -1) - { - core_dump_allowed = can_dump_core (LIMIT_CUR); - - if (!core_dump_allowed) - gdb_demangle_attempt_core_dump = 0; - } - + scoped_restore restore_segv + = make_scoped_restore (&thread_local_segv_handler, + catch_demangler_crashes + ? gdb_demangle_signal_handler + : nullptr); + + bool core_dump_allowed = gdb_demangle_attempt_core_dump; + SIGJMP_BUF jmp_buf; + scoped_restore restore_jmp_buf + = make_scoped_restore (&gdb_demangle_jmp_buf, &jmp_buf); if (catch_demangler_crashes) - { -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) - sa.sa_handler = gdb_demangle_signal_handler; - sigemptyset (&sa.sa_mask); -#ifdef HAVE_SIGALTSTACK - sa.sa_flags = SA_ONSTACK; -#else - sa.sa_flags = 0; -#endif - sigaction (SIGSEGV, &sa, &old_sa); -#else - ofunc = signal (SIGSEGV, gdb_demangle_signal_handler); -#endif - - crash_signal = SIGSETJMP (gdb_demangle_jmp_buf); - } + crash_signal = SIGSETJMP (jmp_buf); #endif if (crash_signal == 0) @@ -1549,45 +1568,20 @@ gdb_demangle (const char *name, int options) #ifdef HAVE_WORKING_FORK if (catch_demangler_crashes) { -#if defined (HAVE_SIGACTION) && defined (SA_RESTART) - sigaction (SIGSEGV, &old_sa, NULL); -#else - signal (SIGSEGV, ofunc); -#endif - if (crash_signal != 0) - { - static int error_reported = 0; - - if (!error_reported) - { - std::string short_msg - = string_printf (_("unable to demangle '%s' " - "(demangler failed with signal %d)"), - name, crash_signal); - - std::string long_msg - = string_printf ("%s:%d: %s: %s", __FILE__, __LINE__, - "demangler-warning", short_msg.c_str ()); - - target_terminal::scoped_restore_terminal_state term_state; - target_terminal::ours_for_output (); - - begin_line (); - if (core_dump_allowed) - fprintf_unfiltered (gdb_stderr, - _("%s\nAttempting to dump core.\n"), - long_msg.c_str ()); - else - warn_cant_dump_core (long_msg.c_str ()); - - demangler_warning (__FILE__, __LINE__, "%s", short_msg.c_str ()); - - error_reported = 1; - } - - result = NULL; - } + { + /* If there was a failure, we can't report it here, because + we might be in a background thread. Instead, arrange for + the reporting to happen on the main thread. */ + std::string copy = name; + run_on_main_thread ([=] () + { + report_failed_demangle (copy.c_str (), core_dump_allowed, + crash_signal); + }); + + result = NULL; + } } #endif @@ -2194,4 +2188,7 @@ display the offending symbol."), selftests::register_test ("cp_remove_params", selftests::test_cp_remove_params); #endif + + if (!can_dump_core (LIMIT_CUR)) + gdb_demangle_attempt_core_dump = false; } diff --git a/gdb/event-top.c b/gdb/event-top.c index 0b05b2f85a5..ad9e3ff4d9c 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -847,6 +847,45 @@ gdb_readline_no_editing_callback (gdb_client_data client_data) } +/* See event-top.h. */ + +thread_local void (*thread_local_segv_handler) (int); + +static void handle_sigsegv (int sig); + +/* Install the SIGSEGV handler. */ +static void +install_handle_sigsegv () +{ +#if defined (HAVE_SIGACTION) + struct sigaction sa; + sa.sa_handler = handle_sigsegv; + sigemptyset (&sa.sa_mask); +#ifdef HAVE_SIGALTSTACK + sa.sa_flags = SA_ONSTACK; +#else + sa.sa_flags = 0; +#endif + sigaction (SIGSEGV, &sa, nullptr); +#else + signal (SIGSEGV, handle_sigsegv); +#endif +} + +/* Handler for SIGSEGV. */ + +static void +handle_sigsegv (int sig) +{ + install_handle_sigsegv (); + + if (thread_local_segv_handler == nullptr) + abort (); + thread_local_segv_handler (sig); +} + + + /* The serial event associated with the QUIT flag. set_quit_flag sets this, and check_quit_flag clears it. Used by interruptible_select to be able to do interruptible I/O with no race with the SIGINT @@ -914,6 +953,8 @@ async_init_signals (void) sigtstp_token = create_async_signal_handler (async_sigtstp_handler, NULL); #endif + + install_handle_sigsegv (); } /* See defs.h. */ diff --git a/gdb/event-top.h b/gdb/event-top.h index 1dc7b13d4f8..e844125cbbf 100644 --- a/gdb/event-top.h +++ b/gdb/event-top.h @@ -70,4 +70,10 @@ extern void gdb_rl_callback_handler_install (const char *prompt); currently installed. */ extern void gdb_rl_callback_handler_reinstall (void); +/* The SIGSEGV handler for this thread, or NULL if there is none. GDB + always installs a global SIGSEGV handler, and then lets threads + indicate their interest in handling the signal by setting this + thread-local variable. */ +extern thread_local void (*thread_local_segv_handler) (int); + #endif -- cgit v1.2.1