Re: [PATCH 14/23] pseudo-merge: fix various memory leaks

[Taylor Blau <me@ttaylorr.com>]

On Mon, Sep 30, 2024 at 11:13:51AM +0200, Patrick Steinhardt wrote:
> diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
> index 4dc0fe8e40..6413dd1731 100644
> --- a/pack-bitmap-write.c
> +++ b/pack-bitmap-write.c
> @@ -64,6 +64,12 @@ static void free_pseudo_merge_commit_idx(struct pseudo_merge_commit_idx *idx)
>  	free(idx);
>  }
>
> +static void pseudo_merge_group_release_cb(void *payload, const char *name UNUSED)
> +{
> +	pseudo_merge_group_release(payload);
> +	free(payload);
> +}
> +
>  void bitmap_writer_free(struct bitmap_writer *writer)
>  {
>  	uint32_t i;
> @@ -82,6 +88,8 @@ void bitmap_writer_free(struct bitmap_writer *writer)
>  	kh_foreach_value(writer->pseudo_merge_commits, idx,
>  			 free_pseudo_merge_commit_idx(idx));
>  	kh_destroy_oid_map(writer->pseudo_merge_commits);
> +	string_list_clear_func(&writer->pseudo_merge_groups,
> +			       pseudo_merge_group_release_cb);

Looking good, and this is the right spot to free the pseudo-merge
groups. Note for other readers: pseudo-merge "groups" are a utility
structure that is only used while writing pseudo-merge bitmaps, so we
don't expect to see any corresponding pseudo_merge_group_release() calls
sprinkled throughout, e.g., pack-bitmap.c.

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 9d9b8c4bfb..32b222a7af 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1390,8 +1390,8 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,
>  		}
>
>  		base = bitmap_new();
> -		if (!cascade_pseudo_merges_1(bitmap_git, base, roots_bitmap))
> -			bitmap_free(roots_bitmap);
> +		cascade_pseudo_merges_1(bitmap_git, base, roots_bitmap);
> +		bitmap_free(roots_bitmap);

I probably would have pulled this leakfix into its own separate patch,
since it's not immediately obvious how it's related to other changes in
the same patch.

But the change here is definitely correct. We initialize the
roots_bitmap field to just the tips of our traversal. I wrote some
details about why in 11d45a6e6a (pack-bitmap.c: use pseudo-merges during
traversal, 2024-05-23), but the gist is as follows: We want to avoid
accidentally thinking that roots which aren't part of some satisfied
pseudo-merge or existing bitmap are part of the reachability closure,
leaving those bits as dangling and leading to incorrect results.

In any event, we definitely do not need the roots_bitmap outside of that
block (regardless of whether or not we successfully cascaded any
pseudo-merges), so free-ing it here is the right thing to do.

> diff --git a/pseudo-merge.c b/pseudo-merge.c
> index 10ebd9a4e9..28782a31c6 100644
> --- a/pseudo-merge.c
> +++ b/pseudo-merge.c
> @@ -97,6 +97,25 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group)
>  	group->stable_size = DEFAULT_PSEUDO_MERGE_STABLE_SIZE;
>  }
>
> +void pseudo_merge_group_release(struct pseudo_merge_group *group)
> +{
> +	struct hashmap_iter iter;
> +	struct strmap_entry *e;
> +
> +	regfree(group->pattern);
> +	free(group->pattern);
> +
> +	strmap_for_each_entry(&group->matches, &iter, e) {
> +		struct pseudo_merge_matches *matches = e->value;
> +		free(matches->stable);
> +		free(matches->unstable);
> +		free(matches);
> +	}
> +	strmap_clear(&group->matches, 0);
> +
> +	free(group->merges);
> +}
> +

All looks good here, I didn't see any fields that were missed or
over-eagerly free'd here.

Thanks,
Taylor