Jeff King <p...@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.

> 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.contre...@gmail.com>
> Signed-off-by: Jeff King <p...@peff.net>

Sounds sensible. Thanks.

> ---
>  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) &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to