From patchwork Tue Nov 26 21:13:11 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Simon Marchi (Code Review)" X-Patchwork-Id: 36255 Received: (qmail 112329 invoked by alias); 26 Nov 2019 21:13:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 112281 invoked by uid 89); 26 Nov 2019 21:13:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy= X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Nov 2019 21:13:51 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 8D82520413; Tue, 26 Nov 2019 16:13:49 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [IPv6:2620:52:3:1:5054:ff:fe06:16ca]) by mx1.osci.io (Postfix) with ESMTP id 8823520C7E; Tue, 26 Nov 2019 16:13:11 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 6CF5228173; Tue, 26 Nov 2019 16:13:11 -0500 (EST) X-Gerrit-PatchSet: 5 Date: Tue, 26 Nov 2019 16:13:11 -0500 From: "Sourceware to Gerrit sync (Code Review)" To: Tom Tromey , gdb-patches@sourceware.org Cc: Pedro Alves Auto-Submitted: auto-generated X-Gerrit-MessageType: merged Subject: [pushed] Introduce thread-safe way to handle SIGSEGV X-Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427 X-Gerrit-Change-Number: 171 X-Gerrit-ChangeURL: X-Gerrit-Commit: 3b3978bca2a204a772563c8e121e4a02be72e802 In-Reply-To: References: Reply-To: noreply@gnutoolchain-gerrit.osci.io, tromey@sourceware.org, palves@redhat.com, gdb-patches@sourceware.org MIME-Version: 1.0 Content-Disposition: inline User-Agent: Gerrit/3.0.3-79-g83ff7f88f1 Message-Id: <20191126211311.6CF5228173@gnutoolchain-gerrit.osci.io> Sourceware to Gerrit sync has submitted this change. Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/171 ...................................................................... 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-11-26 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. Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427 --- M gdb/ChangeLog M gdb/cp-support.c M gdb/event-top.c M gdb/event-top.h 4 files changed, 129 insertions(+), 71 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 80a3ea0..48e7ca1 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,18 @@ 2019-11-26 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-11-26 Tom Tromey + * run-on-main-thread.c: New file. * run-on-main-thread.h: New file. * unittests/main-thread-selftests.c: New file. diff --git a/gdb/cp-support.c b/gdb/cp-support.c index f1ddc74..4de2a98 100644 --- a/gdb/cp-support.c +++ b/gdb/cp-support.c @@ -38,6 +38,9 @@ #include "safe-ctype.h" #include "gdbsupport/selftest.h" #include "gdbsupport/gdb-sigmask.h" +#include +#include "event-top.h" +#include "run-on-main-thread.h" #define d_left(dc) (dc)->u.s_binary.left #define d_right(dc) (dc)->u.s_binary.right @@ -1476,11 +1479,11 @@ /* 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. */ +/* If true, 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 @@ 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,45 +1548,27 @@ 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; + scoped_restore restore_segv + = make_scoped_restore (&thread_local_segv_handler, + catch_demangler_crashes + ? gdb_demangle_signal_handler + : nullptr); - if (core_dump_allowed == -1) - { - core_dump_allowed = can_dump_core (LIMIT_CUR); - - if (!core_dump_allowed) - gdb_demangle_attempt_core_dump = 0; - } - + 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 - /* The signal handler may keep the signal blocked when we longjmp out of it. If we have sigprocmask, we can use it to unblock the signal afterwards and we can avoid the performance overhead of saving the signal mask just in case the signal gets triggered. Otherwise, just tell sigsetjmp to save the mask. */ #ifdef HAVE_SIGPROCMASK - crash_signal = SIGSETJMP (gdb_demangle_jmp_buf, 0); + crash_signal = SIGSETJMP (*gdb_demangle_jmp_buf, 0); #else - crash_signal = SIGSETJMP (gdb_demangle_jmp_buf, 1); + crash_signal = SIGSETJMP (*gdb_demangle_jmp_buf, 1); #endif } #endif @@ -1558,16 +1579,8 @@ #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; - + { #ifdef HAVE_SIGPROCMASK /* If we got the signal, SIGSEGV may still be blocked; restore it. */ sigset_t segv_sig_set; @@ -1576,35 +1589,18 @@ gdb_sigmask (SIG_UNBLOCK, &segv_sig_set, NULL); #endif - if (!error_reported) - { - std::string short_msg - = string_printf (_("unable to demangle '%s' " - "(demangler failed with signal %d)"), - name, crash_signal); + /* 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); + }); - 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; - } + result = NULL; + } } #endif @@ -2211,4 +2207,6 @@ selftests::register_test ("cp_remove_params", selftests::test_cp_remove_params); #endif + + gdb_demangle_attempt_core_dump = can_dump_core (LIMIT_CUR); } diff --git a/gdb/event-top.c b/gdb/event-top.c index 6c6e0ff..9ec599e 100644 --- a/gdb/event-top.c +++ b/gdb/event-top.c @@ -848,6 +848,45 @@ } +/* 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 @@ -915,6 +954,8 @@ 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 1dc7b13..e844125 100644 --- a/gdb/event-top.h +++ b/gdb/event-top.h @@ -70,4 +70,10 @@ 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