Re: [PATCH] solib_open, memory leak

[Jim Blandy <jimb@codesourcery.com>]

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?