Re: [PATCH 1/7] pack-write.c: plug a leak in stage_tmp_packfiles()

[Taylor Blau <me@ttaylorr.com>]

On Tue, Apr 11, 2023 at 09:23:31AM -0400, Derrick Stolee wrote:
> On 4/10/2023 6:53 PM, Taylor Blau wrote:
>
> > Instead, simply free() `rev_tmp_name` at the end of
> > `stage_tmp_packfiles()`.
>
> > @@ -568,6 +568,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
> >  		rename_tmp_packfile(name_buffer, rev_tmp_name, "rev");
> >  	if (mtimes_tmp_name)
> >  		rename_tmp_packfile(name_buffer, mtimes_tmp_name, "mtimes");
> > +
> > +	free((char *)rev_tmp_name);
>
> Just cut off from the context is a "if (rev_tmp_name)", so it might be
> good to group this into that block, since we have the condition, anyway.

Definitely possible, though FWIW it's fine to have this free()
positioned at the end of the function, since we initialize rev_tmp_name
to NULL (making this a noop when not writing an on-disk reverse index).

> But I was also thinking about how we like to use "const" as an indicator
> as "I am not responsible for free()ing this". And this comes from the
> public write_rev_file() method. Based on the API prototype, we could
> think that this string is held by a static strbuf (making the method
> not reentrant, but that happens sometimes in our methods). But generally,
> I wanted to inspect what it would take to make the API reflect the fact
> that it can return a "new" string.
>
> But there are two issues:
>
>  1. The actual logic is inside write_rev_file_order(), so that API
>     needs to change, too.
>
>  2. The "new" string is created only if the rev_name parameter is
>     NULL, which is somewhat understandable but still requires
>     inside knowledge about the implementation to make that choice.
>
>  3. If we inspect the callers to these methods, only one caller
>     passes a non-null name: builtin/index-pack.c. The rest pass NULL,
>     including write_midx_reverse_index() (which then leaks the name).
>
> The below diff includes my attempt to change the API to return a
> non-const string that must be freed by the callers.

I like this direction. I think that all things being equal (and unless
you feel strongly about it in the meantime), I'd just as soon pursue
this as a "fast follow" to avoid intermixing this API change with the
primary intent of this series.

In the meantime, dropping the const via a cast down to "char *" works
fine to plug the leak here.

Thanks,
Taylor