Fix memory leak in python.c:do_start_initialization

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

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
  

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
>
  

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