Re: [PATCH] solib_open, memory leak

Jim Blandy
2007-08-08T21:46:55Z

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?