[review] Introduce thread-safe way to handle SIGSEGV

[review] Introduce thread-safe way to handle SIGSEGV

[Tom Tromey (Code Review)]

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-10-19  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.

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(+), 70 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2abad21..f04d2bd 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,18 @@
 2019-10-19  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-10-19  Tom Tromey  <tom@tromey.com>
+
 	* unittests/main-thread-selftests.c: New file.
 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
 	main-thread-selftests.c.
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index c298d26..37937b4 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 <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
@@ -1477,11 +1480,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.  */
 
-static int gdb_demangle_attempt_core_dump = 1;
+static std::atomic<bool> gdb_demangle_attempt_core_dump;
 
 /* Signal handler for gdb_demangle.  */
 
@@ -1493,10 +1496,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
@@ -1510,45 +1549,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
@@ -1559,16 +1580,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;
@@ -1577,35 +1590,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
 
@@ -2212,4 +2208,7 @@
   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 6c6e0ff..9ec599e 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -848,6 +848,45 @@
 }
 \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
@@ -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

[review v3] Introduce thread-safe way to handle SIGSEGV

[Tom Tromey (Code Review)]

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.

2019-10-19  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.

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(+), 70 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 4992399..1cc0b83 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,18 @@
 2019-10-19  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-10-19  Tom Tromey  <tom@tromey.com>
+
 	* unittests/main-thread-selftests.c: New file.
 	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
 	main-thread-selftests.c.
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index a315a53..9c7a190 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 <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
@@ -1477,11 +1480,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.  */
 
-static int gdb_demangle_attempt_core_dump = 1;
+static std::atomic<bool> gdb_demangle_attempt_core_dump;
 
 /* Signal handler for gdb_demangle.  */
 
@@ -1493,10 +1496,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
@@ -1510,45 +1549,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
@@ -1559,16 +1580,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;
@@ -1577,35 +1590,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
 
@@ -2212,4 +2208,7 @@
   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 6c6e0ff..9ec599e 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -848,6 +848,45 @@
 }
 \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
@@ -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

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[review v3] Introduce thread-safe way to handle SIGSEGV

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/171
......................................................................


Patch Set 3:

(2 comments)

| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1474,20 +1477,20 @@ /* If true, attempt to catch crashes in the demangler and print
|     useful debugging information.  */
|  
|  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.  */

PS3, Line 1485:

"If true,"

|  
| -static int gdb_demangle_attempt_core_dump = 1;
| +static std::atomic<bool> gdb_demangle_attempt_core_dump;

PS3, Line 1487:

Is there anything ever initializing this to true?

Makes me ponder about a maintenance command to force a SIGSEGV.

|  
|  /* Signal handler for gdb_demangle.  */
|  
|  static void
|  gdb_demangle_signal_handler (int signo)
|  {
|    if (gdb_demangle_attempt_core_dump)
|      {
|        if (fork () == 0)

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Fri, 22 Nov 2019 20:07:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v3] Introduce thread-safe way to handle SIGSEGV

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/171
......................................................................


Patch Set 3:

(1 comment)

| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1477,17 +1480,17 @@ 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;

PS3, Line 1487:

> Is there anything ever initializing this to true?

Oops, nope.  I fixed this.

> Makes me ponder about a maintenance command to force a SIGSEGV.

I think there is one actually.
I don't know if I want to try to unit test this or not.
The current problem under discussion is maybe hard to check.

What would be really great is to fuzz the demangler and fix all
the bugs.  Then we could ditch all of this code

|  
|  /* Signal handler for gdb_demangle.  */
|  
|  static void
|  gdb_demangle_signal_handler (int signo)
|  {
|    if (gdb_demangle_attempt_core_dump)
|      {
|        if (fork () == 0)

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 3
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-Comment-Date: Fri, 22 Nov 2019 23:12:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment

[review v4] Introduce thread-safe way to handle SIGSEGV

[Tom Tromey (Code Review)]

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.

2019-10-19  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.

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 901adeb..6da3318 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,16 @@
+2019-10-19  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-11-22  Tom Tromey  <tom@tromey.com>
 
 	* run-on-main-thread.c: New file.
diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index d550929..e24e85b 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 <atomic>
+#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<bool> 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 @@
 }
 \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
@@ -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

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-CC: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: newpatchset

[review v4] Introduce thread-safe way to handle SIGSEGV

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/171
......................................................................


Patch Set 4: Code-Review+2

(2 comments)

LGTM, with the nit below fixed.

| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1477,17 +1480,17 @@ 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;

PS3, Line 1487:

> What would be really great is to fuzz the demangler and fix all
> the bugs.  Then we could ditch all of this code

One can dream.  :-)

|  
|  /* Signal handler for gdb_demangle.  */
|  
|  static void
|  gdb_demangle_signal_handler (int signo)
|  {
|    if (gdb_demangle_attempt_core_dump)
|      {
|        if (fork () == 0)
| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1602,7 +1592,13 @@ #endif
| -
| -	      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;

PS4, Line 1595:

(I thought I had commented on this, but I can't find the comment now.)

This explicit copy is unnecessary/redundant, because you're using [=]
as capture.  As is, you copy the string twice, first into the
"std::string copy" local, and then you copy that again in the lambda's
capture by value.

| +          run_on_main_thread ([=] ()
| +            {
| +              report_failed_demangle (copy.c_str (), core_dump_allowed,
| +                                      crash_signal);
| +            });
| +
| +          result = NULL;
| +        }
|      }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 15:57:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment

[review v4] Introduce thread-safe way to handle SIGSEGV

[Tom Tromey (Code Review)]

Tom Tromey has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/171
......................................................................


Patch Set 4:

(1 comment)

| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1602,7 +1592,13 @@ #endif
| -
| -	      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;

PS4, Line 1595:

> (I thought I had commented on this, but I can't find the comment now.)
> 
> This explicit copy is unnecessary/redundant, because you're using [=] as capture.  As is, you copy the string twice, first into the "std::string copy" local, and then you copy that again in the lambda's capture by value.

You did comment earlier, on gdb-patches I think.

The copy is needed here because `name` is a `const char *` and
it isn't guaranteed to live long enough.

| +          run_on_main_thread ([=] ()
| +            {
| +              report_failed_demangle (copy.c_str (), core_dump_allowed,
| +                                      crash_signal);
| +            });
| +
| +          result = NULL;
| +        }
|      }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 16:11:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment

[review v4] Introduce thread-safe way to handle SIGSEGV

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/171
......................................................................


Patch Set 4:

(1 comment)

| --- gdb/cp-support.c
| +++ gdb/cp-support.c
| @@ -1602,7 +1592,13 @@ #endif
| -
| -	      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;

PS4, Line 1595:

> The copy is needed here because `name` is a `const char *` and
> it isn't guaranteed to live long enough.

Ah, missed that.  Thanks for the clarification.

| +          run_on_main_thread ([=] ()
| +            {
| +              report_failed_demangle (copy.c_str (), core_dump_allowed,
| +                                      crash_signal);
| +            });
| +
| +          result = NULL;
| +        }
|      }

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 4
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 16:23:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment

[pushed] Introduce thread-safe way to handle SIGSEGV

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Tom Tromey.

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

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  <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-11-26  Tom Tromey  <tom@tromey.com>
+
 	* 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 <atomic>
+#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<bool> 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 @@
 }
 \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
@@ -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

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 5
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset

[pushed] Introduce thread-safe way to handle SIGSEGV

[Sourceware to Gerrit sync (Code Review)]

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

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  <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-11-26  Tom Tromey  <tom@tromey.com>
+
 	* 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 <atomic>
+#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<bool> 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 @@
 }
 \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
@@ -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

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: Ic832bbb033b64744e4b44f14b41db7e4168ce427
Gerrit-Change-Number: 171
Gerrit-PatchSet: 5
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged