From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 30 Nov 2023 10:18:31 GMT
Subject: Re: [PATCH 02/24] pack-bitmap-write: deep-clear the `bb_commit` slab
Message-ID: <ZWhhd5gLzYwPEgBl@tanuki>
In-Reply-To: <6f5ff96998946f3f49da56fd05c096b949521339.1701198172.git.me@ttaylorr.com>

On Tue, Nov 28, 2023 at 02:07:59PM -0500, Taylor Blau wrote:
> The `bb_commit` commit slab is used by the pack-bitmap-write machinery
> to track various pieces of bookkeeping used to generate reachability
> bitmaps.
> 
> Even though we clear the slab when freeing the bitmap_builder struct
> (with `bitmap_builder_clear()`), there are still pointers which point to
> locations in memory that have not yet been freed, resulting in a leak.
> 
> Plug the leak by introducing a suitable `free_fn` for the `struct
> bb_commit` type, and make sure it is called on each member of the slab
> via the `deep_clear_bb_data()` function.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  pack-bitmap-write.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index f4ecdf8b0e..dd3a415b9d 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -198,6 +198,13 @@ struct bb_commit {
>  	unsigned idx; /* within selected array */
>  };
>  
> +static void clear_bb_commit(struct bb_commit *commit)
> +{
> +	free(commit->reverse_edges);

I'd have expected to see `free_commit_list()` here instead of a simple
free. Is there any reason why we don't use it?

Patrick

> +	bitmap_free(commit->commit_mask);
> +	bitmap_free(commit->bitmap);
> +}
> +
>  define_commit_slab(bb_data, struct bb_commit);
>  
>  struct bitmap_builder {
> @@ -339,7 +346,7 @@ static void bitmap_builder_init(struct bitmap_builder *bb,
>  
>  static void bitmap_builder_clear(struct bitmap_builder *bb)
>  {
> -	clear_bb_data(&bb->data);
> +	deep_clear_bb_data(&bb->data, clear_bb_commit);
>  	free(bb->commits);
>  	bb->commits_nr = bb->commits_alloc = 0;
>  }
> -- 
> 2.43.0.24.g980b318f98
>