diff mbox

[v2] Fix read after xfree in linux_nat_detach

Message ID 20170323141724.1707affa@ThinkPad
State New, archived
Headers

Commit Message

Philipp Rudo March 23, 2017, 1:17 p.m. UTC 
  On Wed, 22 Mar 2017 17:26:27 +0000
Pedro Alves <palves@redhat.com> wrote:

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

I double checked, the same bug also happens when checkpointing.  The
fix now is simply to remove delete_lwp at the end of linux_nat_detach.

Although testing detach would be good, I'm not sure if the testsuite
would have found this bug.

---

From ee3dced0b22cc1edb10a82aeb79ae35d78d665bc Mon Sep 17 00:00:00 2001
From: Philipp Rudo <prudo@linux.vnet.ibm.com>
Date: Wed, 22 Mar 2017 13:53:50 +0100
Subject: [PATCH v2] Fix read after xfree in linux_nat_detach

At the end of linux_nat_detach the main_lwp is deleted (delete_lwp).
This is problematic as during detach (detach_one_lwp and
linux_fork_detach) main_lwp already gets freed.  Thus calling
delete_lwp causes a read after free.  Fix it by removing the
unnecessary delete_lwp.

gdb/ChangeLog:
	* linux-nat.c (linux_nat_detach): delete_lwp causes read after
free. Remove it.
---
 gdb/linux-nat.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Pedro Alves March 23, 2017, 1:42 p.m. UTC | #1 
OK.

On 03/23/2017 01:17 PM, Philipp Rudo wrote:
> On Wed, 22 Mar 2017 17:26:27 +0000
> Pedro Alves <palves@redhat.com> wrote:
> 
>> 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.
> 
> I double checked, the same bug also happens when checkpointing.  The
> fix now is simply to remove delete_lwp at the end of linux_nat_detach.
> 
> Although testing detach would be good, I'm not sure if the testsuite
> would have found this bug.
> 
> ---
> 
> From ee3dced0b22cc1edb10a82aeb79ae35d78d665bc Mon Sep 17 00:00:00 2001
> From: Philipp Rudo <prudo@linux.vnet.ibm.com>
> Date: Wed, 22 Mar 2017 13:53:50 +0100
> Subject: [PATCH v2] Fix read after xfree in linux_nat_detach
> 
> At the end of linux_nat_detach the main_lwp is deleted (delete_lwp).
> This is problematic as during detach (detach_one_lwp and
> linux_fork_detach) main_lwp already gets freed.  Thus calling
> delete_lwp causes a read after free.  Fix it by removing the
> unnecessary delete_lwp.
> 
> gdb/ChangeLog:
> 	* linux-nat.c (linux_nat_detach): delete_lwp causes read after
> free. Remove it.
> ---
>  gdb/linux-nat.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index dff0da5..efe7daf 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1549,7 +1549,6 @@ linux_nat_detach (struct target_ops *ops, const
> char *args, int from_tty) 
>        inf_ptrace_detach_success (ops);
>      }
> -  delete_lwp (main_lwp->ptid);
>  }
>  
>  /* Resume execution of the inferior process.  If STEP is nonzero,
>
Pedro Alves April 11, 2017, 1:31 p.m. UTC | #2 
Hi,

I ran gdb under Valgrind and noticed that this patch hadn't
been pushed yet.  I've pushed it now.

FYI, for some reason the patch was corrupt and I had to
recreate it by hand:

 $ git am prudo
 Applying: Fix read after xfree in linux_nat_detach
 fatal: corrupt patch at line 26
 Patch failed at 0001 Fix read after xfree in linux_nat_detach

Thanks,
Pedro Alves

On 03/23/2017 01:42 PM, Pedro Alves wrote:
> OK.
> 
> On 03/23/2017 01:17 PM, Philipp Rudo wrote:
>> On Wed, 22 Mar 2017 17:26:27 +0000
>> Pedro Alves <palves@redhat.com> wrote:
>>
>>> 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.
>>
>> I double checked, the same bug also happens when checkpointing.  The
>> fix now is simply to remove delete_lwp at the end of linux_nat_detach.
>>
>> Although testing detach would be good, I'm not sure if the testsuite
>> would have found this bug.
>>
>> ---
>>
>> From ee3dced0b22cc1edb10a82aeb79ae35d78d665bc Mon Sep 17 00:00:00 2001
>> From: Philipp Rudo <prudo@linux.vnet.ibm.com>
>> Date: Wed, 22 Mar 2017 13:53:50 +0100
>> Subject: [PATCH v2] Fix read after xfree in linux_nat_detach
>>
>> At the end of linux_nat_detach the main_lwp is deleted (delete_lwp).
>> This is problematic as during detach (detach_one_lwp and
>> linux_fork_detach) main_lwp already gets freed.  Thus calling
>> delete_lwp causes a read after free.  Fix it by removing the
>> unnecessary delete_lwp.
>>
>> gdb/ChangeLog:
>> 	* linux-nat.c (linux_nat_detach): delete_lwp causes read after
>> free. Remove it.
>> ---
>>  gdb/linux-nat.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
>> index dff0da5..efe7daf 100644
>> --- a/gdb/linux-nat.c
>> +++ b/gdb/linux-nat.c
>> @@ -1549,7 +1549,6 @@ linux_nat_detach (struct target_ops *ops, const
>> char *args, int from_tty) 
>>        inf_ptrace_detach_success (ops);
>>      }
>> -  delete_lwp (main_lwp->ptid);
>>  }
>>  
>>  /* Resume execution of the inferior process.  If STEP is nonzero,
>>
> 
>
Philipp Rudo April 12, 2017, 8:14 a.m. UTC | #3 
Hi Pedro,

thanks for pushing.

I think I found the reason the patch didn't apply.  Instead of git send-email I
just copy/pasted this patch to my mail client and it decided that it would be a
good idea to wrap long lines ...

> >> @@ -1549,7 +1549,6 @@ linux_nat_detach (struct target_ops *ops, const
> >> char *args, int from_tty)

Anyway I updated my settings so it shouldn't happen in the future. Thanks for
the hint and sorry for the inconvenience.

Thanks a lot
Philipp


On Tue, 11 Apr 2017 14:31:10 +0100
Pedro Alves <palves@redhat.com> wrote:

> Hi,
> 
> I ran gdb under Valgrind and noticed that this patch hadn't
> been pushed yet.  I've pushed it now.
> 
> FYI, for some reason the patch was corrupt and I had to
> recreate it by hand:
> 
>  $ git am prudo
>  Applying: Fix read after xfree in linux_nat_detach
>  fatal: corrupt patch at line 26
>  Patch failed at 0001 Fix read after xfree in linux_nat_detach
> 
> Thanks,
> Pedro Alves
> 
> On 03/23/2017 01:42 PM, Pedro Alves wrote:
> > OK.
> > 
> > On 03/23/2017 01:17 PM, Philipp Rudo wrote:  
> >> On Wed, 22 Mar 2017 17:26:27 +0000
> >> Pedro Alves <palves@redhat.com> wrote:
> >>  
> >>> 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.  
> >>
> >> I double checked, the same bug also happens when checkpointing.  The
> >> fix now is simply to remove delete_lwp at the end of linux_nat_detach.
> >>
> >> Although testing detach would be good, I'm not sure if the testsuite
> >> would have found this bug.
> >>
> >> ---
> >>
> >> From ee3dced0b22cc1edb10a82aeb79ae35d78d665bc Mon Sep 17 00:00:00 2001
> >> From: Philipp Rudo <prudo@linux.vnet.ibm.com>
> >> Date: Wed, 22 Mar 2017 13:53:50 +0100
> >> Subject: [PATCH v2] Fix read after xfree in linux_nat_detach
> >>
> >> At the end of linux_nat_detach the main_lwp is deleted (delete_lwp).
> >> This is problematic as during detach (detach_one_lwp and
> >> linux_fork_detach) main_lwp already gets freed.  Thus calling
> >> delete_lwp causes a read after free.  Fix it by removing the
> >> unnecessary delete_lwp.
> >>
> >> gdb/ChangeLog:
> >> 	* linux-nat.c (linux_nat_detach): delete_lwp causes read after
> >> free. Remove it.
> >> ---
> >>  gdb/linux-nat.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> >> index dff0da5..efe7daf 100644
> >> --- a/gdb/linux-nat.c
> >> +++ b/gdb/linux-nat.c
> >> @@ -1549,7 +1549,6 @@ linux_nat_detach (struct target_ops *ops, const
> >> char *args, int from_tty) 
> >>        inf_ptrace_detach_success (ops);
> >>      }
> >> -  delete_lwp (main_lwp->ptid);
> >>  }
> >>  
> >>  /* Resume execution of the inferior process.  If STEP is nonzero,
> >>  
> > 
> >   
>
diff mbox

Patch

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index dff0da5..efe7daf 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1549,7 +1549,6 @@  linux_nat_detach (struct target_ops *ops, const
char *args, int from_tty) 
       inf_ptrace_detach_success (ops);
     }
-  delete_lwp (main_lwp->ptid);
 }
 
 /* Resume execution of the inferior process.  If STEP is nonzero,