[PATCHv2,2/6] gdb: register signal handler after setting up event token

Message ID 8a82c968eef0bc232016ad5c7c6e5985a6689446.1626890878.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • GDB Synchronous Signal Handling
Related show

Commit Message

Andrew Burgess July 21, 2021, 6:08 p.m.
This commit fixes the smallest of small possible bug related to signal
handling.  If we look in async_init_signals we see code like this:

  signal (SIGQUIT, handle_sigquit);
  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");

Then if we look in handle_sigquit we see code like this:

  mark_async_signal_handler (sigquit_token);
  signal (sig, handle_sigquit);

Finally, in mark_async_signal_handler we have:

  async_handler_ptr->ready = 1;

Where async_handler_ptr will be sigquit_token.

What this means is that if a SIGQUIT arrive in async_init_signals
after handle_sigquit has been registered, but before sigquit_token has
been initialised, then GDB will most likely crash.

The chance of this happening is tiny, but fixing this is trivial, just
ensure we call create_async_signal_handler before calling signal, so
lets do that.

There are no tests for this.  Trying to land a signal in the right
spot is pretty hit and miss.  I did try changing the current HEAD GDB
like this:

  signal (SIGQUIT, handle_sigquit);
  raise (SIGQUIT);
  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");

And confirmed that this did result in a crash, after my change I tried
this:

  sigquit_token =
    create_async_signal_handler (async_do_nothing, NULL, "sigquit");
  signal (SIGQUIT, handle_sigquit);
  raise (SIGQUIT);

And GDB now starts up just fine.

gdb/ChangeLog:

	* event-top.c (async_init_signals): For each signal, call signal
	only after calling create_async_signal_handler.
---
 gdb/event-top.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

-- 
2.25.4

Patch

diff --git a/gdb/event-top.c b/gdb/event-top.c
index ab5179b7d32..2d3bfa6a9c9 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -918,12 +918,13 @@  async_init_signals (void)
 
   quit_serial_event = make_serial_event ();
 
-  signal (SIGINT, handle_sigint);
   sigint_token =
     create_async_signal_handler (async_request_quit, NULL, "sigint");
-  signal (SIGTERM, handle_sigterm);
+  signal (SIGINT, handle_sigint);
+
   async_sigterm_token
     = create_async_signal_handler (async_sigterm_handler, NULL, "sigterm");
+  signal (SIGTERM, handle_sigterm);
 
   /* If SIGTRAP was set to SIG_IGN, then the SIG_IGN will get passed
      to the inferior and breakpoints will be ignored.  */
@@ -940,10 +941,11 @@  async_init_signals (void)
      might be in memory, shared between the two).  Since we establish
      a handler for SIGQUIT, when we call exec it will set the signal
      to SIG_DFL for us.  */
-  signal (SIGQUIT, handle_sigquit);
   sigquit_token =
     create_async_signal_handler (async_do_nothing, NULL, "sigquit");
+  signal (SIGQUIT, handle_sigquit);
 #endif
+
 #ifdef SIGHUP
   if (signal (SIGHUP, handle_sighup) != SIG_IGN)
     sighup_token =