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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Sep 30, 2024 at 05:22:28PM -0400, Taylor Blau wrote:
> 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/
> 
> > 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).

Well, to me this is a tradeoff between complexity and performance. If
the performance benefit outweighs the additional complexity that this
shared ownership of keys brings along with it then I'm okay with the
code being intimate with each others lifetime requirements.

But as far as I can see the number of entries is determined by the group
pattern, and I expect that in most cases it's going to be quite limited.
So it does not feel like this should lead to all that many extra
allocations, and thus I tend to prefer the simpler solution.

But please let me know in case you disagree with that assessment.

Patrick