Re: [PATCH v2 01/22] remote: plug memory leak when aliasing URLs

From: Jeff King <peff@peff.net>
Date: Aug 12, 2024, 14:37 UTC
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:

Show 12 quoted lines

> 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