Re: [PATCH 01/24] pack-objects: free packing_data in more places
On Tue, Nov 28, 2023 at 02:07:57PM -0500, Taylor Blau wrote:
Show 59 quoted lines
> The pack-objects internals use a packing_data struct to track what
> objects are part of the pack(s) being formed.
>
> Since these structures contain allocated fields, failing to
> appropriately free() them results in a leak. Plug that leak by
> introducing a free_packing_data() function, and call it in the
> appropriate spots.
>
> This is a fairly straightforward leak to plug, since none of the callers
> expect to read any values or have any references to parts of the address
> space being freed.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> builtin/pack-objects.c | 1 +
> midx.c | 5 +++++
> pack-objects.c | 15 +++++++++++++++
> pack-objects.h | 1 +
> 4 files changed, 22 insertions(+)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 89a8b5a976..bfa60359d4 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4522,6 +4522,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> reuse_packfile_objects);
>
> cleanup:
> + free_packing_data(&to_pack);
> list_objects_filter_release(&filter_options);
> strvec_clear(&rp);
>
> diff --git a/midx.c b/midx.c
> index 2f3863c936..3b727dc633 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1592,8 +1592,13 @@ static int write_midx_internal(const char *object_dir,
> flags) < 0) {
> error(_("could not write multi-pack bitmap"));
> result = 1;
> + free_packing_data(&pdata);
> + free(commits);
> goto cleanup;
> }
> +
> + free_packing_data(&pdata);
> + free(commits);
> }
> /*
> * NOTE: Do not use ctx.entries beyond this point, since it might
> diff --git a/pack-objects.c b/pack-objects.c
> index f403ca6986..1c7bedcc94 100644
> --- a/pack-objects.c
> +++ b/pack-objects.c
> @@ -151,6 +151,21 @@ void prepare_packing_data(struct repository *r, struct packing_data *pdata)
> init_recursive_mutex(&pdata->odb_lock);
> }
>
> +void free_packing_data(struct packing_data *pdata)Nit: shouldn't this rather be called `clear_packing_data`? `free` to me
indicates that the data structure itself will be free'd, as well, which
is not the case.
Patrick
Show 32 quoted lines
> +{
> + if (!pdata)
> + return;
> +
> + free(pdata->cruft_mtime);
> + free(pdata->in_pack);
> + free(pdata->in_pack_by_idx);
> + free(pdata->in_pack_pos);
> + free(pdata->index);
> + free(pdata->layer);
> + free(pdata->objects);
> + free(pdata->tree_depth);
> +}
> +
> struct object_entry *packlist_alloc(struct packing_data *pdata,
> const struct object_id *oid)
> {
> diff --git a/pack-objects.h b/pack-objects.h
> index 0d78db40cb..336217e8cd 100644
> --- a/pack-objects.h
> +++ b/pack-objects.h
> @@ -169,6 +169,7 @@ struct packing_data {
> };
>
> void prepare_packing_data(struct repository *r, struct packing_data *pdata);
> +void free_packing_data(struct packing_data *pdata);
>
> /* Protect access to object database */
> static inline void packing_data_lock(struct packing_data *pdata)
> --
> 2.43.0.24.g980b318f98
> Patrick Steinhardt <ps@pks.im>