[PATCH 2/2] gdb, gdbserver: detach fork child when detaching from fork parent

[Simon Marchi <simon.marchi@polymtl.ca>]

From: Simon Marchi <simon.marchi@efficios.com>

While working with pending fork events, I wondered what would happen if
the user detached an inferior while a thread of that inferior had a
pending fork event.  What happens with the fork child, which is
ptrace-attached by the GDB process (or by GDBserver), but not known to
the core?  Sure enough, neither the core of GDB or the target detach the
child process, so GDB (or GDBserver) just stays ptrace-attached to the
process.  The result is that the fork child process is stuck, while you
would expect it to be detached and run.

When debugging on Linux, there are multiple layers in which the fork
event can be pendings:

 - in GDBserver, where a fork child is saved in the
   thread_info::fork_child field
 - in the Linux native target, in the lwp_info::status field
 - in the core of GDB, in thread_info's pending wait status

I suppose that a pending fork event could also be in the remote target's
stop reply queue, but I didn't find a case where would could detach
while having pending events there.

This patch makes each actor listed above (GDBserver, the Linux native
target, core GDB) take care of detaching any pending fork child it is
responsible for when an inferior is detached.

 - In GDBserver, that is done in the generic handle_detach function.
   Since a process_info already exists for the child, we can simply call
   detach_inferior.
 - In the Linux native target, when detacing a single thread, we check
   its status to see if it's a fork or vfork, and ptrace-detach the
   child before ptrace-detaching the thread itself.  Move the
   ptrace-detach code to a new function (detach_one_pid) to avoid code
   duplication.
 - In the core of GDB, check for a thread with a fork pending waitstatus
   in target_detach.  For this, we need a way to ask the target stack to
   detach a given pid that is not bound to an inferior.  I started by
   trying to shove that in the existing `detach` method (by making it
   take a pid instead of an inferior), but that was message.  I ended up
   adding a new `detach_pid` method.

Update the gdb.threads/pending-fork-event.exp to try to detach the
process while a thread has a pending fork event.  In order to verify
that the fork child process is correctly detached and resumes execution
outside of GDB's control, make that process create a file in the test
output directory, and make the test wait $timeout seconds for that file
to appear (it happens instantly if everything goes well).

The test catches a bug in linux-nat.c, also reported as PR 28512
("waitstatus.h:300: internal-error: gdb_signal target_waitstatus::sig()
const: Assertion `m_kind == TARGET_WAITKIND_STOPPED || m_kind ==
TARGET_WAITKIND_SIGNALLED' failed.).  When detaching a thread with a
pending event, get_detach_signal unconditionally fetches the signal
stored in the waitstatus (`tp->pending_waitstatus ().sig ()`).  However,
that is only valid if the pending event is of type
TARGET_WAITKIND_STOPPED, and this is now enforced using assertions (iit
would also be valid for TARGET_WAITKIND_SIGNALLED, but that would mean
the thread does not exist anymore, so we wouldn't be detaching it).  Add
a condition in get_detach_signal to access the signal number only if the
wait status is of kind TARGET_WAITKIND_STOPPED, and use GDB_SIGNAL_0
instead (since the thread was not stopped with a signal to begin with).

Change-Id: I6d811a56f520e3cb92d5ea563ad38976f92e93dd
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28512
---
 gdb/linux-nat.c                               | 113 ++++++++++++------
 gdb/linux-nat.h                               |   1 +
 gdb/remote.c                                  |   9 ++
 gdb/target-delegates.c                        |  24 ++++
 gdb/target.c                                  |  37 ++++--
 gdb/target.h                                  |  10 ++
 .../pending-fork-event-touch-file.c           |  26 ++++
 .../gdb.threads/pending-fork-event.c          |   6 +-
 .../gdb.threads/pending-fork-event.exp        |  76 +++++++++++-
 gdbserver/server.cc                           |  29 +++++
 10 files changed, 276 insertions(+), 55 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index cada889c5348..fa86e25e2609 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1226,6 +1226,45 @@ linux_nat_target::attach (const char *args, int from_tty)
     target_async (1);
 }
 
+/* Ptrace-detach the thread with pid PID.  */
+
+static void
+detach_one_pid (int pid, int signo)
+{
+  if (ptrace (PTRACE_DETACH, pid, 0, signo) < 0)
+    {
+      int save_errno = errno;
+
+      /* We know the thread exists, so ESRCH must mean the lwp is
+	 zombie.  This can happen if one of the already-detached
+	 threads exits the whole thread group.  In that case we're
+	 still attached, and must reap the lwp.  */
+      if (save_errno == ESRCH)
+	{
+	  int ret, status;
+
+	  ret = my_waitpid (pid, &status, __WALL);
+	  if (ret == -1)
+	    {
+	      warning (_("Couldn't reap LWP %d while detaching: %s"),
+		       pid, safe_strerror (errno));
+	    }
+	  else if (!WIFEXITED (status) && !WIFSIGNALED (status))
+	    {
+	      warning (_("Reaping LWP %d while detaching "
+			 "returned unexpected status 0x%x"),
+		       pid, status);
+	    }
+	}
+      else
+	error (_("Can't detach %d: %s"),
+	       pid, safe_strerror (save_errno));
+    }
+  else
+    linux_nat_debug_printf ("PTRACE_DETACH (%d, %s, 0) (OK)",
+			    pid, strsignal (signo));
+}
+
 /* Get pending signal of THREAD as a host signal number, for detaching
    purposes.  This is the signal the thread last stopped for, which we
    need to deliver to the thread when detaching, otherwise, it'd be
@@ -1268,7 +1307,16 @@ get_detach_signal (struct lwp_info *lp)
       if (target_is_non_stop_p () && !tp->executing ())
 	{
 	  if (tp->has_pending_waitstatus ())
-	    signo = tp->pending_waitstatus ().sig ();
+	    {
+	      /* If the thread has a pending event, and it was stopped with a
+	         signal, use that signal to resume it.  If it has a pending
+		 event of another kind, it was not stopped with a signal, so
+		 resume it without a signal.  */
+	      if (tp->pending_waitstatus ().kind () == TARGET_WAITKIND_STOPPED)
+		signo = tp->pending_waitstatus ().sig ();
+	      else
+		signo = GDB_SIGNAL_0;
+	    }
 	  else
 	    signo = tp->stop_signal ();
 	}
@@ -1320,6 +1368,24 @@ detach_one_lwp (struct lwp_info *lp, int *signo_p)
 
   gdb_assert (lp->status == 0 || WIFSTOPPED (lp->status));
 
+  /* If the lwp/thread we are about to detach has a pending fork event,
+     there is a process GDB is attached to that the core of GDB doesn't know
+     about.  Detach from it.  */
+  if (WIFSTOPPED (lp->status) && linux_is_extended_waitstatus (lp->status))
+    {
+      int event = linux_ptrace_get_extended_event (lp->status);
+
+      if (event == PTRACE_EVENT_FORK || event == PTRACE_EVENT_VFORK)
+	{
+	  unsigned long child_pid;
+	  int ret = ptrace (PTRACE_GETEVENTMSG, lp->ptid.lwp (), 0, &child_pid);
+	  if (ret == 0)
+	    detach_one_pid (child_pid, 0);
+	  else
+	    perror_warning_with_name (_("Failed to detach fork child"));
+	}
+    }
+
   if (lp->status != 0)
     linux_nat_debug_printf ("Pending %s for %s on detach.",
 			    strsignal (WSTOPSIG (lp->status)),
@@ -1356,42 +1422,7 @@ detach_one_lwp (struct lwp_info *lp, int *signo_p)
 	throw;
     }
 
-  if (ptrace (PTRACE_DETACH, lwpid, 0, signo) < 0)
-    {
-      int save_errno = errno;
-
-      /* We know the thread exists, so ESRCH must mean the lwp is
-	 zombie.  This can happen if one of the already-detached
-	 threads exits the whole thread group.  In that case we're
-	 still attached, and must reap the lwp.  */
-      if (save_errno == ESRCH)
-	{
-	  int ret, status;
-
-	  ret = my_waitpid (lwpid, &status, __WALL);
-	  if (ret == -1)
-	    {
-	      warning (_("Couldn't reap LWP %d while detaching: %s"),
-		       lwpid, safe_strerror (errno));
-	    }
-	  else if (!WIFEXITED (status) && !WIFSIGNALED (status))
-	    {
-	      warning (_("Reaping LWP %d while detaching "
-			 "returned unexpected status 0x%x"),
-		       lwpid, status);
-	    }
-	}
-      else
-	{
-	  error (_("Can't detach %s: %s"),
-		 target_pid_to_str (lp->ptid).c_str (),
-		 safe_strerror (save_errno));
-	}
-    }
-  else
-    linux_nat_debug_printf ("PTRACE_DETACH (%s, %s, 0) (OK)",
-			    target_pid_to_str (lp->ptid).c_str (),
-			    strsignal (signo));
+  detach_one_pid (lwpid, signo);
 
   delete_lwp (lp->ptid);
 }
@@ -1407,6 +1438,14 @@ detach_callback (struct lwp_info *lp)
   return 0;
 }
 
+/* Implementation of target_ops::detach_pid.  */
+
+void
+linux_nat_target::detach_pid (int pid)
+{
+  detach_one_pid (pid, 0);
+}
+
 void
 linux_nat_target::detach (inferior *inf, int from_tty)
 {
diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h
index 95e26b7ee460..2ba5f09761a9 100644
--- a/gdb/linux-nat.h
+++ b/gdb/linux-nat.h
@@ -43,6 +43,7 @@ class linux_nat_target : public inf_ptrace_target
   void attach (const char *, int) override;
 
   void detach (inferior *, int) override;
+  void detach_pid (int pid) override;
 
   void resume (ptid_t, int, enum gdb_signal) override;
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 0fb427535960..8bc5bc7a1524 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -424,6 +424,7 @@ class remote_target : public process_stratum_target
   void close () override;
 
   void detach (inferior *, int) override;
+  void detach_pid (int pid) override;
   void disconnect (const char *, int) override;
 
   void commit_resumed () override;
@@ -5910,6 +5911,14 @@ remote_target::detach (inferior *inf, int from_tty)
   remote_detach_1 (inf, from_tty);
 }
 
+/* Implementation of target_ops::detach_pid.  */
+
+void
+remote_target::detach_pid (int pid)
+{
+  remote_detach_pid (pid);
+}
+
 void
 extended_remote_target::detach (inferior *inf, int from_tty)
 {
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index fb9c78a5f793..2ed214ae400d 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -12,6 +12,7 @@ struct dummy_target : public target_ops
 
   void post_attach (int arg0) override;
   void detach (inferior *arg0, int arg1) override;
+  void detach_pid (int arg0) override;
   void disconnect (const char *arg0, int arg1) override;
   void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override;
   void commit_resumed () override;
@@ -187,6 +188,7 @@ struct debug_target : public target_ops
 
   void post_attach (int arg0) override;
   void detach (inferior *arg0, int arg1) override;
+  void detach_pid (int arg0) override;
   void disconnect (const char *arg0, int arg1) override;
   void resume (ptid_t arg0, int arg1, enum gdb_signal arg2) override;
   void commit_resumed () override;
@@ -398,6 +400,28 @@ debug_target::detach (inferior *arg0, int arg1)
   fputs_unfiltered (")\n", gdb_stdlog);
 }
 
+void
+target_ops::detach_pid (int arg0)
+{
+  this->beneath ()->detach_pid (arg0);
+}
+
+void
+dummy_target::detach_pid (int arg0)
+{
+  default_detach_pid (this, arg0);
+}
+
+void
+debug_target::detach_pid (int arg0)
+{
+  fprintf_unfiltered (gdb_stdlog, "-> %s->detach_pid (...)\n", this->beneath ()->shortname ());
+  this->beneath ()->detach_pid (arg0);
+  fprintf_unfiltered (gdb_stdlog, "<- %s->detach_pid (", this->beneath ()->shortname ());
+  target_debug_print_int (arg0);
+  fputs_unfiltered (")\n", gdb_stdlog);
+}
+
 void
 target_ops::disconnect (const char *arg0, int arg1)
 {
diff --git a/gdb/target.c b/gdb/target.c
index 6219393987e8..5e88d59b782f 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -1150,6 +1150,12 @@ default_execution_direction (struct target_ops *self)
 to_execution_direction must be implemented for reverse async");
 }
 
+static void
+default_detach_pid (target_ops *self, int pid)
+{
+  gdb_assert_not_reached ("Target does not implement the detach_pid method.");
+}
+
 /* See target.h.  */
 
 void
@@ -2548,11 +2554,6 @@ target_preopen (int from_tty)
 void
 target_detach (inferior *inf, int from_tty)
 {
-  /* After we have detached, we will clear the register cache for this inferior
-     by calling registers_changed_ptid.  We must save the pid_ptid before
-     detaching, as the target detach method will clear inf->pid.  */
-  ptid_t save_pid_ptid = ptid_t (inf->pid);
-
   /* As long as some to_detach implementations rely on the current_inferior
      (either directly, or indirectly, like through target_gdbarch or by
      reading memory), INF needs to be the current inferior.  When that
@@ -2560,17 +2561,35 @@ target_detach (inferior *inf, int from_tty)
      assertion.  */
   gdb_assert (inf == current_inferior ());
 
+  process_stratum_target *proc_target = inf->process_target ();
+
+  /* Check for a pending fork event stored in the thread_info structure,
+     detach from the fork child.  */
+  for (thread_info *thread : inf->non_exited_threads ())
+    {
+      if (!thread->has_pending_waitstatus ())
+	continue;
+
+      const target_waitstatus &ws = thread->pending_waitstatus ();
+
+      if (ws.kind () == TARGET_WAITKIND_FORKED
+	  || ws.kind () == TARGET_WAITKIND_VFORKED)
+	inf->top_target ()->detach_pid (ws.child_ptid ().pid ());
+    }
+
+  /* After we have detached, we will clear the register cache for this inferior
+     by calling registers_changed_ptid.  We must save the pid_ptid before
+     detaching, as the target detach method will clear inf->pid.  */
+  ptid_t save_pid_ptid = ptid_t (inf->pid);
+
   prepare_for_detach ();
 
   /* Hold a strong reference because detaching may unpush the
      target.  */
-  auto proc_target_ref = target_ops_ref::new_reference (inf->process_target ());
+  auto proc_target_ref = target_ops_ref::new_reference (proc_target);
 
   current_inferior ()->top_target ()->detach (inf, from_tty);
 
-  process_stratum_target *proc_target
-    = as_process_stratum_target (proc_target_ref.get ());
-
   registers_changed_ptid (proc_target, save_pid_ptid);
 
   /* We have to ensure we have no frame cache left.  Normally,
diff --git a/gdb/target.h b/gdb/target.h
index 4dc17fd4806d..e68ddc471e97 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -479,6 +479,16 @@ struct target_ops
     virtual void detach (inferior *, int)
       TARGET_DEFAULT_IGNORE ();
 
+    /* Detach from process with pid PID.
+
+       The difference  between the `detach` method and this one is that `detach`
+       is used to detach an inferior, whereas `detach_pid` is used to detach
+       a process not bound to an inferior.  For example, when detaching from
+       an inferior that has a pending fork event, `detach_pid` is used to
+       detach the fork child.  */
+    virtual void detach_pid (int pid)
+      TARGET_DEFAULT_FUNC (default_detach_pid);
+
     virtual void disconnect (const char *, int)
       TARGET_DEFAULT_NORETURN (tcomplain ());
     virtual void resume (ptid_t,
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c b/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
new file mode 100644
index 000000000000..5536381847b1
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/pending-fork-event-touch-file.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2021 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>
+
+int
+main (void)
+{
+  FILE *f = fopen (TOUCH_FILE_PATH, "w");
+  fclose (f);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.c b/gdb/testsuite/gdb.threads/pending-fork-event.c
index a39ca75a49ac..bab4a99e5786 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.c
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.c
@@ -32,18 +32,18 @@ break_here (void)
 static void
 do_fork (void)
 {
-  pthread_barrier_wait (&barrier);
-
   while (!release_forking_thread);
 
   if (FORK_FUNCTION () == 0)
-    _exit (0);
+    execl (TOUCH_FILE_BIN, TOUCH_FILE_BIN, NULL);
 
 }
 
 static void *
 thread_func (void *p)
 {
+  pthread_barrier_wait (&barrier);
+
 #if defined(MAIN_THREAD_FORKS)
   break_here ();
 #elif defined(OTHER_THREAD_FORKS)
diff --git a/gdb/testsuite/gdb.threads/pending-fork-event.exp b/gdb/testsuite/gdb.threads/pending-fork-event.exp
index 6574b9abf5b7..cbe55c2f5e7c 100644
--- a/gdb/testsuite/gdb.threads/pending-fork-event.exp
+++ b/gdb/testsuite/gdb.threads/pending-fork-event.exp
@@ -28,11 +28,32 @@
 # for it.  Once fixed, the child process' thread is hidden by whoever holds the
 # pending fork event.
 
-standard_testfile
+standard_testfile .c -touch-file.c
 
-proc do_test { target-non-stop who_forks fork_function } {
+set touch_file_bin $binfile-touch-file
+set touch_file_path [standard_output_file some-file]
 
-    set opts [list debug "additional_flags=-DFORK_FUNCTION=$fork_function"]
+set opts [list debug "additional_flags=-DTOUCH_FILE_PATH=\"$touch_file_path\""]
+if { [gdb_compile "$srcdir/$subdir/$srcfile2" $touch_file_bin executable $opts] != "" } {
+    return 0
+}
+
+# Run until execution is stopped and a thread has a pending fork event.
+#
+# WHO_FORKS tells which of the thread program threads does the fork ("main" or
+# "other").
+#
+# FORK_FUNCTION tells which function to call to fork ("fork" or "vfork").
+#
+# Return 1 on success, 0 otherwise.
+
+proc setup { target-non-stop who_forks fork_function } {
+    set this_binfile ${::binfile}-${fork_function}
+
+    set opts [list \
+	debug \
+	"additional_flags=-DFORK_FUNCTION=$fork_function" \
+	"additional_flags=-DTOUCH_FILE_BIN=\"$::touch_file_bin\""]
 
     # WHO_FORKS says which of the main or other thread calls (v)fork.  The
     # thread that does not call (v)fork is the one who tries to step.
@@ -47,7 +68,7 @@ proc do_test { target-non-stop who_forks fork_function } {
     }
 
     if { [gdb_compile_pthreads "$::srcdir/$::subdir/$::srcfile" $this_binfile executable $opts] != "" } {
-	return
+	return 0
     }
 
     save_vars { ::GDBFLAGS } {
@@ -57,7 +78,7 @@ proc do_test { target-non-stop who_forks fork_function } {
 
     if {![runto_main]} {
 	fail "could not run to main"
-	return
+	return 0
     }
 
     # Run until breakpoint in the second thread.
@@ -75,12 +96,55 @@ proc do_test { target-non-stop who_forks fork_function } {
 
     # Make sure there's still a single inferior.
     gdb_test "info inferior" {\* 1 [^\r\n]+}
+
+    return 1
+}
+
+# Return 1 if file ::TOUCH_FILE_PATH exists, else 0.
+
+proc file_exists {} {
+    return [file exists $::touch_file_path]
+}
+
+# Wait up to ::TIMEOUT seconds for file ::TOUCH_FILE_PATH to exist.  Return
+# 1 if it does exist, 0 otherwise.
+
+proc file_exists_with_timeout {} {
+    for {set i 0} {$i < $::timeout} {incr i} {
+	if { [file_exists] } {
+	    return 1
+	}
+
+	sleep 1
+    }
+
+    return 0
+}
+
+# Run until there exists (at least, likely exists) a pending fork event.
+# Detach the fork parent.  Verify that the fork child (which does not appear in
+# GDB, since the fork event is pending) is detached as well.
+
+proc_with_prefix do_test_detach { target-non-stop who_forks fork_function } {
+    file delete $::touch_file_path
+    gdb_assert { ![file_exists] } "file does not exist before test"
+
+    if { ![setup ${target-non-stop} $who_forks $fork_function] } {
+	return
+    }
+
+    gdb_test "detach"
+
+    # After being detached, the fork child creates file ::TOUCH_FILE_PATH.
+    # Seeing this file tells us the fork child was detached and executed
+    # successfully.
+    gdb_assert { [file_exists_with_timeout] } "file exists after detach"
 }
 
 foreach_with_prefix target-non-stop { auto on off } {
     foreach_with_prefix who_forks { main other } {
 	foreach_with_prefix fork_function { fork vfork } {
-	    do_test ${target-non-stop} $who_forks $fork_function
+	    do_test_detach ${target-non-stop} $who_forks $fork_function
 	}
     }
 }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 9890d6cc9798..f34da89d5a6e 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1250,6 +1250,35 @@ handle_detach (char *own_buf)
   /* We'll need this after PROCESS has been destroyed.  */
   int pid = process->pid;
 
+  /* If this process has an unreported fork child, that child is not known to
+     GDB, so detach it as well.
+
+     Here, we specifically don't want to use "safe iteration", as detaching
+     another process might delete the next thread in the iteration, which is
+     the one saved by the safe iterator.  We will never delete the currently
+     iterated on thread, so standard iteration should be safe.  */
+  for (thread_info *thread : all_threads)
+    {
+      /* Only threads that are of the process we are detaching.  */
+      if (thread->id.pid () != pid)
+	continue;
+
+      /* Only threads that have a pending fork event.  */
+      if (thread->fork_child == nullptr)
+	continue;
+
+      process_info *fork_child_process
+	= find_process_pid (thread->fork_child->id.pid ());
+      gdb_assert (fork_child_process != nullptr);
+
+      int fork_child_pid = fork_child_process->pid;
+
+      if (detach_inferior (fork_child_process) != 0)
+	warning (_("Failed to detach fork child %s, child of %s"),
+		 target_pid_to_str (ptid_t (fork_child_pid)).c_str (),
+		 target_pid_to_str (thread->id).c_str ());
+    }
+
   if (detach_inferior (process) != 0)
     write_enn (own_buf);
   else
-- 
2.33.1