On Mon, Sep 30, 2024 at 05:27:09PM -0400, Taylor Blau wrote:
> 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));
> }
Potentially. Until now we never really cared all that much about
releasing resources in error paths, and for all I can see this leak
isn't hit in our test suite. But it is an easy fix to make, so I can
include this in case I'll have to reroll this series due to other
reasons.
Patrick
