Re: [PATCH 2/2] remote: fix trivial memory leak

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 15, 2013, 21:50 UTC
Message-ID: <xmqqob6qcife.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20130924051908.GG2766@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> I wondered if we might also leak when seeing duplicate config options
> (i.e., leaking the old one when replacing it with the new). But we don't
> actually strdup() the configured remote names, but instead just point
> into the "struct branch", which owns the data.

In addition, we do not copy this string to remote->name in make_remote(), so even if we start allowing destruction of existing remote[], the resulting code will stay safe.

Show 21 quoted lines

> So I think an even better fix would be:
>
> -- >8 --
> Subject: remote: do not copy "origin" string literal
>
> Our default_remote_name starts at "origin", but may be
> overridden by the config file. In the former case, we
> allocate a new string, but in the latter case, we point to
> the remote name in an existing "struct branch".
>
> This gives the variable inconsistent free() semantics (we
> are sometimes responsible for freeing the string and
> sometimes pointing to somebody else's storage), and causes a
> small leak when the allocated string is overridden by
> config.
>
> We can fix both by simply dropping the extra copy and
> pointing to the string literal.
>
> Noticed-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>

Sounds sensible. Thanks.

Show 17 quoted lines

> ---
>  remote.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/remote.c b/remote.c
> index e9fedfa..9f1a8aa 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -483,7 +483,7 @@ static void read_config(void)
>  	int flag;
>  	if (default_remote_name) /* did this already */
>  		return;
> -	default_remote_name = xstrdup("origin");
> +	default_remote_name = "origin";
>  	current_branch = NULL;
>  	head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag);
>  	if (head_ref && (flag & REF_ISSYMREF) &&