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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 30, 2024, 21:27 UTC
Message-ID: <ZvsXrQadrU78rEha@nand.local>
In-Reply-To: <5db4e17db9b9b991e0dbe56d7043b56e5d161097.1727687410.git.ps@pks.im>

On Mon, Sep 30, 2024 at 11:14:01AM +0200, Patrick Steinhardt wrote:

Show 15 quoted lines

> 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));
    }

Thanks, Taylor