Re: [PATCH 15/23] pseudo-merge: fix leaking strmap keys

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 30, 2024, 21:22 UTC
Message-ID: <ZvsWlNPGxt5AOQcK@nand.local>
In-Reply-To: <9e1161d55f96253bfeb5cddd1bbd381e2dad8a71.1727687410.git.ps@pks.im>

On Mon, Sep 30, 2024 at 11:13:53AM +0200, Patrick Steinhardt wrote:

> When creating a new pseudo-merge group we collect a set of matchnig

s/matchnig/matching/

Show 8 quoted lines

> commits and put them into a string map. This strmap is initialized such
> that it does not allocate its keys, and instead we try to pass ownership
> of the keys to it via `strmap_put()`. This isn't how it works though:
> the strmap will never try to release these keys, and consequently they
> end up leaking.
>
> Fix this leak by initializing the strmap as duplicating its keys and not
> trying to hand over ownership.

Hmm. I think I am probably splitting hairs, since a few xstrdup() calls between friends is unlikely to matter here, but... ;-)

It does seem wasteful if we can avoid it. We already allocated heap space for the matches via the underlying strbuf, and we really do want to hand ownership over if we can.

Would doing something like this on top of your previous patch do the trick?

--- >8 ---

diff --git a/pseudo-merge.c b/pseudo-merge.c
index 28782a31c6..6b6605d3dc 100644
--- a/pseudo-merge.c
+++ b/pseudo-merge.c
@@ -110,6 +110,7 @@ void pseudo_merge_group_release(struct pseudo_merge_group *group)
 		free(matches->stable);
 		free(matches->unstable);
 		free(matches);
+		free((char*)e->key);
 	}
 	strmap_clear(&group->matches, 0);
--- 8< ---

That introduces an ugly const-cast, but I think it's tolerable (and may
be moreso with a comment explaining why it's safe).

> diff --git a/pseudo-merge.c b/pseudo-merge.c
> index 28782a31c6..bb59965ed2 100644
> --- a/pseudo-merge.c
> +++ b/pseudo-merge.c
> @@ -87,7 +87,7 @@ static void pseudo_merge_group_init(struct pseudo_merge_group *group)
>  {
>  	memset(group, 0, sizeof(struct pseudo_merge_group));
>
> -	strmap_init_with_options(&group->matches, NULL, 0);
> +	strmap_init_with_options(&group->matches, NULL, 1);
>
>  	group->decay = DEFAULT_PSEUDO_MERGE_DECAY;
>  	group->max_merges = DEFAULT_PSEUDO_MERGE_MAX_MERGES;
> @@ -275,7 +275,7 @@ static int find_pseudo_merge_group_for_ref(const char *refname,
>  		matches = strmap_get(&group->matches, group_name.buf);
>  		if (!matches) {
>  			matches = xcalloc(1, sizeof(*matches));
> -			strmap_put(&group->matches, strbuf_detach(&group_name, NULL),
> +			strmap_put(&group->matches, group_name.buf,
>  				   matches);
>  		}

Otherwise, I think what's written here looks good to me.

Thanks,
Taylor