Subject: Re: [PATCH 01/24] pack-objects: free packing_data in more places

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 30 Nov 2023 11:18:26 +0100

On Tue, Nov 28, 2023 at 02:07:57PM -0500, Taylor Blau wrote:
> 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 <[email-masked]>
> ---
>  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

> +{
> +	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
>