[RFA] Fix memory leak in windows_xfer_shared_libraries

[RFA] Fix memory leak in windows_xfer_shared_libraries

[Pierre Muller]

  The current mechanism of getting the list of DLLs when command
infl dll 
is given to gdb prompt,
info_shared_library function in solib.c calls
  windows_xfer_shared_libraries in windows-nat.c

  using target_read_stralloc, which calls target_read_alloc_1.

  That function reiterates calls to target_read_partial
until the number of transferred bytes is zero...

  This results even if the buffer is large enough to contain all data at
first
call in a second call in which the same xml answer is computed again,
and nothing is done, because the offset correspond to the end of the
resulting
string.

  The current code has a memory leak that is fixed by the patch below.

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

Pierre Muller
GDB pascal language maintainer


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

        * windows-nat.c (windows_xfer_shared_libraries): Avoid
        memory leak when OFFSET >= LEN_AVAIL.

Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.236
diff -u -p -r1.236 windows-nat.c
--- 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);

   obstack_free (&obstack, NULL);
   return len;

RE: [RFA] Fix memory leak in windows_xfer_shared_libraries

[Pierre Muller]

 Whoops, 
  I forgot to test my patch :(

  Once again, got bitten by a difference
in syntax between C and pascal...
  a semicolon was missing before the else keyword...


Sorry about that,

Pierre  



> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Pierre Muller
> Envoyé : jeudi 13 décembre 2012 12:11
> À : gdb-patches@sourceware.org
> Objet : [RFA] Fix memory leak in windows_xfer_shared_libraries
> 
>   The current mechanism of getting the list of DLLs when command
> infl dll
> is given to gdb prompt,
> info_shared_library function in solib.c calls
>   windows_xfer_shared_libraries in windows-nat.c
> 
>   using target_read_stralloc, which calls target_read_alloc_1.
> 
>   That function reiterates calls to target_read_partial
> until the number of transferred bytes is zero...
> 
>   This results even if the buffer is large enough to contain all data at
> first
> call in a second call in which the same xml answer is computed again,
> and nothing is done, because the offset correspond to the end of the
> resulting
> string.
> 
>   The current code has a memory leak that is fixed by the patch below.
> 
>   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
> :(
> 

Fixed patch:

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

	* windows-nat.c (windows_xfer_shared_libraries): Avoid
	memory leak when OFFSET >= LEN_AVAIL.

Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.236
diff -u -p -r1.236 windows-nat.c
--- 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);
 
   obstack_free (&obstack, NULL);
   return len;

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

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