Jeff King <peff@xxxxxxxx> writes:
>> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the
>> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the
>> &tmp you pass in here) and then "does a "goto error_out".
>>
>> It then *resets* the strbuf, but doesn't release it, assuming that
>> you're going to pass it in again. So in that case we'd leak here, no?
>>
>> I.e. a NULL return value from strbuf_realpath() doesn't mean that it
>> didn't allocate in the scratch area passed to it, so we need to
>> strbuf_release(&tmp) here too.
>
> We don't use MANY_MISSING in this code path, but I didn't read
> strbuf_realpath_1() carefully enough to see if that is the only case.
> But regardless, I think it is a bug in strbuf_realpath(). All of the
> strbuf functions generally try to leave a buffer untouched on error.
>
> So IMHO we would want a preparatory patch with s/reset/release/ in that
> function, which better matches the intent (we might be freeing an
> allocated buffer, but that's OK from the caller perspective).
Is that always OK? I would think that we'd do something closer to
strbuf_getcwd():
int strbuf_getcwd(struct strbuf *sb)
{
size_t oldalloc = sb->alloc;
/* ... */
if (oldalloc == 0)
strbuf_release(sb);
else
strbuf_reset(sb);
}
i.e. if the caller passed in a strbuf with allocated contents, they're
responsible for free()-ing it, otherwise we free() it. That does fix the
leak in this patch, but I don't feel strongly enough about changing
strbuf_realpath() to do it now, so I'll do without the change for now.
