Patchwork [Regression] Segfault on native-extended-gdbserver + fork

login
register
mail settings
Submitter Sergio Durigan Junior
Date Jan. 29, 2018, 5:36 p.m.
Message ID <87372o8t45.fsf@redhat.com>
Download mbox | patch
Permalink /patch/25672/
State New
Headers show

Comments

Sergio Durigan Junior - Jan. 29, 2018, 5:36 p.m.
On Monday, January 29 2018, I wrote:

> On Monday, January 29 2018, Simon Marchi wrote:
>
>> On 2018-01-29 11:00, Pedro Alves wrote:
>>> On 01/28/2018 04:50 PM, Simon Marchi wrote:
>>>> On 2018-01-28 01:32, Sergio Durigan Junior wrote:
>>>> 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.
>>>
>>> But why would we want to print that?  We will have already printed
>>>
>>>   "Detaching after fork from child process PID."
>>>
>>> from the common code.  When native debugging, in this scenario,
>>> we don't call detach_inferior either, right?  Can't see why
>>> we'd want to call it for remote.
>>
>> It's true that it's a bit of a lie to say "[Inferior PID detached]" if
>> there never actually was an inferior for that PID.  Since we never
>> print "[Inferior PID detached]" on native in that case, I am fine with
>> removing the call from remote.c.  Sergio, that would fix the crash you
>> found I think?
>
> I was also unsure about printing the message in this case, because
> there's no real detach happening.  I'm fine with not printing it.  And
> yes, removing the call to "detach_inferior" also fixes the problem.
>
> I'll prepare a patch.

Here's what I have.  WDYT?

I'll address Pedro's comment about changing the "[Inferior PID
detached]" output in another patch.
Pedro Alves - Jan. 29, 2018, 5:47 p.m.
On 01/29/2018 05:36 PM, Sergio Durigan Junior wrote:
> From 4a37d08ca6c1aec7f47e2278b0fe78a0038eb9ee Mon Sep 17 00:00:00 2001
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Mon, 29 Jan 2018 12:29:21 -0500
> Subject: [PATCH] Don't call "detach_inferior" on "remote_follow_fork"
> 
> This patch fixes a regression that has been introduced by:
> 
> commit bc09b0c14fb713a9aec25e09b78499f3bc2441b5
> Date:   Fri Jan 19 11:48:11 2018 -0500
> 
>     Make linux_nat_detach/thread_db_detach use the inferior parameter
> 
> Consider the following example program:
> 
>   #include <unistd.h>
> 
>   int
>   main (int argc, char *argv[])
>   {
>     fork ();
> 
>     return 0;
>   }
> 

Please also mention gdb.base/foll-fork.exp.

> When running it under gdbserver:
> 
>   # ./gdb/gdbserver/gdbserver --multi --once :2345
> 
> And debugging it under GDB, we see a segmentation fault:
> 
>   # ./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'.

s/there is not/there is no/

> 
> The proper fix here is to not call "detach_inferior" when doing remote
> follow-fork, because we don't have an inferior to detach on the host
> side.

Add something like this here:

 Before bc09b0c1, that call was already a nop (exit_inferior_1 bails
 out early if you pass it a NULL inferior), except that it printed
 "Inferior PID detached" when "set print inferior-events" is on.
 Since native debugging doesn't call detach_inferior in this case,
 removing the call from remote aligns remote debugging output
 with native debugging output further.

and it's good to me.

Thanks,
Pedro Alves
Sergio Durigan Junior - Jan. 29, 2018, 6:06 p.m.
On Monday, January 29 2018, Pedro Alves wrote:

> On 01/29/2018 05:36 PM, Sergio Durigan Junior wrote:
>> From 4a37d08ca6c1aec7f47e2278b0fe78a0038eb9ee Mon Sep 17 00:00:00 2001
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Mon, 29 Jan 2018 12:29:21 -0500
>> Subject: [PATCH] Don't call "detach_inferior" on "remote_follow_fork"
>> 
>> This patch fixes a regression that has been introduced by:
>> 
>> commit bc09b0c14fb713a9aec25e09b78499f3bc2441b5
>> Date:   Fri Jan 19 11:48:11 2018 -0500
>> 
>>     Make linux_nat_detach/thread_db_detach use the inferior parameter
>> 
>> Consider the following example program:
>> 
>>   #include <unistd.h>
>> 
>>   int
>>   main (int argc, char *argv[])
>>   {
>>     fork ();
>> 
>>     return 0;
>>   }
>> 
>
> Please also mention gdb.base/foll-fork.exp.

Done.

>> When running it under gdbserver:
>> 
>>   # ./gdb/gdbserver/gdbserver --multi --once :2345
>> 
>> And debugging it under GDB, we see a segmentation fault:
>> 
>>   # ./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'.
>
> s/there is not/there is no/

Fixed.

>> 
>> The proper fix here is to not call "detach_inferior" when doing remote
>> follow-fork, because we don't have an inferior to detach on the host
>> side.
>
> Add something like this here:
>
>  Before bc09b0c1, that call was already a nop (exit_inferior_1 bails
>  out early if you pass it a NULL inferior), except that it printed
>  "Inferior PID detached" when "set print inferior-events" is on.
>  Since native debugging doesn't call detach_inferior in this case,
>  removing the call from remote aligns remote debugging output
>  with native debugging output further.

Added.

> and it's good to me.

Pushed.

69ab5edb4d601611ba7b4d05e56689d4b60ca3b1

Thanks,

Patch

diff --git a/gdb/remote.c b/gdb/remote.c
index 5ac84df0a0..74d18f7b17 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5206,7 +5206,6 @@  remote_follow_fork (struct target_ops *ops, int follow_child,
 	  child_pid = ptid_get_pid (child_ptid);
 
 	  remote_detach_pid (child_pid);
-	  detach_inferior (child_pid);
 	}
     }
   return 0;