[patch] Fix SIGTERM signal safety (PR gdb/15358)

[patch] Fix SIGTERM signal safety (PR gdb/15358)

[Jan Kratochvil]

Hi,

gdb deadlock due to gdb calling calloc() in signal handler
http://sourceware.org/bugzilla/show_bug.cgi?id=15358

Patch depends in its functionality on:
	[patchv2 2/2] Fix CTRL-C for remote.c (PR remote/15297)
	http://sourceware.org/ml/gdb-patches/2013-06/msg00943.html
	Message-ID: <20130630181110.GB29548@host2.jankratochvil.net>

async mode seems easy.

The sync mode is a bit difficult - assuming it is safe to call quit_force from
any place of QUIT;.  OTOH the patch above assumes it can do:
	  if (check_quit_flag ())
	    send_interrupt_sequence ();
which clears quit_flag but we used set_quit_flag () to call quit_force, not
just to throw the quit exception.  This is why QUIT now checks also for
SYNC_QUIT_FORCE_RUN.

The change in linux-nat.c comes from testing i386-linux-nat.c (therefore
32-bit host GDB).  i386_linux_resume there calls QUIT; via target_read ().
This is a bug on its own, filed as:
	http://sourceware.org/bugzilla/show_bug.cgi?id=15713

But I have seen another bug in linux-nat.c, it was depending on PTRACE_KILL
but at least Linux kernel ptrace expert Oleg Nesterov considers PTRACE_KILL
superseded by kill(SIGKILL).  Therefore I used there (also) more safe SIGKILL 
so that the possibly inconsistent state of inferior from i386_linux_resume
does not matter.


No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and in
gdbserver mode.


Thanks,
Jan


gdb/
2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* defs.h (sync_quit_force_run): New declaration.
	(QUIT): Check also SYNC_QUIT_FORCE_RUN.
	* event-top.c (async_sigterm_handler): New declaration.
	(async_sigterm_token): New variable.
	(async_init_signals): Create also async_sigterm_token.
	(async_sigterm_handler): New function.
	(sync_quit_force_run): New variable.
	(handle_sigterm): Replace quit_force call by other calls.
	* linux-nat.c (linux_nat_kill): Use kill_callback first.
	Extend the comment for stop_callback.
	* utils.c (quit): Call quit_force if SYNC_QUIT_FORCE_RUN.

gdb/testsuite/
2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* gdb.base/gdb-sigterm.c: New file.
	* gdb.base/gdb-sigterm.exp: New file.

diff --git a/gdb/defs.h b/gdb/defs.h
index d8a1adb..1807167 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -176,6 +176,9 @@ extern int check_quit_flag (void);
 /* Set the quit flag.  */
 extern void set_quit_flag (void);
 
+/* Flag that function quit should call quit_force.  */
+extern volatile int sync_quit_force_run;
+
 extern int immediate_quit;
 
 extern void quit (void);
@@ -188,7 +191,7 @@ extern void quit (void);
    needed.  */
 
 #define QUIT { \
-  if (check_quit_flag ()) quit (); \
+  if (check_quit_flag () || sync_quit_force_run) quit (); \
   if (deprecated_interactive_hook) deprecated_interactive_hook (); \
 }
 
diff --git a/gdb/event-top.c b/gdb/event-top.c
index f00ab7d..4e9aa4d 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -72,6 +72,7 @@ static void async_float_handler (gdb_client_data);
 #ifdef STOP_SIGNAL
 static void async_stop_sig (gdb_client_data);
 #endif
+static void async_sigterm_handler (gdb_client_data arg);
 
 /* Readline offers an alternate interface, via callback
    functions.  These are all included in the file callback.c in the
@@ -135,6 +136,7 @@ static struct async_signal_handler *sigfpe_token;
 #ifdef STOP_SIGNAL
 static struct async_signal_handler *sigtstp_token;
 #endif
+static struct async_signal_handler *async_sigterm_token;
 
 /* Structure to save a partially entered command.  This is used when
    the user types '\' at the end of a command line.  This is necessary
@@ -769,6 +771,8 @@ async_init_signals (void)
     create_async_signal_handler (async_stop_sig, NULL);
 #endif
 
+  async_sigterm_token =
+    create_async_signal_handler (async_sigterm_handler, NULL);
 }
 
 /* Tell the event loop what to do if SIGINT is received.
@@ -796,13 +800,31 @@ handle_sigint (int sig)
   gdb_call_async_signal_handler (sigint_token, immediate_quit);
 }
 
+/* Handle GDB exit upon receiving SIGTERM if target_can_async_p ().  */
+
+static void
+async_sigterm_handler (gdb_client_data arg)
+{
+  quit_force (NULL, stdin == instream);
+}
+
+/* See defs.h.  */
+volatile int sync_quit_force_run;
+
 /* Quit GDB if SIGTERM is received.
    GDB would quit anyway, but this way it will clean up properly.  */
 void
 handle_sigterm (int sig)
 {
   signal (sig, handle_sigterm);
-  quit_force ((char *) 0, stdin == instream);
+
+  if (target_can_async_p ())
+    mark_async_signal_handler (async_sigterm_token);
+  else
+    {
+      sync_quit_force_run = 1;
+      set_quit_flag ();
+    }
 }
 
 /* Do the quit.  All the checks have been done by the caller.  */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 6ba71ba..e13cc9e 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4072,8 +4072,15 @@ linux_nat_kill (struct target_ops *ops)
     {
       ptid_t ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
 
+      /* Kill all LWP's before trying to stop them.  In rare cases the
+	 lwp_info state may not match the inferior and
+	 stop_wait_callback could lock up.  */
+      iterate_over_lwps (ptid, kill_callback, NULL);
+
       /* Stop all threads before killing them, since ptrace requires
-	 that the thread is stopped to sucessfully PTRACE_KILL.  */
+	 that the thread is stopped to sucessfully PTRACE_KILL.
+	 kill_callback normally already turned the inferior into a zombie
+	 except for old Linux kernels 2.4.x.  */
       iterate_over_lwps (ptid, stop_callback, NULL);
       /* ... and wait until all of them have reported back that
 	 they're no longer running.  */
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.c b/gdb/testsuite/gdb.base/gdb-sigterm.c
new file mode 100644
index 0000000..ffa09e4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+
+int
+main (void)
+{
+  alarm (60);
+
+  for (;;); /* loop-line */
+}
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
new file mode 100644
index 0000000..8baeb96
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
@@ -0,0 +1,94 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if { [build_executable ${testfile}.exp ${testfile}] == -1 } {
+    return -1
+}
+
+proc do_test { pass } {
+    global testfile gdb_prompt binfile pf_prefix
+
+    if ![runto_main] {
+	return -1
+    }
+
+    gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
+		   temporary
+
+    # gdb_continue_to_breakpoint would print a pass message.
+    gdb_test "continue" "Temporary breakpoint .* loop-line .*" ""
+
+    gdb_test_no_output "set range-stepping off" ""
+    gdb_test_no_output "set debug infrun 1" ""
+    gdb_test_no_output "set debug lin-lwp 1" ""
+
+    set test "run a bit #$pass"
+    set abort 1
+    gdb_test_multiple "step" $test {
+	-re "infrun: stepping inside range" {
+	    # Suppress pass $test
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+    }
+    if $abort {
+	return
+    }
+
+    set gdb_pid [exp_pid -i [board_info host fileid]]
+    remote_exec host "kill -TERM ${gdb_pid}"
+
+    set test "expect eof #$pass"
+    set abort 1
+    set stepping 0
+    gdb_test_multiple "" $test {
+	eof {
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+	-re "infrun: stepping inside range" {
+	    incr stepping
+	    if { $stepping > 200 } {
+		fail "$test (stepping inside range $stepping times)"
+	    } else {
+		exp_continue
+	    }
+	}
+    }
+    if $abort {
+	return
+    }
+}
+
+
+for {set pass 0} {$pass < 50} {incr pass} {
+
+    clean_restart ${testfile}
+    gdb_test_no_output "set target-async off" ""
+    with_test_prefix "sync" {
+        do_test $pass
+    }
+
+    clean_restart ${testfile}
+    gdb_test_no_output "set target-async on" ""
+    with_test_prefix "async" {
+        do_test $pass
+    }
+}
+pass "$pass SIGTERM passes"
diff --git a/gdb/utils.c b/gdb/utils.c
index f5c1339..1a4955c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1026,6 +1026,12 @@ print_sys_errmsg (const char *string, int errcode)
 void
 quit (void)
 {
+  if (sync_quit_force_run)
+    {
+      sync_quit_force_run = 0;
+      quit_force (NULL, stdin == instream);
+    }
+
 #ifdef __MSDOS__
   /* No steenking SIGINT will ever be coming our way when the
      program is resumed.  Don't lie.  */

Re: [patch] Fix SIGTERM signal safety (PR gdb/15358)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan>  void
Jan>  handle_sigterm (int sig)
Jan>  {
Jan>    signal (sig, handle_sigterm);
Jan> -  quit_force ((char *) 0, stdin == instream);
Jan> +
Jan> +  if (target_can_async_p ())
Jan> +    mark_async_signal_handler (async_sigterm_token);
Jan> +  else
Jan> +    {
Jan> +      sync_quit_force_run = 1;
Jan> +      set_quit_flag ();

I think calling set_quit_flag here may do the wrong thing in one case.

If Python is enabled, set_quit_flag sets a flag in the Python
interpreter.  This will cause a KeyboardInterrupt exception to be raised
if the Python interpreter happens to be the code checking the flag
first.

This means that Python would raise this exception, probably not what is
meant.  And then, since sync_quit_force_run is set, you'd still get the
quit_force call sometime after re-entering gdb anyhow.

It seems to me that simply not calling set_quit_flag ought to be safe
here.  QUIT checks the sync_quit_force_run already, so there doesn't
seem to be a need to set both flags.

I think this would fix the Python problem.

Tom

Re: [patch] Fix SIGTERM signal safety (PR gdb/15358)

[Jan Kratochvil]

On Thu, 25 Jul 2013 17:33:30 +0200, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan>  void
> Jan>  handle_sigterm (int sig)
> Jan>  {
> Jan>    signal (sig, handle_sigterm);
> Jan> -  quit_force ((char *) 0, stdin == instream);
> Jan> +
> Jan> +  if (target_can_async_p ())
> Jan> +    mark_async_signal_handler (async_sigterm_token);
> Jan> +  else
> Jan> +    {
> Jan> +      sync_quit_force_run = 1;
> Jan> +      set_quit_flag ();
> 
> I think calling set_quit_flag here may do the wrong thing in one case.
> 
> If Python is enabled, set_quit_flag sets a flag in the Python
> interpreter.  This will cause a KeyboardInterrupt exception to be raised
> if the Python interpreter happens to be the code checking the flag
> first.
> 
> This means that Python would raise this exception, probably not what is
> meant.  And then, since sync_quit_force_run is set, you'd still get the
> quit_force call sometime after re-entering gdb anyhow.

This is SIGTERM.  We do not care much what happens with GDB, we just need to
ensure GDB does not crash and that GDB removes breakpoints from the inferior.

I am roughly aware it may somehow abort Python scripts but IMO that is what we
want to do.


> It seems to me that simply not calling set_quit_flag ought to be safe
> here.  QUIT checks the sync_quit_force_run already, so there doesn't
> seem to be a need to set both flags.

Without aborting Python the Python code may run for too long before QUIT; may
get checked again back in GDB.


> I think this would fix the Python problem.

Could you be more specific what is the Python problem?  It is expected SIGTERM
is not a nice signal.


Thanks,
Jan

Re: [patch] Fix SIGTERM signal safety (PR gdb/15358)

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> I am roughly aware it may somehow abort Python scripts but IMO that
Jan> is what we want to do.

Ok.  It wasn't clear from your note that you were aware of the effect.

It isn't obvious to me that KeyboardInterrupt is what we want on the
Python side, but I dug around in CPython a bit and didn't see any way to
do better, at least not without awful hacks.

Tom