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 for the detailed explanation here.
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> ---
> remote.c | 2 ++
> t/t0210-trace2-normal.sh | 2 +-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index f43cf5e7a4..3b898edd23 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -499,6 +499,7 @@ static void alias_all_urls(struct remote_state *remote_state)
> if (alias)
> strvec_replace(&remote_state->remotes[i]->pushurl,
> j, alias);
> + free(alias);
> }
> add_pushurl_aliases = remote_state->remotes[i]->pushurl.nr == 0;
> for (j = 0; j < remote_state->remotes[i]->url.nr; j++) {
> @@ -512,6 +513,7 @@ static void alias_all_urls(struct remote_state *remote_state)
> if (alias)
> strvec_replace(&remote_state->remotes[i]->url,
> j, alias);
> + free(alias);
> }
> }
> }
These both make sense to me, since alias_url() allocates the string it
returns via xstrfmt(), so having the caller free it makes sense.
I was wondering if there was a nice way to neaten up these two call
paths that both call alias_url(), check for NULL, call strbuf_replace(),
and then free the result. But I think the result here would be pretty
awkward from my attempts at it, so I think this patch looks good as-is.
Thanks,
Taylor
