Re: [Regression] Segfault on native-extended-gdbserver + fork

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

On 2018-01-28 01:32, Sergio Durigan Junior wrote:
> Hey Simon,
> 
> While working on something else, I noticed a regression introduced by
> this patch.

Hi Sergio,

Thanks for reporting and fixing this!

> 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'.

Ah yeah, I hadn't thought about that.

> 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 fine with this, but I was curious about what happens in Pedro's 
multi-target branch.  I remember he said that the detach_inferior(int) 
version disappears in that branch, though I can't find where he said 
that.  But looking at the branch I can see it's indeed the case:

   
https://github.com/palves/gdb/blob/palves/multi-target/gdb/inferior.c#L250

So I was wondering what remote_follow_fork calls in that case, since it 
can't call the detach_inferior(inferior *) version without an inferior.  
Apparently it calls a new remote_detach_pid function:

   
https://github.com/palves/gdb/blob/palves/multi-target/gdb/remote.c#L5859

This means (I just tried it) that it won't show the "[Inferior %d 
detached]\n" message in that case.  So what I would suggest is putting

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

in its own function, called by both versions of detach_inferior for now 
(bonus, it de-duplicates the printing of the message).  In the 
multi-target branch, remote_target::follow_fork (renamed from 
remote_follow_fork) can call this function in the case where we don't 
have an 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?

That's fine with me with the printing of the message in its own 
function, as explained above.

> --
> 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

Thanks,

Simon