[PATCH] Fix memory leak in python.c:do_start_initialization

[PATCH] Fix memory leak in python.c:do_start_initialization

[Philipp Rudo]

When intializing Python the path to the python binary is build the
following way

progname = concat (ldirname (python_libdir), SLASH_STRING, "bin",
		   SLASH_STRING, "python", (char *) NULL);

This is problematic as both concat and ldirname allocate memory for the
string they return.  Thus the memory allocated by ldirname cannot be
accessed afterwards causing a memory leak.  Fix it by temporarily storing
libdir in a variable and xfree it after concat.

gdb/ChangeLog:
	python/python.c (do_start_initialization): Fix memory leak.
---
 gdb/python/python.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/gdb/python/python.c b/gdb/python/python.c
index 73fb3d0..6b16613 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1535,7 +1535,7 @@ extern initialize_file_ftype _initialize_python;
 static bool
 do_start_initialization ()
 {
-  char *progname;
+  char *progname, *libdir;
 #ifdef IS_PY3K
   int i;
   size_t progsize, count;
@@ -1550,8 +1550,10 @@ do_start_initialization ()
      /foo/bin/python
      /foo/lib/pythonX.Y/...
      This must be done before calling Py_Initialize.  */
-  progname = concat (ldirname (python_libdir), SLASH_STRING, "bin",
+  libdir = ldirname (python_libdir);
+  progname = concat (libdir, SLASH_STRING, "bin",
 		     SLASH_STRING, "python", (char *) NULL);
+  xfree (libdir);
 #ifdef IS_PY3K
   oldloc = xstrdup (setlocale (LC_ALL, NULL));
   setlocale (LC_ALL, "");
-- 
2.8.4

[PATCH] Fix read after xfree in linux_nat_detach

[Philipp Rudo]

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

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);
-- 
2.8.4

Re: [PATCH] Fix read after xfree in linux_nat_detach

[Pedro Alves]

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

Re: [PATCH] Fix read after xfree in linux_nat_detach

[Philipp Rudo]

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
> 

Re: [PATCH] Fix read after xfree in linux_nat_detach

[Pedro Alves]

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

[PATCH v2] Fix read after xfree in linux_nat_detach

[Philipp Rudo]

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

Re: [PATCH v2] Fix read after xfree in linux_nat_detach

[Pedro Alves]

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


-- 
Thanks,
Pedro Alves

Re: [PATCH v2] Fix read after xfree in linux_nat_detach

[Philipp Rudo]

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

Re: [PATCH] Fix memory leak in python.c:do_start_initialization

[Pedro Alves]

Hi Philipp,

Thanks.

On 03/22/2017 01:11 PM, Philipp Rudo wrote:

> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index 73fb3d0..6b16613 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -1535,7 +1535,7 @@ extern initialize_file_ftype _initialize_python;
>  static bool
>  do_start_initialization ()
>  {
> -  char *progname;
> +  char *progname, *libdir;
>  #ifdef IS_PY3K
>    int i;
>    size_t progsize, count;
> @@ -1550,8 +1550,10 @@ do_start_initialization ()
>       /foo/bin/python
>       /foo/lib/pythonX.Y/...
>       This must be done before calling Py_Initialize.  */
> -  progname = concat (ldirname (python_libdir), SLASH_STRING, "bin",
> +  libdir = ldirname (python_libdir);
> +  progname = concat (libdir, SLASH_STRING, "bin",
>  		     SLASH_STRING, "python", (char *) NULL);
> +  xfree (libdir);

Let's restrict the new variable to the #if block that needs it.
I.e., declare the variable where is initialized, like:

  const char *libdir = ldirname (python_libdir);
  progname = concat (libdir, SLASH_STRING, "bin",

OK with that change.  Please push.

Note, you could have used reconcat instead of concat, avoiding the
xfree call, and maybe one reallocation, but that's hardly an
issue here.

Perhaps better overall would be to make ldirname return a std::string
and eliminate these leaks "by design".  It'd get rid of several
make_cleanup calls throughout too.  I'll give that a quick try.

Thanks,
Pedro Alves

Re: [PATCH] Fix memory leak in python.c:do_start_initialization

[Philipp Rudo]

On Wed, 22 Mar 2017 15:19:16 +0000
Pedro Alves <palves@redhat.com> wrote:

> Hi Philipp,
> 
> Thanks.
> 
> On 03/22/2017 01:11 PM, Philipp Rudo wrote:
> 
> > diff --git a/gdb/python/python.c b/gdb/python/python.c
> > index 73fb3d0..6b16613 100644
> > --- a/gdb/python/python.c
> > +++ b/gdb/python/python.c
> > @@ -1535,7 +1535,7 @@ extern initialize_file_ftype
> > _initialize_python; static bool
> >  do_start_initialization ()
> >  {
> > -  char *progname;
> > +  char *progname, *libdir;
> >  #ifdef IS_PY3K
> >    int i;
> >    size_t progsize, count;
> > @@ -1550,8 +1550,10 @@ do_start_initialization ()
> >       /foo/bin/python
> >       /foo/lib/pythonX.Y/...
> >       This must be done before calling Py_Initialize.  */
> > -  progname = concat (ldirname (python_libdir), SLASH_STRING, "bin",
> > +  libdir = ldirname (python_libdir);
> > +  progname = concat (libdir, SLASH_STRING, "bin",
> >  		     SLASH_STRING, "python", (char *) NULL);
> > +  xfree (libdir);  
> 
> Let's restrict the new variable to the #if block that needs it.
> I.e., declare the variable where is initialized, like:
> 
>   const char *libdir = ldirname (python_libdir);
>   progname = concat (libdir, SLASH_STRING, "bin",
> 
> OK with that change.  Please push.

Shall I fix it? Or do we go with your ldirname fix?

> 
> Note, you could have used reconcat instead of concat, avoiding the
> xfree call, and maybe one reallocation, but that's hardly an
> issue here.

Thought about using reconcat.  But in the end reconcat would have done
the same -- calling free on libdir.  Thats why I decided it would be
better readable when not hidden.  

> Perhaps better overall would be to make ldirname return a std::string
> and eliminate these leaks "by design".  It'd get rid of several
> make_cleanup calls throughout too.  I'll give that a quick try.

Or maybe combining all path handling in a 'class gdbpath'.  But
make ldirname return std::string definitely is an advantage.

> 
> Thanks,
> Pedro Alves
> 

Re: [PATCH] Fix memory leak in python.c:do_start_initialization

[Pedro Alves]

On 03/22/2017 05:52 PM, Philipp Rudo wrote:

>> OK with that change.  Please push.
> 
> Shall I fix it? Or do we go with your ldirname fix?

Please push in your fix first.  It's small and obviously correct.

>> Perhaps better overall would be to make ldirname return a std::string
>> and eliminate these leaks "by design".  It'd get rid of several
>> make_cleanup calls throughout too.  I'll give that a quick try.
> 
> Or maybe combining all path handling in a 'class gdbpath'.  But
> make ldirname return std::string definitely is an advantage.

That'd be nice.  If we do that, I think we should model the API on
C++17's filesystem::path http://en.cppreference.com/w/cpp/filesystem/path , to
ease a future transition.

Thanks,
Pedro Alves