diff mbox

Fix read after xfree in linux_nat_detach

Message ID 20170322131132.98976-2-prudo@linux.vnet.ibm.com
State New, archived
Headers

Commit Message

Philipp Rudo March 22, 2017, 1:11 p.m. UTC 
  At the end of linux_nat_detach there is a check whether the inferior has a
fork.  If no fork exists the main_lwp is detached (detach_one_lwp) and
later, outside the check, deleted (delete_lwp).  This is problematic as
detach_one_lwp also calls delete_lwp freeing main_lwp.  Thus the second
call to delete_lwp reads from already freed memory.  Fix this by removing
delete_lwp at the end of detach_one_lwp.

gdb/ChangeLog:
	* linux-nat.c (detach_one_lwp): Remove call to delete_lwp.
	(detach_callback): Add call to delete_lwp and rename ...
	(detach_and_delete_callback): ... to this.
	(linux_nat_detach): Adjust.
---
 gdb/linux-nat.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Pedro Alves March 22, 2017, 3:07 p.m. UTC | #1 
On 03/22/2017 01:11 PM, Philipp Rudo wrote:
> At the end of linux_nat_detach there is a check whether the inferior has a
> fork.  If no fork exists the main_lwp is detached (detach_one_lwp) and
> later, outside the check, deleted (delete_lwp).  This is problematic as
> detach_one_lwp also calls delete_lwp freeing main_lwp.  Thus the second
> call to delete_lwp reads from already freed memory.  Fix this by removing
> delete_lwp at the end of detach_one_lwp.

Why not just move that unconditional call to delete_lwp call at
the end of linux_nat_detach to the forks_exist_p/true branch?

Actually, that call looks unnecessary for the fork case too,
since we have:

  linux_fork_detach
    -> fork_load_infrun_state
      -> linux_nat_switch_fork
         -> purge_lwp_list
            -> lwp_lwpid_htab_remove_pid
               -> lwp_free

So... couldn't we just remove that delete_lwp line and be done with it?

Thanks,
Pedro Alves
Philipp Rudo March 22, 2017, 5:16 p.m. UTC | #2 
On Wed, 22 Mar 2017 15:07:22 +0000
Pedro Alves <palves@redhat.com> wrote:

> On 03/22/2017 01:11 PM, Philipp Rudo wrote:
> > At the end of linux_nat_detach there is a check whether the
> > inferior has a fork.  If no fork exists the main_lwp is detached
> > (detach_one_lwp) and later, outside the check, deleted
> > (delete_lwp).  This is problematic as detach_one_lwp also calls
> > delete_lwp freeing main_lwp.  Thus the second call to delete_lwp
> > reads from already freed memory.  Fix this by removing delete_lwp
> > at the end of detach_one_lwp.  
> 
> Why not just move that unconditional call to delete_lwp call at
> the end of linux_nat_detach to the forks_exist_p/true branch?

That was the first idea I had.  But I decided that it would be better
for both detach functions (linux_fork_detach and detach_one_lwp) to have
the same behavior and not free the lwp but do that in a separate
step ...

> Actually, that call looks unnecessary for the fork case too,
> since we have:
> 
>   linux_fork_detach
>     -> fork_load_infrun_state
>       -> linux_nat_switch_fork
>          -> purge_lwp_list
>             -> lwp_lwpid_htab_remove_pid
>                -> lwp_free  

...  I obviously missed that.

> 
> So... couldn't we just remove that delete_lwp line and be done with
> it?

Looks like we can get simply rid of it.  I'll see that I get a test
case running which forks to verify it, tomorrow.

Thanks
Philipp

> Thanks,
> Pedro Alves
>
Pedro Alves March 22, 2017, 5:26 p.m. UTC | #3 
On 03/22/2017 05:16 PM, Philipp Rudo wrote:

> Looks like we can get simply rid of it.  I'll see that I get a test
> case running which forks to verify it, tomorrow.

This forks handling is the support for the "checkpoint" & 
friends commands, covered by gdb.base/checkpoint.exp.
Doesn't seem to exercise detach yet though, unfortunately.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 73ef2d4..b578f69 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1483,18 +1483,19 @@  detach_one_lwp (struct lwp_info *lp, int *signo_p)
 			  target_pid_to_str (lp->ptid),
 			  strsignal (signo));
     }
-
-  delete_lwp (lp->ptid);
 }
 
 static int
-detach_callback (struct lwp_info *lp, void *data)
+detach_and_delete_callback (struct lwp_info *lp, void *data)
 {
   /* We don't actually detach from the thread group leader just yet.
      If the thread group exits, we must reap the zombie clone lwps
      before we're able to reap the leader.  */
   if (ptid_get_lwp (lp->ptid) != ptid_get_pid (lp->ptid))
-    detach_one_lwp (lp, NULL);
+    {
+      detach_one_lwp (lp, NULL);
+      delete_lwp (lp->ptid);
+    }
   return 0;
 }
 
@@ -1516,7 +1517,7 @@  linux_nat_detach (struct target_ops *ops, const char *args, int from_tty)
      they're no longer running.  */
   iterate_over_lwps (pid_to_ptid (pid), stop_wait_callback, NULL);
 
-  iterate_over_lwps (pid_to_ptid (pid), detach_callback, NULL);
+  iterate_over_lwps (pid_to_ptid (pid), detach_and_delete_callback, NULL);
 
   /* Only the initial process should be left right now.  */
   gdb_assert (num_lwps (ptid_get_pid (inferior_ptid)) == 1);