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  <tom@tromey.com>

	* 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.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index d0bd8af785ce384c28119ee461cd137dd2145ddf..402d5432425c7eac9ca2dfcf66302146ab1c0101 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2019-09-30  Tom Tromey  <tom@tromey.com>
+
+       * 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  <tom@tromey.com>
 
        * unittests/main-thread-selftests.c: New file.

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index cd732b60e7d9c6a7d89588dada4666756d161794..8fe9ea161f9f471e33cb2a900a247a75b8ac9df9 100644 (file)
--- 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 <atomic>
+#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<bool> 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 0b05b2f85a52e6ca0a5c79885f8ed30bcf24e766..ad9e3ff4d9c2f8d623c862e6331214ba278ba303 100644 (file)
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -847,6 +847,45 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
 }
 \f
 
+/* 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);
+}
+
+\f
+
 /* 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 1dc7b13d4f8adb5f515fed5114bf4f04628cd46b..e844125cbbf2f1204227ecb6f02ca0e9675606e7 100644 (file)
--- 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