[v3,6/8] 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 thread 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.

gdb/ChangeLog
2019-05-29  Tom Tromey  <tom@tromey.com>

	* event-top.h (segv_handler): Declare.
	* event-top.c (segv_handler): New global.
	(handle_sigsegv): New function.
	(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 | 114 +++++++++++++++++++++++------------------------
 gdb/event-top.c  |  37 +++++++++++++++
 gdb/event-top.h  |   3 ++
 4 files changed, 109 insertions(+), 58 deletions(-)
  

On 5/29/19 10:29 PM, Tom Tromey wrote:
> 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 thread indicate their interest in handling the signal by

"lets threads"

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

The design is explained here, but the patch itself doesn't include
any comment in that direction.  I think future readers of the code
would benefit a lot such commentary.

> 
> gdb/ChangeLog
> 2019-05-29  Tom Tromey  <tom@tromey.com>
> 
> 	* event-top.h (segv_handler): Declare.
> 	* event-top.c (segv_handler): New global.
> 	(handle_sigsegv): New function.
> 	(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 | 114 +++++++++++++++++++++++------------------------
>  gdb/event-top.c  |  37 +++++++++++++++
>  gdb/event-top.h  |   3 ++
>  4 files changed, 109 insertions(+), 58 deletions(-)
> 
> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> index 4afb79a4ea9..60bbbc23ec3 100644
> --- a/gdb/cp-support.c
> +++ b/gdb/cp-support.c
> @@ -37,6 +37,9 @@
>  #include "common/gdb_setjmp.h"
>  #include "safe-ctype.h"
>  #include "common/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
> @@ -1480,7 +1483,7 @@ static int catch_demangler_crashes = 1;
>  
>  /* 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.  */
>  
> @@ -1499,7 +1502,43 @@ gdb_demangle_signal_handler (int signo)
>        gdb_demangle_attempt_core_dump = 0;
>      }
>  
> -  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, int core_dump_allowed,
> +			int crash_signal)
> +{
> +  static int error_reported = 0;

bool, while at it?

> +
> +  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;
> +    }
>  }
>  
>  #endif
> @@ -1513,12 +1552,7 @@ 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;
> +  static std::atomic<int> core_dump_allowed (-1);
>  
>    if (core_dump_allowed == -1)
>      {
> @@ -1528,23 +1562,15 @@ gdb_demangle (const char *name, int options)
>  	gdb_demangle_attempt_core_dump = 0;

Are accesses to gdb_demangle_attempt_core_dump going to be racy?
Maybe not, didn't think that much about it.  If they are, I wonder whether it
wouldn't be better if this whole can_dump_core check block were moved to the
main thread, before threads are spun?  Say, to a gdb_demangle_init() function?
Not sure, might not be convenient form an abstraction-hiding perspective.

>      }
>  
> -  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
> +  scoped_restore restore_segv
> +    = make_scoped_restore (&segv_handler,
> +			   catch_demangler_crashes
> +			   ? gdb_demangle_signal_handler
> +			   : nullptr);
>  
> -      crash_signal = SIGSETJMP (gdb_demangle_jmp_buf);
> -    }
> +  SIGJMP_BUF jmp_buf;
> +  if (catch_demangler_crashes)
> +    crash_signal = SIGSETJMP (jmp_buf);
>  #endif

gdb_demangle_jmp_buf is now a pointer, but I'm not seeing
anything set it?

>  
>    if (crash_signal == 0)
> @@ -1553,42 +1579,14 @@ 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;
> -	    }
> +	  std::string copy = name;

I don't think you need this copy, because ...

> +	  run_on_main_thread ([=] ()

... you're already copying here, with "[=]".

> +            {

tabs vs spaces.

> +	      report_failed_demangle (copy.c_str (), core_dump_allowed,
> +				      crash_signal);
> +	    });
>  
>  	  result = NULL;
>  	}
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index 93b7d2d28bc..f1466c0c2ba 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -849,6 +849,29 @@ gdb_readline_no_editing_callback (gdb_client_data client_data)
>  }
>  
>  
> +/* See event-top.h.  */
> +
> +thread_local void (*segv_handler) (int);

I think it'd help readability if this were named something
that indicates it's thread-local, like e.g., thr_segv_handler.

> +
> +/* Handler for SIGSEGV.  */
> +
> +static void
> +handle_sigsegv (int sig)
> +{
> +  if (segv_handler == nullptr)
> +    {
> +      signal (sig, nullptr);
> +      raise (sig);
> +    }
> +  else
> +    {
> +      signal (sig, handle_sigsegv);

Doesn't this lose the SA_ONSTACK?

> +      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
> @@ -916,6 +939,20 @@ async_init_signals (void)
>    sigtstp_token =
>      create_async_signal_handler (async_sigtstp_handler, NULL);
>  #endif
> +
> +#if defined (HAVE_SIGACTION) && defined (SA_RESTART)
> +  struct sigaction sa, old_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, &old_sa);
> +#else
> +  signal (SIGSEGV, handle_sigsegv);
> +#endif
>  }
>  
>  /* See defs.h.  */
> diff --git a/gdb/event-top.h b/gdb/event-top.h
> index 4c3e6cb8612..849f40d9192 100644
> --- a/gdb/event-top.h
> +++ b/gdb/event-top.h
> @@ -70,4 +70,7 @@ 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.  */
> +extern thread_local void (*segv_handler) (int);
> +
>  #endif
> 

Thanks,
Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

>> +      signal (sig, handle_sigsegv);

Pedro> Doesn't this lose the SA_ONSTACK?

Yeah.  I'm glad you caught this since it points out that we have to call
sigaltstack per-thread.

Tom