Fix PR 19494: hang when killing unfollowed fork children
authorPedro Alves <palves@redhat.com>
Mon, 25 Jan 2016 12:00:20 +0000 (12:00 +0000)
committerPedro Alves <palves@redhat.com>
Mon, 25 Jan 2016 13:16:43 +0000 (13:16 +0000)
linux_nat_kill relies on get_last_target_status to determine whether
the current inferior is stopped at a unfollowed fork/vfork event.
This is bad because many things can happen ever since we caught the
fork/vfork event...  This commit rewrites that code to instead walk
the thread list looking for unfollowed fork events, similarly to what
was done for remote.c.

New test included.  The main idea of the test is make sure that when
the program stops for a fork catchpoint, and the user kills the
parent, gdb also kills the unfollowed fork child.  Since the child
hasn't been added as an inferior at that point, we need some other
portable way to detect that the child is gone.  The test uses a pipe
for that.  The program forks twice, so you have grandparent, child and
grandchild.  The grandchild inherits the write side of the pipe.  The
grandparent hangs reading from the pipe, since nothing ever writes to
it.  If, when GDB kills the child, it also kills the grandchild, then
the grandparent's pipe read returns 0/EOF and the test passes.
Otherwise, if GDB doesn't kill the grandchild, then the pipe read
never returns and the test times out, like:

 FAIL: gdb.base/catch-fork-kill.exp: fork-kind=fork: exit-kind=kill: fork: kill parent (timeout)
 FAIL: gdb.base/catch-fork-kill.exp: fork-kind=vfork: exit-kind=kill: vfork: kill parent (timeout)

No regressions on x86_64 Fedora 20.  New test passes with gdbserver as
well.

gdb/ChangeLog:
2016-01-25  Pedro Alves  <palves@redhat.com>

PR gdb/19494
* linux-nat.c (kill_one_lwp): New, factored out from ...
(kill_callback): ... this.
(kill_wait_callback): New, factored out from ...
(kill_wait_one_lwp): ... this.
(kill_unfollowed_fork_children): New function.
(linux_nat_kill): Use it.

gdb/testsuite/ChangeLog:
2016-01-25  Pedro Alves  <palves@redhat.com>

PR gdb/19494
* gdb.base/catch-fork-kill.c: New file.
* gdb.base/catch-fork-kill.exp: New file.

gdb/ChangeLog
gdb/linux-nat.c
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.base/catch-fork-kill.c [new file with mode: 0644]
gdb/testsuite/gdb.base/catch-fork-kill.exp [new file with mode: 0644]

index 057c14f8cf4a804ab2d21e989850e3f8a4eee977..4cfabfb600a2c4fe1b11b2558524e6936725ac54 100644 (file)
@@ -1,3 +1,13 @@
+2016-01-25  Pedro Alves  <palves@redhat.com>
+
+       PR gdb/19494
+       * linux-nat.c (kill_one_lwp): New, factored out from ...
+       (kill_callback): ... this.
+       (kill_wait_callback): New, factored out from ...
+       (kill_wait_one_lwp): ... this.
+       (kill_unfollowed_fork_children): New function.
+       (linux_nat_kill): Use it.
+
 2016-01-22  John Baldwin  <jhb@FreeBSD.org>
 
        * fbsd-nat.c (fbsd_pid_to_str): Adjust string format.
index eb609c85b73e077dff205dbff409b89eb23fbe86..6ded38da298e8af473006f2c0b37c432d78211c2 100644 (file)
@@ -3466,44 +3466,44 @@ linux_nat_wait (struct target_ops *ops,
   return event_ptid;
 }
 
-static int
-kill_callback (struct lwp_info *lp, void *data)
+/* Kill one LWP.  */
+
+static void
+kill_one_lwp (pid_t pid)
 {
   /* PTRACE_KILL may resume the inferior.  Send SIGKILL first.  */
 
   errno = 0;
-  kill_lwp (ptid_get_lwp (lp->ptid), SIGKILL);
+  kill_lwp (pid, SIGKILL);
   if (debug_linux_nat)
     {
       int save_errno = errno;
 
       fprintf_unfiltered (gdb_stdlog,
-                         "KC:  kill (SIGKILL) %s, 0, 0 (%s)\n",
-                         target_pid_to_str (lp->ptid),
+                         "KC:  kill (SIGKILL) %ld, 0, 0 (%s)\n", (long) pid,
                          save_errno ? safe_strerror (save_errno) : "OK");
     }
 
   /* Some kernels ignore even SIGKILL for processes under ptrace.  */
 
   errno = 0;
-  ptrace (PTRACE_KILL, ptid_get_lwp (lp->ptid), 0, 0);
+  ptrace (PTRACE_KILL, pid, 0, 0);
   if (debug_linux_nat)
     {
       int save_errno = errno;
 
       fprintf_unfiltered (gdb_stdlog,
-                         "KC:  PTRACE_KILL %s, 0, 0 (%s)\n",
-                         target_pid_to_str (lp->ptid),
+                         "KC:  PTRACE_KILL %ld, 0, 0 (%s)\n", (long) pid,
                          save_errno ? safe_strerror (save_errno) : "OK");
     }
-
-  return 0;
 }
 
-static int
-kill_wait_callback (struct lwp_info *lp, void *data)
+/* Wait for an LWP to die.  */
+
+static void
+kill_wait_one_lwp (pid_t pid)
 {
-  pid_t pid;
+  pid_t res;
 
   /* We must make sure that there are no pending events (delayed
      SIGSTOPs, pending SIGTRAPs, etc.) to make sure the current
@@ -3511,49 +3511,83 @@ kill_wait_callback (struct lwp_info *lp, void *data)
 
   do
     {
-      pid = my_waitpid (ptid_get_lwp (lp->ptid), NULL, __WALL);
-      if (pid != (pid_t) -1)
+      res = my_waitpid (pid, NULL, __WALL);
+      if (res != (pid_t) -1)
        {
          if (debug_linux_nat)
            fprintf_unfiltered (gdb_stdlog,
-                               "KWC: wait %s received unknown.\n",
-                               target_pid_to_str (lp->ptid));
+                               "KWC: wait %ld received unknown.\n",
+                               (long) pid);
          /* The Linux kernel sometimes fails to kill a thread
             completely after PTRACE_KILL; that goes from the stop
             point in do_fork out to the one in get_signal_to_deliver
             and waits again.  So kill it again.  */
-         kill_callback (lp, NULL);
+         kill_one_lwp (pid);
        }
     }
-  while (pid == ptid_get_lwp (lp->ptid));
+  while (res == pid);
+
+  gdb_assert (res == -1 && errno == ECHILD);
+}
+
+/* Callback for iterate_over_lwps.  */
 
-  gdb_assert (pid == -1 && errno == ECHILD);
+static int
+kill_callback (struct lwp_info *lp, void *data)
+{
+  kill_one_lwp (ptid_get_lwp (lp->ptid));
   return 0;
 }
 
+/* Callback for iterate_over_lwps.  */
+
+static int
+kill_wait_callback (struct lwp_info *lp, void *data)
+{
+  kill_wait_one_lwp (ptid_get_lwp (lp->ptid));
+  return 0;
+}
+
+/* Kill the fork children of any threads of inferior INF that are
+   stopped at a fork event.  */
+
+static void
+kill_unfollowed_fork_children (struct inferior *inf)
+{
+  struct thread_info *thread;
+
+  ALL_NON_EXITED_THREADS (thread)
+    if (thread->inf == inf)
+      {
+       struct target_waitstatus *ws = &thread->pending_follow;
+
+       if (ws->kind == TARGET_WAITKIND_FORKED
+           || ws->kind == TARGET_WAITKIND_VFORKED)
+         {
+           ptid_t child_ptid = ws->value.related_pid;
+           int child_pid = ptid_get_pid (child_ptid);
+           int child_lwp = ptid_get_lwp (child_ptid);
+           int status;
+
+           kill_one_lwp (child_lwp);
+           kill_wait_one_lwp (child_lwp);
+
+           /* Let the arch-specific native code know this process is
+              gone.  */
+           linux_nat_forget_process (child_pid);
+         }
+      }
+}
+
 static void
 linux_nat_kill (struct target_ops *ops)
 {
   struct target_waitstatus last;
-  ptid_t last_ptid;
-  int status;
 
   /* If we're stopped while forking and we haven't followed yet,
      kill the other task.  We need to do this first because the
      parent will be sleeping if this is a vfork.  */
-
-  get_last_target_status (&last_ptid, &last);
-
-  if (last.kind == TARGET_WAITKIND_FORKED
-      || last.kind == TARGET_WAITKIND_VFORKED)
-    {
-      ptrace (PT_KILL, ptid_get_pid (last.value.related_pid), 0, 0);
-      wait (&status);
-
-      /* Let the arch-specific native code know this process is
-        gone.  */
-      linux_nat_forget_process (ptid_get_pid (last.value.related_pid));
-    }
+  kill_unfollowed_fork_children (current_inferior ());
 
   if (forks_exist_p ())
     linux_fork_killall ();
index aea573d87fa500f3dceaaf400991dfc7bc617392..a6daf80de30a0440baf387f17927c6e99de72fc9 100644 (file)
@@ -1,3 +1,9 @@
+2016-01-25  Pedro Alves  <palves@redhat.com>
+
+       PR gdb/19494
+       * gdb.base/catch-fork-kill.c: New file.
+       * gdb.base/catch-fork-kill.exp: New file.
+
 2016-01-25  Pedro Alves  <palves@redhat.com>
 
        * gdb.base/step-sw-breakpoint-adjust-pc.exp (foreach_with_prefix):
diff --git a/gdb/testsuite/gdb.base/catch-fork-kill.c b/gdb/testsuite/gdb.base/catch-fork-kill.c
new file mode 100644 (file)
index 0000000..607df2e
--- /dev/null
@@ -0,0 +1,97 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 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 <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <assert.h>
+
+int fds[2] = { -1, -1 };
+
+static void
+grandparent_done (void)
+{
+}
+
+/* The exp file overrides this in order to test both fork and
+   vfork.  */
+#ifndef FORK
+#define FORK fork
+#endif
+
+int
+main (void)
+{
+  int pid;
+  int nbytes;
+  const char string[] = "Hello, world!\n";
+  char readbuffer[80];
+
+  /* Don't run forever.  */
+  alarm (300);
+
+  /* Create a pipe.  The write side will be inherited all the way to
+     the grandchild.  The grandparent will read this, expecting to see
+     EOF (meaning the grandchild closed the pipe).  */
+  pipe (fds);
+
+  pid = FORK ();
+  if (pid < 0)
+    {
+      perror ("fork");
+      exit (1);
+    }
+  else if (pid == 0)
+    {
+      /* Close input side of pipe.  */
+      close (fds[0]);
+
+      pid = FORK ();
+      if (pid == 0)
+       {
+         printf ("I'm the grandchild!\n");
+
+         /* Don't explicitly close the pipe.  If GDB fails to kill
+            this process, then the grandparent will hang in the pipe
+            read below.  */
+#if 0
+         close (fds[1]);
+#endif
+         while (1)
+           sleep (1);
+       }
+      else
+       {
+         close (fds[1]);
+         printf ("I'm the proud parent of child #%d!\n", pid);
+         wait (NULL);
+       }
+    }
+  else if (pid > 0)
+    {
+      close (fds[1]);
+      printf ("I'm the proud parent of child #%d!\n", pid);
+      nbytes = read (fds[0], readbuffer, sizeof (readbuffer));
+      assert (nbytes == 0);
+      printf ("read returned nbytes=%d\n", nbytes);
+      wait (NULL);
+
+      grandparent_done ();
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/catch-fork-kill.exp b/gdb/testsuite/gdb.base/catch-fork-kill.exp
new file mode 100644 (file)
index 0000000..29d2077
--- /dev/null
@@ -0,0 +1,99 @@
+# Copyright 2016 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/>.
+
+# When we intercept a fork/vfork with a catchpoint, the child is left
+# stopped.  At that point, if we kill the parent, we should kill the
+# child as well.  This test makes sure that works.  See PR gdb/19494.
+
+# The main idea of the test is make sure that when the program stops
+# for a fork catchpoint, and the user kills the parent, gdb also kills
+# the unfollowed fork child.  Since the child hasn't been added as an
+# inferior at that point, we need some other portable way to detect
+# that the child is gone.  The test uses a pipe for that.  The program
+# forks twice, so you have grandparent, child and grandchild.  The
+# grandchild inherits the write side of the pipe.  The grandparent
+# hangs reading from the pipe, since nothing ever writes to it.  If,
+# when GDB kills the child, it also kills the grandchild, then the
+# grandparent's pipe read returns 0/EOF and the test passes.
+# Otherwise, if GDB doesn't kill the grandchild, then the pipe read
+# never returns and the test times out.
+
+standard_testfile
+
+# Build two programs -- one for fork, and another for vfork.
+set testfile_fork "${testfile}-fork"
+set testfile_vfork "${testfile}-vfork"
+
+foreach kind {"fork" "vfork"} {
+    set compile_options "debug additional_flags=-DFORK=$kind"
+    set testfile [set testfile_$kind]
+    if {[build_executable $testfile.exp $testfile ${srcfile} \
+            ${compile_options}] == -1} {
+       untested "failed to compile $testfile"
+       return -1
+    }
+}
+
+# The test proper.  FORK_KIND is either "fork" or "vfork".  EXIT_KIND
+# is either "exit" (run the parent to exit) or "kill" (kill parent).
+
+proc do_test {fork_kind exit_kind} {
+    global testfile testfile_$fork_kind
+
+    set testfile [set testfile_$fork_kind]
+
+    with_test_prefix "$fork_kind" {
+       clean_restart $testfile
+
+       if ![runto_main] {
+           untested "could not run to main"
+           return -1
+       }
+
+       gdb_test_no_output "set follow-fork child"
+       gdb_test_no_output "set detach-on-fork off"
+
+       gdb_test "catch $fork_kind" "Catchpoint .*($fork_kind).*"
+
+       gdb_test "continue" \
+           "Catchpoint \[0-9\]* \\(${fork_kind}ed process \[0-9\]*\\),.*" \
+           "continue to child ${fork_kind}"
+
+       gdb_test "continue" \
+           "Catchpoint \[0-9\]* \\(${fork_kind}ed process \[0-9\]*\\),.*" \
+           "continue to grandchild ${fork_kind}"
+
+       gdb_test "kill inferior 2" "" "kill child" \
+           "Kill the program being debugged.*y or n. $" "y"
+
+       gdb_test "inferior 1" "Switching to inferior 1 .*" "switch to parent"
+
+       if {$exit_kind == "exit"} {
+           gdb_test "break grandparent_done" "Breakpoint .*"
+           gdb_test "continue" "hit Breakpoint .*, grandparent_done.*"
+       } elseif {$exit_kind == "kill"} {
+           gdb_test "kill" "" "kill parent" \
+               "Kill the program being debugged.*y or n. $" "y"
+       } else {
+           perror "unreachable"
+       }
+    }
+}
+
+foreach_with_prefix fork-kind {"fork" "vfork"} {
+    foreach_with_prefix exit-kind {"exit" "kill"} {
+       do_test ${fork-kind} ${exit-kind}
+    }
+}