repack: free existing_cruft array after use

Commit Message

Jeff King Oct. 7, 2023, 5:20 p.m. UTC

On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:

> +static void collapse_small_cruft_packs(FILE *in, size_t max_size,
> +				       struct existing_packs *existing)
> +{
> +	struct packed_git **existing_cruft, *p;
> +	struct strbuf buf = STRBUF_INIT;
> [...]
> +
> +	strbuf_release(&buf);
> +}

Coverity (using the just-merged-to-next version of the workflow file!)
flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in
'next', I think we'd want this on top:

-- >8 --
Subject: [PATCH] repack: free existing_cruft array after use

We allocate an array of packed_git pointers so that we can sort the list
of cruft packs, but we never free the array, causing a small leak. Note
that we don't need to free the packed_git structs themselves; they're
owned by the repository object.

Signed-off-by: Jeff King <[e-mail]>
---
 builtin/repack.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Taylor Blau Oct. 9, 2023, 1:24 a.m. UTC | #1

On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote:
> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:
>
> > +static void collapse_small_cruft_packs(FILE *in, size_t max_size,
> > +				       struct existing_packs *existing)
> > +{
> > +	struct packed_git **existing_cruft, *p;
> > +	struct strbuf buf = STRBUF_INIT;
> > [...]
> > +
> > +	strbuf_release(&buf);
> > +}
>
> Coverity (using the just-merged-to-next version of the workflow file!)
> flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in
> 'next', I think we'd want this on top:

Woohoo! I'm glad that this is already paying dividends.

> -- >8 --
> Subject: [PATCH] repack: free existing_cruft array after use
>
> We allocate an array of packed_git pointers so that we can sort the list
> of cruft packs, but we never free the array, causing a small leak. Note
> that we don't need to free the packed_git structs themselves; they're
> owned by the repository object.

Thanks, I can't believe I missed this when writing this function. The
fix looks obviously correct to me, so this has my:

    Acked-by: Taylor Blau <[e-mail]>

Thanks,
Taylor

Junio C Hamano Oct. 9, 2023, 5:28 p.m. UTC | #2

Taylor Blau <[e-mail]> writes:

> On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote:
>> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote:
>> ...
>> Coverity (using the just-merged-to-next version of the workflow file!)
>> flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in
>> 'next', I think we'd want this on top:
>
> Woohoo! I'm glad that this is already paying dividends.
> ...
> Thanks, I can't believe I missed this when writing this function. The
> fix looks obviously correct to me, so this has my:
>
>     Acked-by: Taylor Blau <[e-mail]>
>
> Thanks,

Will queue.  Thanks.

Patch

diff --git a/builtin/repack.c b/builtin/repack.c
index a1a893d952..69e8f302c0 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -955,6 +955,7 @@  static void collapse_small_cruft_packs(FILE *in, size_t max_size,
 			existing->non_kept_packs.items[i].string);
 
 	strbuf_release(&buf);
+	free(existing_cruft);
 }
 
 static int write_cruft_pack(const struct pack_objects_args *args,