Re: [RFA] Fix memory leak in windows_xfer_shared_libraries

Re: [RFA] Fix memory leak in windows_xfer_shared_libraries

[Pedro Alves]

On 12/13/2012 11:11 AM, Pierre Muller wrote:
> --- windows-nat.c       13 Nov 2012 09:46:10 -0000      1.236
> +++ windows-nat.c       13 Dec 2012 10:54:18 -0000
> @@ -2411,11 +2411,11 @@ windows_xfer_shared_libraries (struct ta
>    buf = obstack_finish (&obstack);
>    len_avail = strlen (buf);
>    if (offset >= len_avail)
> -    return 0;
> -
> -  if (len > len_avail - offset)
> +    len= 0
> +  else if (len > len_avail - offset)
>      len = len_avail - offset;
> -  memcpy (readbuf, buf + offset, len);
> +  if (len > 0)
> +    memcpy (readbuf, buf + offset, len);
> 

You can avoid the last if by writing as:

  if (offset >= len_avail)
    len = 0;
  else
    {
      if (len > len_avail - offset)
	len = len_avail - offset;
      memcpy (readbuf, buf + offset, len);
    }

I'd prefer that, but patch is okay either way.

>    obstack_free (&obstack, NULL);
>    return len;

>
>   I was also wondering if it would not be better to keep the obstack in
> between the two calls, but that would probably require some static variable
> :(

That'd be fine.  We actually do that in some cases in gdbserver, like
handle_qxfer_threads and handle_qxfer_traceframe_info.  It just didn't
look like worth it enough to bother when I initially wrote this.

-- 
Pedro Alves

RE: [RFA] Fix memory leak in windows_xfer_shared_libraries

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : jeudi 13 décembre 2012 21:16
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] Fix memory leak in windows_xfer_shared_libraries
> 
> On 12/13/2012 11:11 AM, Pierre Muller wrote:
> > --- windows-nat.c       13 Nov 2012 09:46:10 -0000      1.236
> > +++ windows-nat.c       13 Dec 2012 10:54:18 -0000
> > @@ -2411,11 +2411,11 @@ windows_xfer_shared_libraries (struct ta
> >    buf = obstack_finish (&obstack);
> >    len_avail = strlen (buf);
> >    if (offset >= len_avail)
> > -    return 0;
> > -
> > -  if (len > len_avail - offset)
> > +    len= 0
> > +  else if (len > len_avail - offset)
> >      len = len_avail - offset;
> > -  memcpy (readbuf, buf + offset, len);
> > +  if (len > 0)
> > +    memcpy (readbuf, buf + offset, len);
> >
> 
> You can avoid the last if by writing as:
> 
>   if (offset >= len_avail)
>     len = 0;
>   else
>     {
>       if (len > len_avail - offset)
> 	len = len_avail - offset;
>       memcpy (readbuf, buf + offset, len);
>     }
> 
> I'd prefer that, but patch is okay either way.

  I committed with your modification.
Thanks for the approval.

> >    obstack_free (&obstack, NULL);
> >    return len;
> 
> >
> >   I was also wondering if it would not be better to keep the obstack in
> > between the two calls, but that would probably require some static
> variable
> > :(
> 
> That'd be fine.  We actually do that in some cases in gdbserver, like
> handle_qxfer_threads and handle_qxfer_traceframe_info.  It just didn't
> look like worth it enough to bother when I initially wrote this.

  I was wondering if this would become a problem if we later add support for
multiple inferior
for windows-nat
   I vaguely remember that I tried to achieve this a long time ago...

  Anyhow, the memory leak is gone at least!

Pierre  

Re: [RFA] Fix memory leak in windows_xfer_shared_libraries

[Pedro Alves]

On 12/14/2012 07:53 AM, Pierre Muller wrote:

>>>   I was also wondering if it would not be better to keep the obstack in
>>> between the two calls, but that would probably require some static
>> variable
>>> :(
>>
>> That'd be fine.  We actually do that in some cases in gdbserver, like
>> handle_qxfer_threads and handle_qxfer_traceframe_info.  It just didn't
>> look like worth it enough to bother when I initially wrote this.
> 
>   I was wondering if this would become a problem if we later add support for
> multiple inferior
> for windows-nat

I don't think so.

>    I vaguely remember that I tried to achieve this a long time ago...

ISTR you had an archer branch for that and other Windows stuff.

> 
>   Anyhow, the memory leak is gone at least!

Thanks.

-- 
Pedro Alves

[RFA] Fix other memory leak in solib_target_current_sos

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : vendredi 14 décembre 2012 10:45
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] Fix memory leak in windows_xfer_shared_libraries
> 
> On 12/14/2012 07:53 AM, Pierre Muller wrote:
> 
> >>>   I was also wondering if it would not be better to keep the obstack
in
> >>> between the two calls, but that would probably require some static
> >> variable
> >>> :(
> >>
> >> That'd be fine.  We actually do that in some cases in gdbserver, like
> >> handle_qxfer_threads and handle_qxfer_traceframe_info.  It just didn't
> >> look like worth it enough to bother when I initially wrote this.
> >
> >   I was wondering if this would become a problem if we later add support
> for
> > multiple inferior
> > for windows-nat
> 
> I don't think so.
> 
> >    I vaguely remember that I tried to achieve this a long time ago...
> 
> ISTR you had an archer branch for that and other Windows stuff.

  Oh, no...
  I completely forgot that :(
It's been a loooong  time since I last worked on that :(
> >
> >   Anyhow, the memory leak is gone at least!


  I found another leak, down the same line,
solib_target_current_sos
  calls target_read_stralloc
which as its name suggests allocates the result on the heap...
But that string was not freed in the current code...

  This patch fixes that leak.



2012-12-14  Pierre Muller  <muller@sourceware.org>

        * solib-target.c (solib_target_current_sos): Remove 'const'
        qualifier from type of library_document local variable to be
        able to free it and avoid a memory leak.

Index: src/gdb/solib-target.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-target.c,v
retrieving revision 1.24
diff -u -p -r1.24 solib-target.c
--- src/gdb/solib-target.c      4 Jan 2012 08:17:11 -0000       1.24
+++ src/gdb/solib-target.c      14 Dec 2012 10:17:58 -0000
@@ -246,7 +246,7 @@ static struct so_list *
 solib_target_current_sos (void)
 {
   struct so_list *new_solib, *start = NULL, *last = NULL;
-  const char *library_document;
+  char *library_document;
   VEC(lm_info_p) *library_list;
   struct lm_info *info;
   int ix;
@@ -261,7 +261,10 @@ solib_target_current_sos (void)
   /* Parse the list.  */
   library_list = solib_target_parse_libraries (library_document);
   if (library_list == NULL)
-    return NULL;
+    {
+      xfree (library_document);
+      return NULL;
+    }

   /* Build a struct so_list for each entry on the list.  */
   for (ix = 0; VEC_iterate (lm_info_p, library_list, ix, info); ix++)
@@ -291,6 +294,9 @@ solib_target_current_sos (void)
   /* Free the library list, but not its members.  */
   VEC_free (lm_info_p, library_list);

+  /* Also free allocated library_document string.  */
+  xfree (library_document);
+
   return start;
 }

Re: [RFA] Fix other memory leak in solib_target_current_sos

[Pedro Alves]

On 12/14/2012 10:25 AM, Pierre Muller wrote:

>   I found another leak, down the same line,
> solib_target_current_sos
>   calls target_read_stralloc
> which as its name suggests allocates the result on the heap...
> But that string was not freed in the current code...
> 
>   This patch fixes that leak.

Thanks.

> 2012-12-14  Pierre Muller  <muller@sourceware.org>
> 
>         * solib-target.c (solib_target_current_sos): Remove 'const'
>         qualifier from type of library_document local variable to be
>         able to free it and avoid a memory leak.
> 
> Index: src/gdb/solib-target.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib-target.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 solib-target.c
> --- src/gdb/solib-target.c      4 Jan 2012 08:17:11 -0000       1.24
> +++ src/gdb/solib-target.c      14 Dec 2012 10:17:58 -0000
> @@ -246,7 +246,7 @@ static struct so_list *
>  solib_target_current_sos (void)
>  {
>    struct so_list *new_solib, *start = NULL, *last = NULL;
> -  const char *library_document;
> +  char *library_document;
>    VEC(lm_info_p) *library_list;
>    struct lm_info *info;
>    int ix;
> @@ -261,7 +261,10 @@ solib_target_current_sos (void)
>    /* Parse the list.  */
>    library_list = solib_target_parse_libraries (library_document);
>    if (library_list == NULL)
> -    return NULL;
> +    {
> +      xfree (library_document);
> +      return NULL;
> +    }
> 
>    /* Build a struct so_list for each entry on the list.  */
>    for (ix = 0; VEC_iterate (lm_info_p, library_list, ix, info); ix++)
> @@ -291,6 +294,9 @@ solib_target_current_sos (void)
>    /* Free the library list, but not its members.  */
>    VEC_free (lm_info_p, library_list);
> 
> +  /* Also free allocated library_document string.  */
> +  xfree (library_document);
> +
>    return start;
>  }
> 

Parsing XML, within solib_target_parse_libraries, may throw.  Thus, a
cleanup would be better:

+  old_chain = make_cleanup (xfree, library_document);
  library_list = solib_target_parse_libraries (library_document);
+  do_cleanups (old_chain);

-- 
Pedro Alves

[RFA-v2] Fix other memory leak in solib_target_current_sos

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pedro Alves
> Envoyé : vendredi 14 décembre 2012 11:55
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: [RFA] Fix other memory leak in solib_target_current_sos
> 
> On 12/14/2012 10:25 AM, Pierre Muller wrote:
> 
> >   I found another leak, down the same line,
> > solib_target_current_sos
> >   calls target_read_stralloc
> > which as its name suggests allocates the result on the heap...
> > But that string was not freed in the current code...
> >
> >   This patch fixes that leak.
> 
> Thanks.
> 
> > 2012-12-14  Pierre Muller  <muller@sourceware.org>
> >
> >         * solib-target.c (solib_target_current_sos): Remove 'const'
> >         qualifier from type of library_document local variable to be
> >         able to free it and avoid a memory leak.
> >
> > Index: src/gdb/solib-target.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/solib-target.c,v
> > retrieving revision 1.24
> > diff -u -p -r1.24 solib-target.c
> > --- src/gdb/solib-target.c      4 Jan 2012 08:17:11 -0000       1.24
> > +++ src/gdb/solib-target.c      14 Dec 2012 10:17:58 -0000
> > @@ -246,7 +246,7 @@ static struct so_list *
> >  solib_target_current_sos (void)
> >  {
> >    struct so_list *new_solib, *start = NULL, *last = NULL;
> > -  const char *library_document;
> > +  char *library_document;
> >    VEC(lm_info_p) *library_list;
> >    struct lm_info *info;
> >    int ix;
> > @@ -261,7 +261,10 @@ solib_target_current_sos (void)
> >    /* Parse the list.  */
> >    library_list = solib_target_parse_libraries (library_document);
> >    if (library_list == NULL)
> > -    return NULL;
> > +    {
> > +      xfree (library_document);
> > +      return NULL;
> > +    }
> >
> >    /* Build a struct so_list for each entry on the list.  */
> >    for (ix = 0; VEC_iterate (lm_info_p, library_list, ix, info); ix++)
> > @@ -291,6 +294,9 @@ solib_target_current_sos (void)
> >    /* Free the library list, but not its members.  */
> >    VEC_free (lm_info_p, library_list);
> >
> > +  /* Also free allocated library_document string.  */
> > +  xfree (library_document);
> > +
> >    return start;
> >  }
> >
> 
> Parsing XML, within solib_target_parse_libraries, may throw.  Thus, a
> cleanup would be better:
> 
> +  old_chain = make_cleanup (xfree, library_document);
>   library_list = solib_target_parse_libraries (library_document);
> +  do_cleanups (old_chain);

Of course...
  library_document isn't needed after that point anymore.

New version of the patch below.

Pierre

2012-12-14  Pierre Muller  <muller@sourceware.org>
	      Pedro Alves  <palves@redhat.com>

        * solib-target.c (solib_target_current_sos): Remove 'const'
        qualifier from type of library_document local variable to be
        able to free it and avoid a memory leak.
        Use cleanup chain to avoid leak even if exceptino is generated.


Index: solib-target.c
===================================================================
RCS file: /cvs/src/src/gdb/solib-target.c,v
retrieving revision 1.24
diff -u -p -r1.24 solib-target.c
--- solib-target.c      4 Jan 2012 08:17:11 -0000       1.24
+++ solib-target.c      14 Dec 2012 13:03:13 -0000
@@ -246,7 +246,8 @@ static struct so_list *
 solib_target_current_sos (void)
 {
   struct so_list *new_solib, *start = NULL, *last = NULL;
-  const char *library_document;
+  char *library_document;
+  struct cleanup *old_chain;
   VEC(lm_info_p) *library_list;
   struct lm_info *info;
   int ix;
@@ -258,8 +259,15 @@ solib_target_current_sos (void)
   if (library_document == NULL)
     return NULL;

+  /* solib_target_parse_libraries may throw, so we use a cleanup.  */
+  old_chain = make_cleanup (xfree, library_document);
+
   /* Parse the list.  */
   library_list = solib_target_parse_libraries (library_document);
+
+  /* library_document string is not needed behind this point.  */
+  do_cleanups (old_chain);
+
   if (library_list == NULL)
     return NULL;

Re: [RFA-v2] Fix other memory leak in solib_target_current_sos

[Pedro Alves]

On 12/14/2012 01:07 PM, Pierre Muller wrote:

> New version of the patch below.

OK.

Thanks.

-- 
Pedro Alves

Re: [RFA] Fix other memory leak in solib_target_current_sos

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>    library_list = solib_target_parse_libraries (library_document);
Pierre>    if (library_list == NULL)
Pierre> -    return NULL;
Pierre> +    {
Pierre> +      xfree (library_document);
Pierre> +      return NULL;
Pierre> +    }

It seems to me that you could unconditionally free it here, before the if.

Tom

RE: [RFA] Fix other memory leak in solib_target_current_sos

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : vendredi 14 décembre 2012 15:14
> À : Pierre Muller
> Cc : 'Pedro Alves'; gdb-patches@sourceware.org
> Objet : Re: [RFA] Fix other memory leak in solib_target_current_sos
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
writes:
> 
> Pierre>    library_list = solib_target_parse_libraries (library_document);
> Pierre>    if (library_list == NULL)
> Pierre> -    return NULL;
> Pierre> +    {
> Pierre> +      xfree (library_document);
> Pierre> +      return NULL;
> Pierre> +    }
> 
> It seems to me that you could unconditionally free it here, before the if.

  That's what the committed version does
thanks to Pedro's suggestion....

Pierre