public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Fix for gdb/PR 14808, vfork/exec inferior problem
@ 2014-05-12 23:34 donb
  2014-05-15  7:08 ` Yao Qi
  0 siblings, 1 reply; 5+ messages in thread
From: donb @ 2014-05-12 23:34 UTC (permalink / raw)
  To: gdb-patches; +Cc: Don Breazeal

From: Don Breazeal <donb@codesourcery.com>

Hi,

This patch is a fix for gdb/PR14808: "vfork, follow-fork child,
detach-on-fork on, child execs, parent changes executable too (but should
not)".  The problem was that after a vfork the parent and child inferiors
shared their pspace members (program space), reflecting the reality of
the processes' shared address space.  When the child exec'd, and the
parent was detached, the pspace continued to be shared.  The executable
file name is stored in the pspace, so when the child's exec file name
was updated after the exec, the parent's was also updated.

The solution was to create a separate program space for the parent, so
that its exec file name remains intact.

This patch also includes a test for this scenario.

thanks
--Don

gdb/
2014-05-12  Don Breazeal  <donb@codesourcery.com>

	* infrun.c (handle_vfork_child_exec_or_exit): For the case
	of a vfork where we follow the child and detach the parent,
	and the child execs, create a new pspace and aspace for the
	parent inferior.

gdb/testsuite
2014-05-12  Don Breazeal  <donb@codesourcery.com>

	* gdb.base/foll-vfork.exp (vfork_relations_in_info_inferiors):
	Test that after a vfork and child exec, the parent's exec file
	name has not been changed.

---
 gdb/infrun.c                          |   42 ++++++++++++++++++++++-----------
 gdb/testsuite/gdb.base/foll-vfork.exp |   11 ++++++++
 2 files changed, 39 insertions(+), 14 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index ab39b6e..82a67d5 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -649,6 +649,7 @@ handle_vfork_child_exec_or_exit (int exec)
 	  struct cleanup *old_chain;
 	  struct program_space *pspace;
 	  struct address_space *aspace;
+	  struct inferior *parent_inf;
 
 	  /* follow-fork child, detach-on-fork on.  */
 
@@ -665,27 +666,39 @@ handle_vfork_child_exec_or_exit (int exec)
 	  else
 	    old_chain = save_current_space_and_thread ();
 
-	  /* We're letting loose of the parent.  */
+	  /* Make the parent the current inferior for target_detach.  */
 	  tp = any_live_thread_of_process (inf->vfork_parent->pid);
 	  switch_to_thread (tp->ptid);
 
-	  /* We're about to detach from the parent, which implicitly
-	     removes breakpoints from its address space.  There's a
-	     catch here: we want to reuse the spaces for the child,
-	     but, parent/child are still sharing the pspace at this
-	     point, although the exec in reality makes the kernel give
-	     the child a fresh set of new pages.  The problem here is
-	     that the breakpoints module being unaware of this, would
-	     likely chose the child process to write to the parent
-	     address space.  Swapping the child temporarily away from
-	     the spaces has the desired effect.  Yes, this is "sort
-	     of" a hack.  */
-
+	  /* The child inferior INF may be dead, so avoid giving the
+	     breakpoints module the option to write through to it
+	     by swapping the child temporarily away from the spaces
+	     (cloning a program space resets breakpoints).  */
 	  pspace = inf->pspace;
 	  aspace = inf->aspace;
 	  inf->aspace = NULL;
 	  inf->pspace = NULL;
 
+	  if (exec)
+	    {
+	      /* The parent and child inferiors have been sharing
+		 program and address space structures from the point
+		 where the parent called vfork.  Now that the child has
+		 called exec and we are detaching from the parent, the
+		 parent inferior needs to have its own pspace and aspace
+		 so that changes in the child don't affect it.  We have
+		 to give the new spaces to the parent since we saved the
+		 child's spaces as the current spaces above.  Even though
+		 we are detaching the parent, we want to keep the
+		 corresponding entry in the inferiors list intact.  */
+	      parent_inf = current_inferior ();
+	      parent_inf->aspace = new_address_space ();
+	      parent_inf->pspace = add_program_space (parent_inf->aspace);
+	      parent_inf->removable = inf->removable;
+	      set_current_program_space (parent_inf->pspace);
+	      clone_program_space (parent_inf->pspace, pspace);
+	    }
+
 	  if (debug_infrun || info_verbose)
 	    {
 	      target_terminal_ours ();
@@ -702,9 +715,10 @@ handle_vfork_child_exec_or_exit (int exec)
 				  inf->vfork_parent->pid);
 	    }
 
+	  /* Detach the parent.  */
 	  target_detach (NULL, 0);
 
-	  /* Put it back.  */
+	  /* Put the child spaces back.  */
 	  inf->pspace = pspace;
 	  inf->aspace = aspace;
 
diff --git a/gdb/testsuite/gdb.base/foll-vfork.exp b/gdb/testsuite/gdb.base/foll-vfork.exp
index fe3663c..e9b0110 100644
--- a/gdb/testsuite/gdb.base/foll-vfork.exp
+++ b/gdb/testsuite/gdb.base/foll-vfork.exp
@@ -442,6 +442,17 @@ proc vfork_relations_in_info_inferiors { variant } {
 	   pass $test
        }
    }
+
+   # Make sure the exec file name of the vfork parent is not
+   # changed when the child's is changed.
+   if { $variant == "exec" } {
+       set test "exec file name change"
+       gdb_test_multiple "info inferiors" $test {
+	   -re " 2 .*vforked-prog.*  1 .*foll-vfork.*$gdb_prompt " {
+	       pass $test
+	   }
+       }
+   }
 }}
 
 proc do_vfork_and_follow_parent_tests {} {
-- 
1.7.0.4

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] Fix for gdb/PR 14808, vfork/exec inferior problem
@ 2014-05-15 20:43 Don Breazeal
  2014-05-24  0:59 ` Yao Qi
  0 siblings, 1 reply; 5+ messages in thread
From: Don Breazeal @ 2014-05-15 20:43 UTC (permalink / raw)
  To: gdb-patches, yao

On 5/15/2014 12:06 AM, Yao Qi wrote:
>  05/13/2014 07:34 AM, donb@codesourcery.com wrote:
>> @@ -649,6 +649,7 @@ handle_vfork_child_exec_or_exit (int exec)
>>  	  struct cleanup *old_chain;
>>  	  struct program_space *pspace;
>>  	  struct address_space *aspace;
>> +	  struct inferior *parent_inf;
>>  
> 
> Local parent_inf is only used in the "if (exec)" block below, so better
> to declare it there.
OK, done.

--snip--

>> +		 where the parent called vfork.  Now that the child has
>> +		 called exec and we are detaching from the parent, the
>> +		 parent inferior needs to have its own pspace and aspace
> 
> parent inferior has its own pspace, but may not have its own aspace,
> depending on gdbarch_has_shared_address_space.
Fixed.

> 
>> +		 so that changes in the child don't affect it.  We have
>> +		 to give the new spaces to the parent since we saved the
>> +		 child's spaces as the current spaces above.  Even though
>> +		 we are detaching the parent, we want to keep the
>> +		 corresponding entry in the inferiors list intact.  */
>> +	      parent_inf = current_inferior ();
>> +	      parent_inf->aspace = new_address_space ();
> 
> Rather than creating a new address space, use maybe_new_address_space, like
>> +	      parent_inf->pspace = add_program_space (parent_inf->aspace);
> 
> 	      parent_inf->pspace
> 		= add_program_space (maybe_new_address_space ());
> 	      parent_inf->aspace = parent_inf->pspace->aspace;
Done.

> 
>> +	      parent_inf->removable = inf->removable;
> 
> Field removable of parent inferior should be unchanged, IMO.
It turns out this assignment was a NOP.  Removed.

> 
>> +	      set_current_program_space (parent_inf->pspace);
>> +	      clone_program_space (parent_inf->pspace, pspace);
> 
> Do we need to unlink parent and child?  I am not very sure.
> 
> 	      /* Break the bonds.  */
> 	      inf->vfork_parent->vfork_child = NULL;
> 
We don't need to do it here, since this happens in target_detach.  Since parent_inf is the parent, it's fork_child pointer is set.  The unlinking happens in inferior.c:exit_inferior_1, which is ultimately called via target_detach.  The child's vfork_parent pointer is cleared by existing code near the end of handle_vfork_child_exec_or_exit.

Here is the updated patch.
--Don

gdb/
2014-05-15  Don Breazeal  <donb@codesourcery.com>

	* infrun.c (handle_vfork_child_exec_or_exit): For the case
	of a vfork where we follow the child and detach the parent,
	and the child execs, create a new pspace for the parent
	inferior.

gdb/testsuite
2014-05-15  Don Breazeal  <donb@codesourcery.com>

	* gdb.base/foll-vfork.exp (vfork_relations_in_info_inferiors):
	Test that after a vfork and child exec, the parent's exec file
	name has not been changed.

---
 gdb/infrun.c                          |   44 ++++++++++++++++++++++----------
 gdb/testsuite/gdb.base/foll-vfork.exp |   11 ++++++++
 2 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index ab39b6e..20ab003 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -665,27 +665,42 @@ handle_vfork_child_exec_or_exit (int exec)
 	  else
 	    old_chain = save_current_space_and_thread ();
 
-	  /* We're letting loose of the parent.  */
+	  /* Make the parent the current inferior for target_detach.  */
 	  tp = any_live_thread_of_process (inf->vfork_parent->pid);
 	  switch_to_thread (tp->ptid);
 
-	  /* We're about to detach from the parent, which implicitly
-	     removes breakpoints from its address space.  There's a
-	     catch here: we want to reuse the spaces for the child,
-	     but, parent/child are still sharing the pspace at this
-	     point, although the exec in reality makes the kernel give
-	     the child a fresh set of new pages.  The problem here is
-	     that the breakpoints module being unaware of this, would
-	     likely chose the child process to write to the parent
-	     address space.  Swapping the child temporarily away from
-	     the spaces has the desired effect.  Yes, this is "sort
-	     of" a hack.  */
-
+	  /* The child inferior may be dead, so avoid giving the
+	     breakpoints module the option to write through to it
+	     by swapping the child temporarily away from the spaces
+	     (cloning a program space resets breakpoints).  */
 	  pspace = inf->pspace;
 	  aspace = inf->aspace;
 	  inf->aspace = NULL;
 	  inf->pspace = NULL;
 
+	  if (exec)
+	    {
+	      struct inferior *parent_inf;
+
+	      /* The parent and child inferiors have been sharing
+		 program and address space structures from the point
+		 where the parent called vfork.  Now that the child has
+		 called exec and we are detaching from the parent, the
+		 parent inferior needs to have its own pspace so that
+		 changes in the child don't affect it.  We have to give
+		 the new pspace to the parent (instead of the child)
+		 since we saved the child's spaces as the current spaces
+		 above.  Even though we are detaching the parent, we
+		 want to keep the corresponding entry in the inferiors
+		 list intact.  */
+	      parent_inf = current_inferior ();
+	      parent_inf->pspace
+	       = add_program_space (maybe_new_address_space ());
+	      parent_inf->aspace = parent_inf->pspace->aspace;
+	      set_current_program_space (parent_inf->pspace);
+	      clone_program_space (parent_inf->pspace, pspace);
+	    }
+
 	  if (debug_infrun || info_verbose)
 	    {
 	      target_terminal_ours ();
@@ -702,9 +717,10 @@ handle_vfork_child_exec_or_exit (int exec)
 				  inf->vfork_parent->pid);
 	    }
 
+	  /* Detach the parent.  */
 	  target_detach (NULL, 0);
 
-	  /* Put it back.  */
+	  /* Put the child spaces back.  */
 	  inf->pspace = pspace;
 	  inf->aspace = aspace;
 
diff --git a/gdb/testsuite/gdb.base/foll-vfork.exp b/gdb/testsuite/gdb.base/foll-vfork.exp
index fe3663c..e9b0110 100644
--- a/gdb/testsuite/gdb.base/foll-vfork.exp
+++ b/gdb/testsuite/gdb.base/foll-vfork.exp
@@ -442,6 +442,17 @@ proc vfork_relations_in_info_inferiors { variant } {
 	   pass $test
        }
    }
+
+   # Make sure the exec file name of the vfork parent is not
+   # changed when the child's is changed.
+   if { $variant == "exec" } {
+       set test "exec file name change"
+       gdb_test_multiple "info inferiors" $test {
+	   -re " 2 .*vforked-prog.*  1 .*foll-vfork.*$gdb_prompt " {
+	       pass $test
+	   }
+       }
+   }
 }}
 
 proc do_vfork_and_follow_parent_tests {} {
-- 
1.7.0.4

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-05-27 17:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-12 23:34 [PATCH] Fix for gdb/PR 14808, vfork/exec inferior problem donb
2014-05-15  7:08 ` Yao Qi
2014-05-15 20:43 Don Breazeal
2014-05-24  0:59 ` Yao Qi
2014-05-27 17:01   ` Breazeal, Don

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).