On Mon, Sep 30, 2024 at 11:14:01AM +0200, Patrick Steinhardt wrote:
> The buffer used to compute the final MIDX name is never released. Plug
> this memory leak.
>
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> midx-write.c | 2 ++
> t/t5334-incremental-multi-pack-index.sh | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/midx-write.c b/midx-write.c
> index 1ef62c4f4b..625c7d3137 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir,
> return -1;
Would you also want to strbuf_release() the final_midx_name buffer here
as well?
I guess the point is moot since there are a number of other finalization
steps that we likewise skip here by returning -1 directly instead of
jumping to the cleanup sub-routine.
In the above sense I'm OK with it as-is, but it would be nice to verify
that this portion of the code is leak-free even when we return early
(e.g., because we couldn't rename() the tempfile into place).
Of course, because final_midx_name is local to the body of this
conditional, I think you'd need to do something like:
if (ctx.incremntal) {
struct strbuf final_midx_name = STRBUF_INIT;
/* ... */
get_split_midx_filename_ext(&final_midx_name, object_dir,
midx_hash, MIDX_EXT_MIDX);
if (rename_tempfile(&incr, final_midx_name.buf) < 0) {
result = error_errno(_("unable to rename new multi-pack-index layer"));
strbuf_release(&final_midx_name); /* <- here */
goto cleanup;
}
strbuf_release(&final_midx_name); /* ... <- and here */
keep_hashes[ctx.num_multi_pack_indexes_before] =
xstrdup(hash_to_hex(midx_hash));
}
Thanks,
Taylor
