[v4] Fix signal unsafe call inside a signal

Message ID DU2PR08MB10263AA976BA58BEDFCA269D4E44A2@DU2PR08MB10263.eurprd08.prod.outlook.com
State New
Headers
Series [v4] Fix signal unsafe call inside a signal |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Bernd Edlinger Oct. 28, 2024, 4:47 a.m. UTC
  It can easily happen that the signal handler function
`handle_fatal_signal` uses various signal unsafe functions.
The problematic functions are `_` and `strsignal` which
can be pre-computed after the `setlocale` call is done.

Unfortunately when compiled with --disable-libbacktrace a
different code path is used, that calls the glibc function
`backtrace` which calls `malloc` and `free` and is therefore
also signal unsafe, that is probably unfixable, so there
is no attempt to fix anything in this code path.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31713#c9
---
 gdb/bt-utils.c  | 19 +++++++++--
 gdb/bt-utils.h  |  2 ++
 gdb/event-top.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-----
 gdb/main.c      |  2 ++
 4 files changed, 98 insertions(+), 9 deletions(-)

v2: moved initalization of language specific string to an init function
v3: addressed review comments
v4: improved commit message, addressed review commments
  

Comments

Andrew Burgess Oct. 29, 2024, 10:27 a.m. UTC | #1
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:

> It can easily happen that the signal handler function
> `handle_fatal_signal` uses various signal unsafe functions.
> The problematic functions are `_` and `strsignal` which
> can be pre-computed after the `setlocale` call is done.
>
> Unfortunately when compiled with --disable-libbacktrace a
> different code path is used, that calls the glibc function
> `backtrace` which calls `malloc` and `free` and is therefore
> also signal unsafe, that is probably unfixable, so there
> is no attempt to fix anything in this code path.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31713#c9
> ---
>  gdb/bt-utils.c  | 19 +++++++++--
>  gdb/bt-utils.h  |  2 ++
>  gdb/event-top.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-----
>  gdb/main.c      |  2 ++
>  4 files changed, 98 insertions(+), 9 deletions(-)
>
> v2: moved initalization of language specific string to an init function
> v3: addressed review comments
> v4: improved commit message, addressed review commments
>  
> diff --git a/gdb/bt-utils.c b/gdb/bt-utils.c
> index f658ce0d4bc..fb006c29178 100644
> --- a/gdb/bt-utils.c
> +++ b/gdb/bt-utils.c
> @@ -145,8 +145,23 @@ gdb_internal_backtrace_1 ()
>  #else
>  #error "unexpected internal backtrace policy"
>  #endif
> +
> +static const char *str_backtrace = "----- Backtrace -----\n";
> +static const char *str_backtrace_unavailable = "Backtrace unavailable\n";
> +
>  #endif /* GDB_PRINT_INTERNAL_BACKTRACE */
>  
> +/* Initialize language specific strings.  */

Move this comment into the header file please.  Then place a comment
here that just says:

  /* See bt-utils.h.  */

With that change made:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> +
> +void
> +gdb_internal_backtrace_init_str ()
> +{
> +#ifdef GDB_PRINT_INTERNAL_BACKTRACE
> +  str_backtrace = _("----- Backtrace -----\n");
> +  str_backtrace_unavailable = _("Backtrace unavailable\n");
> +#endif
> +}
> +
>  /* See bt-utils.h.  */
>  
>  void
> @@ -161,12 +176,12 @@ gdb_internal_backtrace ()
>      gdb_stderr->write_async_safe (msg, strlen (msg));
>    };
>  
> -  sig_write (_("----- Backtrace -----\n"));
> +  sig_write (str_backtrace);
>  
>    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..6fe7c8c4f3b 100644
> --- a/gdb/bt-utils.h
> +++ b/gdb/bt-utils.h
> @@ -71,4 +71,6 @@ extern void gdb_internal_backtrace ();
>  extern void gdb_internal_backtrace_set_cmd (const char *args, int from_tty,
>  					    cmd_list_element *c);
>  
> +extern void gdb_internal_backtrace_init_str ();
> +
>  #endif /* BT_UTILS_H */
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index d3cf144958a..e7556f71453 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -908,6 +908,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.  */
>  
>  [[noreturn]] static void
> @@ -926,19 +971,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 (".");
>  	}
> @@ -1066,6 +1132,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 e4a40c51023..4370e95ada4 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,7 @@ 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);
> +  gdb_internal_backtrace_init_str ();
>    current_ui = main_ui;
>  
>    gdb_stdtarg = gdb_stderr;
> -- 
> 2.39.2
  
Bernd Edlinger Oct. 29, 2024, 3:05 p.m. UTC | #2
On 10/29/24 11:27, Andrew Burgess wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 
>> It can easily happen that the signal handler function
>> `handle_fatal_signal` uses various signal unsafe functions.
>> The problematic functions are `_` and `strsignal` which
>> can be pre-computed after the `setlocale` call is done.
>>
>> Unfortunately when compiled with --disable-libbacktrace a
>> different code path is used, that calls the glibc function
>> `backtrace` which calls `malloc` and `free` and is therefore
>> also signal unsafe, that is probably unfixable, so there
>> is no attempt to fix anything in this code path.
>>
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31713#c9
>> ---
>>  gdb/bt-utils.c  | 19 +++++++++--
>>  gdb/bt-utils.h  |  2 ++
>>  gdb/event-top.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-----
>>  gdb/main.c      |  2 ++
>>  4 files changed, 98 insertions(+), 9 deletions(-)
>>
>> v2: moved initalization of language specific string to an init function
>> v3: addressed review comments
>> v4: improved commit message, addressed review commments
>>  
>> diff --git a/gdb/bt-utils.c b/gdb/bt-utils.c
>> index f658ce0d4bc..fb006c29178 100644
>> --- a/gdb/bt-utils.c
>> +++ b/gdb/bt-utils.c
>> @@ -145,8 +145,23 @@ gdb_internal_backtrace_1 ()
>>  #else
>>  #error "unexpected internal backtrace policy"
>>  #endif
>> +
>> +static const char *str_backtrace = "----- Backtrace -----\n";
>> +static const char *str_backtrace_unavailable = "Backtrace unavailable\n";
>> +
>>  #endif /* GDB_PRINT_INTERNAL_BACKTRACE */
>>  
>> +/* Initialize language specific strings.  */
> 
> Move this comment into the header file please.  Then place a comment
> here that just says:
> 
>   /* See bt-utils.h.  */
> 
> With that change made:
> 
> Approved-By: Andrew Burgess <aburgess@redhat.com>
> 
> Thanks,
> Andrew
> 

Ok, pushed as 7ffd4868a7e22dd2f8126822501bfa18479d715c.


Thanks
Bernd.
  

Patch

diff --git a/gdb/bt-utils.c b/gdb/bt-utils.c
index f658ce0d4bc..fb006c29178 100644
--- a/gdb/bt-utils.c
+++ b/gdb/bt-utils.c
@@ -145,8 +145,23 @@  gdb_internal_backtrace_1 ()
 #else
 #error "unexpected internal backtrace policy"
 #endif
+
+static const char *str_backtrace = "----- Backtrace -----\n";
+static const char *str_backtrace_unavailable = "Backtrace unavailable\n";
+
 #endif /* GDB_PRINT_INTERNAL_BACKTRACE */
 
+/* Initialize language specific strings.  */
+
+void
+gdb_internal_backtrace_init_str ()
+{
+#ifdef GDB_PRINT_INTERNAL_BACKTRACE
+  str_backtrace = _("----- Backtrace -----\n");
+  str_backtrace_unavailable = _("Backtrace unavailable\n");
+#endif
+}
+
 /* See bt-utils.h.  */
 
 void
@@ -161,12 +176,12 @@  gdb_internal_backtrace ()
     gdb_stderr->write_async_safe (msg, strlen (msg));
   };
 
-  sig_write (_("----- Backtrace -----\n"));
+  sig_write (str_backtrace);
 
   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..6fe7c8c4f3b 100644
--- a/gdb/bt-utils.h
+++ b/gdb/bt-utils.h
@@ -71,4 +71,6 @@  extern void gdb_internal_backtrace ();
 extern void gdb_internal_backtrace_set_cmd (const char *args, int from_tty,
 					    cmd_list_element *c);
 
+extern void gdb_internal_backtrace_init_str ();
+
 #endif /* BT_UTILS_H */
diff --git a/gdb/event-top.c b/gdb/event-top.c
index d3cf144958a..e7556f71453 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -908,6 +908,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.  */
 
 [[noreturn]] static void
@@ -926,19 +971,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 (".");
 	}
@@ -1066,6 +1132,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 e4a40c51023..4370e95ada4 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,7 @@  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);
+  gdb_internal_backtrace_init_str ();
   current_ui = main_ui;
 
   gdb_stdtarg = gdb_stderr;