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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Aug 12, 2024 at 10:37:10AM -0400, Jeff King wrote:
> 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. ;)

Oh, interesting. I certainly wanted to use it back then, but guess that
later iterations removed those callsites again. In any case, it is being
used now, so at least it isn't dead code :)

Patrick