Re: [PATCH 17/23] midx-write: fix leaking buffer

[Patrick Steinhardt <ps@pks.im>]

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@pks.im>
> > ---
> >  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