[PATCH] solib_open, memory leak

[PATCH] solib_open, memory leak

[msnyder]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

I hope it's not getting to be too late at night for me to do this stuff...

If I'm not spacy, solib_open is leaking memory, because openp passes back
a malloc buffer for temp_pathname.  In order to free it, it has to always
be a malloc buffer (hence no alloca etc).


[-- Attachment #2: 138.txt --]
[-- Type: text/plain, Size: 1921 bytes --]

2007-08-04  Michael Snyder  <msnyder@access-company.com>

	* solib.c (solib_open): Memory leak -- openp returns xmalloc buffer.

Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.94
diff -p -r1.94 solib.c
*** solib.c	3 Jul 2007 12:14:43 -0000	1.94
--- solib.c	5 Aug 2007 03:13:49 -0000
*************** solib_open (char *in_pathname, char **fo
*** 156,162 ****
    gdb_sysroot_is_empty = (gdb_sysroot == NULL || *gdb_sysroot == 0);
  
    if (! IS_ABSOLUTE_PATH (in_pathname) || gdb_sysroot_is_empty)
!     temp_pathname = in_pathname;
    else
      {
        int prefix_len = strlen (gdb_sysroot);
--- 156,162 ----
    gdb_sysroot_is_empty = (gdb_sysroot == NULL || *gdb_sysroot == 0);
  
    if (! IS_ABSOLUTE_PATH (in_pathname) || gdb_sysroot_is_empty)
!     temp_pathname = xstrdup (in_pathname);
    else
      {
        int prefix_len = strlen (gdb_sysroot);
*************** solib_open (char *in_pathname, char **fo
*** 167,173 ****
  	prefix_len--;
  
        /* Cat the prefixed pathname together.  */
!       temp_pathname = alloca (prefix_len + strlen (in_pathname) + 1);
        strncpy (temp_pathname, gdb_sysroot, prefix_len);
        temp_pathname[prefix_len] = '\0';
        strcat (temp_pathname, in_pathname);
--- 167,173 ----
  	prefix_len--;
  
        /* Cat the prefixed pathname together.  */
!       temp_pathname = xmalloc (prefix_len + strlen (in_pathname) + 1);
        strncpy (temp_pathname, gdb_sysroot, prefix_len);
        temp_pathname[prefix_len] = '\0';
        strcat (temp_pathname, in_pathname);
*************** solib_open (char *in_pathname, char **fo
*** 226,231 ****
--- 226,233 ----
       (optionally) found_pathname.  */
    if (found_pathname != NULL && temp_pathname != NULL)
      *found_pathname = xstrdup (temp_pathname);
+ 
+   xfree (temp_pathname);
    return found_file;
  }
  

Re: [PATCH] solib_open, memory leak

[Jim Blandy]

msnyder@sonic.net writes:
> I hope it's not getting to be too late at night for me to do this stuff...
>
> If I'm not spacy, solib_open is leaking memory, because openp passes back
> a malloc buffer for temp_pathname.  In order to free it, it has to always
> be a malloc buffer (hence no alloca etc).

I think you were a little bit spacy.  :)

Even if you've made sure that temp_pathname is malloc'd by the time we
reach the 'open', any later 'openp' call will throw away its value.
openp is careful to store *something* in the pointer referred to by
its last argument, even on error.

I think the invariant should be that, when found_file becomes >= 0,
then temp_pathname is malloc'd, and not before.  The 'openp' clauses
will preserve that.  So I think you need:

  if (found_file >= 0)
    temp_pathname = xstrdup (temp_pathname);

after the 'open'.

And then there's no need for the xstrdup at the bottom; just return
temp_pathname, or xfree it if the caller doesn't want it.

Re: [PATCH] solib_open, memory leak

[Kevin Buettner]

On Mon, 06 Aug 2007 15:28:28 -0700
Jim Blandy <jimb@codesourcery.com> wrote:

> Even if you've made sure that temp_pathname is malloc'd by the time we
> reach the 'open', any later 'openp' call will throw away its value.
> openp is careful to store *something* in the pointer referred to by
> its last argument, even on error.
> 
> I think the invariant should be that, when found_file becomes >= 0,
> then temp_pathname is malloc'd, and not before.  The 'openp' clauses
> will preserve that.  So I think you need:
> 
>   if (found_file >= 0)
>     temp_pathname = xstrdup (temp_pathname);
> 
> after the 'open'.
> 
> And then there's no need for the xstrdup at the bottom; just return
> temp_pathname, or xfree it if the caller doesn't want it.

Jim's analysis looks right to me.

Kevin

Re: [PATCH] solib_open, memory leak

[msnyder]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

>
> msnyder@sonic.net writes:
>> I hope it's not getting to be too late at night for me to do this
>> stuff...
>>
>> If I'm not spacy, solib_open is leaking memory, because openp passes
>> back
>> a malloc buffer for temp_pathname.  In order to free it, it has to
>> always
>> be a malloc buffer (hence no alloca etc).
>
> I think you were a little bit spacy.  :)
>
> Even if you've made sure that temp_pathname is malloc'd by the time we
> reach the 'open', any later 'openp' call will throw away its value.
> openp is careful to store *something* in the pointer referred to by
> its last argument, even on error.

Uhhh, yeah, you're right.


> I think the invariant should be that, when found_file becomes >= 0,
> then temp_pathname is malloc'd, and not before.  The 'openp' clauses
> will preserve that.  So I think you need:
>
>   if (found_file >= 0)
>     temp_pathname = xstrdup (temp_pathname);
>
> after the 'open'.
>
> And then there's no need for the xstrdup at the bottom; just return
> temp_pathname, or xfree it if the caller doesn't want it.

That sounds great, except for one thing (which I had also not addressed).

If openp receives an xmalloc'd temp_pathname, it will clobber it
without freeing it.


I'm thinking that openp will never use the buffer, only the pointer.
So rather than what you suggest, what if we AVOID xmalloc before
openp and just make sure that we pass openp a null pointer or a pointer
to memory that does not need to be freed (such as alloca).

Umm, thus:


[-- Attachment #2: solib_open.txt --]
[-- Type: text/plain, Size: 2664 bytes --]

Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.94
diff -p -r1.94 solib.c
*** solib.c	3 Jul 2007 12:14:43 -0000	1.94
--- solib.c	8 Aug 2007 18:29:15 -0000
*************** solib_open (char *in_pathname, char **fo
*** 176,194 ****
    /* Now see if we can open it.  */
    found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0);
  
!   /* If the search in gdb_sysroot failed, and the path name is
!      absolute at this point, make it relative.  (openp will try and open the
!      file according to its absolute path otherwise, which is not what we want.)
!      Affects subsequent searches for this solib.  */
!   if (found_file < 0 && IS_ABSOLUTE_PATH (in_pathname))
!     {
!       /* First, get rid of any drive letters etc.  */
!       while (!IS_DIR_SEPARATOR (*in_pathname))
!         in_pathname++;
! 
!       /* Next, get rid of all leading dir separators.  */
!       while (IS_DIR_SEPARATOR (*in_pathname))
!         in_pathname++;
      }
    
    /* If not found, search the solib_search_path (if any).  */
--- 176,205 ----
    /* Now see if we can open it.  */
    found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0);
  
!   if (found_file < 0)
!     {
!       /* If the search in gdb_sysroot failed, and the path name is
!          absolute at this point, make it relative.  (openp will try
!          and open the file according to its absolute path otherwise,
!          which is not what we want.)  Affects subsequent searches for
!          this solib.  */
!       if (IS_ABSOLUTE_PATH (in_pathname))
! 	{
! 	  /* First, get rid of any drive letters etc.  */
! 	  while (!IS_DIR_SEPARATOR (*in_pathname))
! 	    in_pathname++;
! 
! 	  /* Next, get rid of all leading dir separators.  */
! 	  while (IS_DIR_SEPARATOR (*in_pathname))
! 	    in_pathname++;
! 	}
!       /* Previous temp_pathname buffer is no longer in use.  */
!       temp_pathname = NULL;
!     }
!   else if (temp_pathname)
!     {
!       /* Make a copy that can be returned.  */
!       temp_pathname = xstrdup (temp_pathname);
      }
    
    /* If not found, search the solib_search_path (if any).  */
*************** solib_open (char *in_pathname, char **fo
*** 224,229 ****
--- 235,247 ----
  
    /* Done.  If not found, tough luck.  Return found_file and 
       (optionally) found_pathname.  */
+   if (temp_pathname)
+     {
+       if (found_pathname)
+ 	*found_pathname = temp_pathname;
+       else
+ 	xfree (temp_pathname);
+     }
    if (found_pathname != NULL && temp_pathname != NULL)
      *found_pathname = xstrdup (temp_pathname);
    return found_file;

Re: [PATCH] solib_open, memory leak

[Jim Blandy]

msnyder@sonic.net writes:
>> I think the invariant should be that, when found_file becomes >= 0,
>> then temp_pathname is malloc'd, and not before.  The 'openp' clauses
>> will preserve that.  So I think you need:
>>
>>   if (found_file >= 0)
>>     temp_pathname = xstrdup (temp_pathname);
>>
>> after the 'open'.
>>
>> And then there's no need for the xstrdup at the bottom; just return
>> temp_pathname, or xfree it if the caller doesn't want it.
>
> That sounds great, except for one thing (which I had also not addressed).
>
> If openp receives an xmalloc'd temp_pathname, it will clobber it
> without freeing it.
>
>
> I'm thinking that openp will never use the buffer, only the pointer.

Right.

> So rather than what you suggest, what if we AVOID xmalloc before
> openp and just make sure that we pass openp a null pointer or a pointer
> to memory that does not need to be freed (such as alloca).
>
> Umm, thus:

You're right that temp_pathname needs to be NULL if we haven't found
something, so that the *found_pathname code at the end works right.

The code clearly deserves a comment like:

    /* We try to find the library in various ways.  After each
       attempt, either found_file >= 0 and temp_pathname is a malloc'd
       string, or found_file < 0 and temp_pathname does not point to
       storage that needs to be freed.  */

In that light, it seems clearer to me to just put:

   if (found_file < 0)
     temp_pathname = NULL;
   else
     temp_pathname = xstrdup (temp_pathname);

immediately after the open, to make it obvious that the rule is
followed there.  

The stuff about making in_pathname relative doesn't affect whether
anything is found or the allocatedness of temp_pathname, so it should
be left alone.

> *************** solib_open (char *in_pathname, char **fo
> *** 224,229 ****
> --- 235,247 ----
>   
>     /* Done.  If not found, tough luck.  Return found_file and 
>        (optionally) found_pathname.  */
> +   if (temp_pathname)
> +     {
> +       if (found_pathname)
> + 	*found_pathname = temp_pathname;
> +       else
> + 	xfree (temp_pathname);
> +     }
>     if (found_pathname != NULL && temp_pathname != NULL)
>       *found_pathname = xstrdup (temp_pathname);
>     return found_file;

Don't you want those new lines to replace the two that followed them,
not just precede them?

Re: [PATCH] solib_open, memory leak

[msnyder]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

[Jim:]
> The code clearly deserves a comment like:
>
>     /* We try to find the library in various ways.  After each
>        attempt, either found_file >= 0 and temp_pathname is a malloc'd
>        string, or found_file < 0 and temp_pathname does not point to
>        storage that needs to be freed.  */
>
> In that light, it seems clearer to me to just put:
>
>    if (found_file < 0)
>      temp_pathname = NULL;
>    else
>      temp_pathname = xstrdup (temp_pathname);
>
> immediately after the open, to make it obvious that the rule is
> followed there.
>
[...]
> Don't you want those new lines to replace the two that followed them,
> not just precede them?

Yes, thanks.  How about this?


[-- Attachment #2: solib_open2.txt --]
[-- Type: text/plain, Size: 1692 bytes --]

Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.94
diff -p -r1.94 solib.c
*** solib.c	3 Jul 2007 12:14:43 -0000	1.94
--- solib.c	8 Aug 2007 21:57:27 -0000
*************** solib_open (char *in_pathname, char **fo
*** 176,181 ****
--- 176,192 ----
    /* Now see if we can open it.  */
    found_file = open (temp_pathname, O_RDONLY | O_BINARY, 0);
  
+   /* We try to find the library in various ways.  After each attempt
+      (except for the one above), either found_file >= 0 and
+      temp_pathname is a malloc'd string, or found_file < 0 and
+      temp_pathname does not point to storage that needs to be
+      freed.  */
+ 
+     if (found_file < 0)
+       temp_pathname = NULL;
+     else
+       temp_pathname = xstrdup (temp_pathname);
+ 
    /* If the search in gdb_sysroot failed, and the path name is
       absolute at this point, make it relative.  (openp will try and open the
       file according to its absolute path otherwise, which is not what we want.)
*************** solib_open (char *in_pathname, char **fo
*** 224,231 ****
  
    /* Done.  If not found, tough luck.  Return found_file and 
       (optionally) found_pathname.  */
!   if (found_pathname != NULL && temp_pathname != NULL)
!     *found_pathname = xstrdup (temp_pathname);
    return found_file;
  }
  
--- 235,247 ----
  
    /* Done.  If not found, tough luck.  Return found_file and 
       (optionally) found_pathname.  */
!   if (temp_pathname)
!     {
!       if (found_pathname != NULL)
! 	*found_pathname = temp_pathname;
!       else
! 	xfree (temp_pathname);
!     }
    return found_file;
  }
  

Re: [PATCH] solib_open, memory leak

[Jim Blandy]

msnyder@sonic.net writes:
> [Jim:]
>> The code clearly deserves a comment like:
>>
>>     /* We try to find the library in various ways.  After each
>>        attempt, either found_file >= 0 and temp_pathname is a malloc'd
>>        string, or found_file < 0 and temp_pathname does not point to
>>        storage that needs to be freed.  */
>>
>> In that light, it seems clearer to me to just put:
>>
>>    if (found_file < 0)
>>      temp_pathname = NULL;
>>    else
>>      temp_pathname = xstrdup (temp_pathname);
>>
>> immediately after the open, to make it obvious that the rule is
>> followed there.
>>
> [...]
>> Don't you want those new lines to replace the two that followed them,
>> not just precede them?
>
> Yes, thanks.  How about this?

Poifect.

Re: [PATCH] solib_open, memory leak

[msnyder]

>
> msnyder@sonic.net writes:
>> [Jim:]
>>> The code clearly deserves a comment like:
>>>
>>>     /* We try to find the library in various ways.  After each
>>>        attempt, either found_file >= 0 and temp_pathname is a malloc'd
>>>        string, or found_file < 0 and temp_pathname does not point to
>>>        storage that needs to be freed.  */
>>>
>>> In that light, it seems clearer to me to just put:
>>>
>>>    if (found_file < 0)
>>>      temp_pathname = NULL;
>>>    else
>>>      temp_pathname = xstrdup (temp_pathname);
>>>
>>> immediately after the open, to make it obvious that the rule is
>>> followed there.
>>>
>> [...]
>>> Don't you want those new lines to replace the two that followed them,
>>> not just precede them?
>>
>> Yes, thanks.  How about this?
>
> Poifect.

Committed.