[PATCH v3] Fix signal unsafe call inside a signal

[Andrew Burgess]

Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

Hi Bernd,

Thanks for the new version, and for the feedback on the v2 review.  I do
have a few more thoughts...

> As mentioned in
> https://sourceware.org/bugzilla/show_bug.cgi?id=31713#c9
> it can easily happen that the signal handler function
> handle_fatal_signal() uses various signal unsafe functions.
> Fix that by pre-computing the necessary language specific
> strings.

I think this commit message would be improved by mentioning the
problematic functions ... it's only `_` and `strsignal` right?  The bug
can still be referenced, ideally with a:

  Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31713#c9

at the end of the commit message.  But it's nice if the commit message
doesn't just say: "go read here for details", but instead contains most
of the necessary details.

> ---
>  gdb/bt-utils.c  | 24 ++++++++++++--
>  gdb/bt-utils.h  |  4 +++
>  gdb/event-top.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-----
>  gdb/main.c      |  4 +++
>  4 files changed, 106 insertions(+), 10 deletions(-)
>
> v2: moved initalization of language specific string to an init function
> v3: addressed review comments
>
> diff --git a/gdb/bt-utils.c b/gdb/bt-utils.c
> index f658ce0d4bc..787f425625c 100644
> --- a/gdb/bt-utils.c
> +++ b/gdb/bt-utils.c
> @@ -123,6 +123,8 @@ gdb_internal_backtrace_1 ()
>  
>  #elif defined GDB_PRINT_INTERNAL_BACKTRACE_USING_EXECINFO
>  
> +static const char *str_backtrace_incomplete;
> +
>  /* See the comment on previous version of this function.  */
>  
>  static void
> @@ -139,12 +141,28 @@ gdb_internal_backtrace_1 ()
>  
>    backtrace_symbols_fd (buffer, frames, gdb_stderr->fd ());
>    if (frames == ARRAY_SIZE (buffer))
> -    sig_write (_("Backtrace might be incomplete.\n"));
> +    sig_write (str_backtrace_incomplete);
>  }
>  
>  #else
>  #error "unexpected internal backtrace policy"
>  #endif
> +
> +static const char *str_backtrace;
> +static const char *str_backtrace_unavailable;
> +
> +/* Initialize language specific strings.  */
> +
> +void
> +init_str_internal_backtrace ()
> +{
> +  str_backtrace = _("----- Backtrace -----\n");
> +  str_backtrace_unavailable = _("Backtrace unavailable\n");
> +#ifdef GDB_PRINT_INTERNAL_BACKTRACE_USING_EXECINFO
> +  str_backtrace_incomplete = _("Backtrace might be incomplete.\n");
> +#endif
> +}
> +
>  #endif /* GDB_PRINT_INTERNAL_BACKTRACE */
>  
>  /* See bt-utils.h.  */
> @@ -161,12 +179,12 @@ gdb_internal_backtrace ()
>      gdb_stderr->write_async_safe (msg, strlen (msg));
>    };
>  
> -  sig_write (_("----- Backtrace -----\n"));
> +  sig_write (str_backtrace);

I think before this line you can add:

  gdb_assert (str_backtrace != nullptr);

If we go back to the example you give in the v2 thread, where an
internal_error() call after current_ui is set, but before the strings
are set, previously (as you point out) this would result in a segfault,
now it gives:

  ../../src.dev-h/gdb/main.c:681: internal-error: foo
  A problem internal to GDB has been detected,
  further debugging may prove unreliable.
  Recursive internal problem.
  Aborted (core dumped)

The initial internal_error() is still reported, and GDB no longer
segfaults.  Possibly we should gdb_assert() that all the strings are
set?  Though given they are all setup together, just checking the first
is likely enough.

>  
>    if (gdb_stderr->fd () > -1)
>      gdb_internal_backtrace_1 ();
>    else
> -    sig_write (_("Backtrace unavailable\n"));
> +    sig_write (str_backtrace_unavailable);
>  
>    sig_write ("---------------------\n");
>  #endif
> diff --git a/gdb/bt-utils.h b/gdb/bt-utils.h
> index ec2d14a5484..b6a7ef5464d 100644
> --- a/gdb/bt-utils.h
> +++ b/gdb/bt-utils.h
> @@ -71,4 +71,8 @@ extern void gdb_internal_backtrace ();
>  extern void gdb_internal_backtrace_set_cmd (const char *args, int from_tty,
>  					    cmd_list_element *c);
>  
> +#ifdef GDB_PRINT_INTERNAL_BACKTRACE
> +extern void init_str_internal_backtrace ();
> +#endif

I think the function should be renamed as
`gdb_internal_backtrace_init_str` as this follows the naming convention
of all the other functions exported by this file.

But more important, GDB generally tries to avoid using #ifdef in "other"
parts of GDB to control calls into components.  So in this case, you use
the '#ifdef GDB_PRINT_INTERNAL_BACKTRACE' in main.c to avoid calling the
new init_str_internal_backtrace function in the case where you know that
the call is not needed.

I think the more GDB style would be to place the #idef check inside the
init_str_internal_backtrace function, and then make the call in main.c
unguarded.  The run time cost is minimal, this is a single call in
startup code.

Thanks,
Andrew
> +
>  #endif /* BT_UTILS_H */
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index b81970d11bb..1ef2eacfe94 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -892,6 +892,51 @@ unblock_signal (int sig)
>    return false;
>  }
>  
> +/* Signal safe language specific strings.  */
> +
> +#ifdef GDB_PRINT_INTERNAL_BACKTRACE
> +static const char *str_fatal_signal;
> +static const char *str_sigsegv;
> +#ifdef SIGFPE
> +static const char *str_sigfpe;
> +#endif
> +#ifdef SIGBUS
> +static const char *str_sigbus;
> +#endif
> +#ifdef SIGABRT
> +static const char *str_sigabrt;
> +#endif
> +static const char *str_unknown_signal;
> +static const char *str_fatal_error_detected_gdb_will_now_terminate;
> +static const char *str_this_is_a_bug;
> +static const char *str_for_instructions_see;
> +
> +/* Initialize language specific strings.  */
> +
> +static void
> +init_str_handle_fatal_signal ()
> +{
> +  str_fatal_signal = _("Fatal signal: ");
> +  str_sigsegv = strsignal (SIGSEGV);
> +#ifdef SIGFPE
> +  str_sigfpe = strsignal (SIGFPE);
> +#endif
> +#ifdef SIGBUS
> +  str_sigbus = strsignal (SIGBUS);
> +#endif
> +#ifdef SIGABRT
> +  str_sigabrt = strsignal (SIGABRT);
> +#endif
> +  str_unknown_signal = _("Unknown signal");
> +  str_fatal_error_detected_gdb_will_now_terminate =
> +	_("A fatal error internal to GDB has been detected, "
> +	  "further\ndebugging is not possible.  GDB will now "
> +	  "terminate.\n\n");
> +  str_this_is_a_bug = _("This is a bug, please report it.");
> +  str_for_instructions_see = _("  For instructions, see:\n");
> +}
> +#endif
> +
>  /* Called to handle fatal signals.  SIG is the signal number.  */
>  
>  static void ATTRIBUTE_NORETURN
> @@ -910,19 +955,40 @@ handle_fatal_signal (int sig)
>    if (bt_on_fatal_signal)
>      {
>        sig_write ("\n\n");
> -      sig_write (_("Fatal signal: "));
> -      sig_write (strsignal (sig));
> +      sig_write (str_fatal_signal);
> +      switch (sig)
> +	{
> +	case SIGSEGV:
> +	  sig_write (str_sigsegv);
> +	  break;
> +#ifdef SIGFPE
> +	case SIGFPE:
> +	  sig_write (str_sigfpe);
> +	  break;
> +#endif
> +#ifdef SIGBUS
> +	case SIGBUS:
> +	  sig_write (str_sigbus);
> +	  break;
> +#endif
> +#ifdef SIGABRT
> +	case SIGABRT:
> +	  sig_write (str_sigabrt);
> +	  break;
> +#endif
> +	default:
> +	  sig_write (str_unknown_signal);
> +	  break;
> +	}
>        sig_write ("\n");
>  
>        gdb_internal_backtrace ();
>  
> -      sig_write (_("A fatal error internal to GDB has been detected, "
> -		   "further\ndebugging is not possible.  GDB will now "
> -		   "terminate.\n\n"));
> -      sig_write (_("This is a bug, please report it."));
> +      sig_write (str_fatal_error_detected_gdb_will_now_terminate);
> +      sig_write (str_this_is_a_bug);
>        if (REPORT_BUGS_TO[0] != '\0')
>  	{
> -	  sig_write (_("  For instructions, see:\n"));
> +	  sig_write (str_for_instructions_see);
>  	  sig_write (REPORT_BUGS_TO);
>  	  sig_write (".");
>  	}
> @@ -1050,6 +1116,10 @@ gdb_init_signals (void)
>      create_async_signal_handler (async_sigtstp_handler, NULL, "sigtstp");
>  #endif
>  
> +#ifdef GDB_PRINT_INTERNAL_BACKTRACE
> +  init_str_handle_fatal_signal ();
> +#endif
> +
>  #ifdef SIGFPE
>    signal (SIGFPE, handle_fatal_signal);
>  #endif
> diff --git a/gdb/main.c b/gdb/main.c
> index 4dd68f3d976..213490e419c 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -58,6 +58,7 @@
>  #include "observable.h"
>  #include "serial.h"
>  #include "cli-out.h"
> +#include "bt-utils.h"
>  
>  /* The selected interpreter.  */
>  std::string interpreter_p;
> @@ -676,6 +677,9 @@ captured_main_1 (struct captured_main_args *context)
>    /* Note: `error' cannot be called before this point, because the
>       caller will crash when trying to print the exception.  */
>    main_ui = new ui (stdin, stdout, stderr);
> +#ifdef GDB_PRINT_INTERNAL_BACKTRACE
> +  init_str_internal_backtrace ();
> +#endif
>    current_ui = main_ui;
>  
>    gdb_stdtarg = gdb_stderr;
> -- 
> 2.39.2