[Regression] Segfault on native-extended-gdbserver + fork (was: Re: [PATCH v2 3/3] Make linux_nat_detach/thread_db_detach use the inferior parameter)

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Friday, January 19 2018, Simon Marchi wrote:

> From: Simon Marchi <simon.marchi@ericsson.com>
>
> No changes in v2.
>
> This patch makes these two functions actually use the inferior parameter
> added by the previous patch, instead of reading inferior_ptid.  I chose
> these two, because they are the one actually used when I detach on my
> GNU/Linux system, so they were easy to test.
>
> I took the opportunity to pass the inferior being detached to
> inf_ptrace_detach_success, so it could use it too.  From there, it made
> sense to add an overload of detach_inferior that takes the inferior
> directly rather than the pid, to avoid having to pass inf->pid only for
> the callee to look up the inferior structure by pid.

Hey Simon,

While working on something else, I noticed a regression introduced by
this patch.  Consider the following example program:

  #include <unistd.h>

  int
  main (int argc, char *argv[])
  {
    fork ();

    return 0;
  }

When running it under gdbserver:

  # ./gdb/gdbserver/gdbserver --multi --once :2345

And debugging it under GDB:

  # ./gdb/gdb -q -batch -ex 'set remote exec-file ./a.out' -ex 'tar extended-remote :2345' -ex r ./a.out
  Starting program:
  ...
  [Detaching after fork from child process 16102.]
  Segmentation fault (core dumped)

The problem happens on inferior.c:detach_inferior:

  void
  detach_inferior (inferior *inf)
  {
    /* Save the pid, since exit_inferior_1 will reset it.  */
    int pid = inf->pid;
              ^^^^^^^^^

    exit_inferior_1 (inf, 0);

    if (print_inferior_events)
      printf_unfiltered (_("[Inferior %d detached]\n"), pid);
  }

When this code is called from remote.c:remote_follow_fork, the PID is
valid but there is not 'inferior' associated with it, which means that
'inf == NULL'.

I've been thinking about the proper fix to this, and arrived at the
patch attached (without a ChangeLog entry; will add that if the patch
seems OK for you).  Since we will still want to print inferior events
even if 'inf == NULL', I've duplicated the print on the "detach_inferior
(int pid)" version.  Other than that, the patch is basically restoring
the old behaviour of just skipping the detach procedure if there's no
inferior object.

I'm running a regression test on BuildBot to make sure no regressions
are introduced.  I was going to write a testcase to exercise this
scenario, but we already have one, gdb.base/foll-vfork.exp.  The
failures were marked as ERROR's by dejagnu, which may explain why they
were missed...?  Not sure.  Oh, and this regression is not present in
the 8.1 branch.

WDYT?

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

diff --git a/gdb/inferior.c b/gdb/inferior.c
index 38b7369275..94432a92b1 100644
--- a/gdb/inferior.c
+++ b/gdb/inferior.c
@@ -272,7 +272,15 @@ detach_inferior (inferior *inf)
 void
 detach_inferior (int pid)
 {
-  detach_inferior (find_inferior_pid (pid));
+  inferior *inf = find_inferior_pid (pid);
+
+  if (inf != NULL)
+    detach_inferior (inf);
+  else
+    {
+      if (print_inferior_events)
+	printf_unfiltered (_("[Inferior %d detached]\n"), pid);
+    }
 }
 
 void