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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux