Re: [PATCH 14/23] pseudo-merge: fix various memory leaks
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Sep 30, 2024, 16:21 UTC
- Message-ID
- <ZvrQByUPoHhlDMiF@nand.local>
- In-Reply-To
- <6e7a272c29577536ba992cc73d736d8f66397607.1727687410.git.ps@pks.im>
On Mon, Sep 30, 2024 at 11:13:51AM +0200, Patrick Steinhardt wrote:
Show 23 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 27 quoted lines
> 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