From: Jeff King <peff@peff.net>
Date: Mon, 12 Aug 2024 14:37:10 GMT
Subject: Re: [PATCH v2 01/22] remote: plug memory leak when aliasing URLs
Message-ID: <20240812143710.GA3800522@coredump.intra.peff.net>
In-Reply-To: <2afa51f9ffedfa6dab51c9515f695ddfe0a9a4f9.1723121979.git.ps@pks.im>

On Thu, Aug 08, 2024 at 03:04:33PM +0200, Patrick Steinhardt wrote:

> When we have a `url.*.insteadOf` configuration, then we end up aliasing
> URLs when populating remotes. One place where this happens is in
> `alias_all_urls()`, where we loop through all remotes and then alias
> each of their URLs. The actual aliasing logic is then contained in
> `alias_url()`, which returns an allocated string that contains the new
> URL. This URL replaces the old URL that we have in the strvec that
> contanis all remote URLs.
> 
> We replace the remote URLs via `strvec_replace()`, which does not hand
> over ownership of the new string to the vector. Still, we didn't free
> the aliased URL and thus have a memory leak here. Fix it by freeing the
> aliased string.

Thanks, this one is my fault. When I replaced the open-coded replacement
in 8e804415fd (remote: use strvecs to store remote url/pushurl,
2024-06-14), for some reason I thought that strvec_replace() would take
ownership of the pointer. We could make a "_nodup()" variant, but it is
probably not worth the extra API complexity.

Curiously, these are the only calls for strvec_replace(). You added it
in 11ce77b5cc (strvec: add functions to replace and remove strings,
2024-05-27) but I don't see them used in any iteration of that patch
series. So yet another option is to change the semantics of
strvec_replace(), but I think that is an even worse idea. ;)

-Peff