Re: [PATCH v2 01/10] pack-write.c: plug a leak in stage_tmp_packfiles()

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 20, 2023, 16:31 UTC
Message-ID: <ZEFo4iGFTg1UWpL0@nand.local>
In-Reply-To: <xmqqpm7z7crz.fsf@gitster.g>

On Wed, Apr 19, 2023 at 03:00:48PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
>
> > The function `stage_tmp_packfiles()` generates a filename to use for
> > staging the contents of what will become the pack's ".mtimes" file.
> >
> > The name is generated in `write_mtimes_file()` and the result is
> > returned back to `stage_tmp_packfiles()` which uses it to rename the
> > temporary file into place via `rename_tmp_packfiles()`.
> >
> > `write_mtimes_file()` returns a `const char *`, indicating that callers
> > are not expected to free its result (similar to, e.g., `oid_to_hex()`).
> > But callers are expected to free its result, so this return type is
> > incorrect.
>
> Indeed the string that holds the name of the file returned by
> write_mtimes_file() is leaking.  Does the same logic apply to the
> returned filename from write_rev_file() and stored in rev_tmp_name
> that is not freed in stage_tmp_packfiles() in another topic?

They are similar, but unfortunately different.

Here, our temporary name is generated by `write_mtimes_file()` which *always* comes up with a new name from scratch. So we know that it should always be free()'d at the end of `stage_tmp_packfiles()`.

But in the case of `write_rev_file()`, it only *sometimes* generates a new name from scratch. The first parameter can be NULL, in which case `write_rev_file()` will generate a new name. Or it can be non-NULL, in which case that name will be used instead.

So in that topic, it's less clear on what the ultimate right path forward is. But in this topic, changing `mtimes_tmp_name` and the return type of `write_mtimes_file()` to be a non-const `char *` (and free()ing it, of course ;-)) is the right thing to do.

Thanks, Taylor