From: Andrew Burgess <aburgess@redhat.com>
To: Bernd Edlinger <bernd.edlinger@hotmail.de>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Re: [PATCH v2] Fix signal unsafe call inside a signal
Date: Fri, 21 Jun 2024 16:20:30 +0100 [thread overview]
Message-ID: <87v822491d.fsf@redhat.com> (raw)
In-Reply-To: <AS8P193MB12852E4057E90165B587083AE4C92@AS8P193MB1285.EURP193.PROD.OUTLOOK.COM>
Thank you for spotting and fixing this.
Bernd Edlinger <bernd.edlinger@hotmail.de> writes:
> 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.
> ---
> gdb/bt-utils.c | 22 +++++++++++--
> gdb/bt-utils.h | 4 +++
> gdb/event-top.c | 83 ++++++++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 99 insertions(+), 10 deletions(-)
>
> v2: moved initalization of language specific string to an init function
>
> diff --git a/gdb/bt-utils.c b/gdb/bt-utils.c
> index f658ce0d4bc..5626662a9c1 100644
> --- a/gdb/bt-utils.c
> +++ b/gdb/bt-utils.c
> @@ -125,6 +125,8 @@ gdb_internal_backtrace_1 ()
>
> /* See the comment on previous version of this function. */
>
> +static const char *str_backtrace_incomplete;
> +
You've placed this string between the comment and the function that the
comment applies too.
> static void
> gdb_internal_backtrace_1 ()
> {
> @@ -139,12 +141,26 @@ 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;
> +
> +void
> +init_str_internal_backtrace ()
Rather than placing this initialisation into an extern function and then
calling it from event-top.c I think it would be better to place this
initialisation into a function like:
void _initialize_bt_utils ();
void
_initialize_bt_utils ()
{
...
}
GDB will then take care of calling this automatically for you. My
reasoning is that this breaks the dependency between the signal handling
setup and the backtrace utility function.
> +{
> + 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 +177,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..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
> +
> #endif /* BT_UTILS_H */
If you pick up the _initialize_bt_utils() approach then this will no
longer be needed.
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index b81970d11bb..02e21634ce5 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -892,6 +892,50 @@ 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;
> +
> +static void
> +init_str_handle_fatal_signal ()
Static functions should have a comment on them please.
> +{
> + 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);
Missing a space after strsignal and the '(' in 4 places above.
Thanks,
Andrew
> +#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");
> + init_str_internal_backtrace ();
> +}
> +#endif
> +
> /* Called to handle fatal signals. SIG is the signal number. */
>
> static void ATTRIBUTE_NORETURN
> @@ -910,19 +954,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 +1115,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
> --
> 2.39.2
next prev parent reply other threads:[~2024-06-21 15:20 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-06-21 10:54 Bernd Edlinger
2024-06-21 15:20 ` Andrew Burgess [this message]
2024-06-24 4:59 ` Bernd Edlinger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87v822491d.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=bernd.edlinger@hotmail.de \
--cc=gdb-patches@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).