Re: [PATCH] solib_open, memory leak

[msnyder@sonic.net]

[-- 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;