From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 13 Nov 2011 20:41:41 GMT
Subject: Re: [PATCH 2/2] Copy resolve_ref() return value for longer use
Message-ID: <7vbosfoiuy.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1321179735-21890-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> resolve_ref() may return a pointer to a static buffer. Callers that
> use this value longer than a couple of statements should copy the
> value to avoid some hidden resolve_ref() call that may change the
> static buffer's value.
>
> The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
> demonstrates this. The first call is in cmd_merge()
>
> branch = resolve_ref("HEAD", head_sha1, 0, &flag);
>
> Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
> may be called again and destroy "branch".
>
> lookup_commit_or_die
>  lookup_commit_reference
>   lookup_commit_reference_gently
>    parse_object
>     lookup_replace_object
>      do_lookup_replace_object
>       prepare_replace_object
>        for_each_replace_ref
>         do_for_each_ref
>          get_loose_refs
>           get_ref_dir
>            get_ref_dir
>             resolve_ref
>
> All call sites are checked and made sure that xstrdup() is called if
> the value should be saved.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Unfortunately there are still 37 resolve_ref() calls. An API change
>  would touch all of them and potentially introduce more bugs along the
>  way, either leak more memory or free() the wrong way.
>
>  So I'd stick with this for v1.7.8.
>
>  builtin/branch.c        |    5 +++-
>  builtin/checkout.c      |    4 ++-
>  builtin/commit.c        |    3 +-
>  builtin/fmt-merge-msg.c |    6 ++++-
>  builtin/merge.c         |   62 ++++++++++++++++++++++++++++++----------------
>  builtin/notes.c         |    6 ++++-
>  builtin/receive-pack.c  |    3 ++
>  reflog-walk.c           |    5 +++-
>  8 files changed, 66 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0fe9c4d..5b6d839 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -115,8 +115,10 @@ static int branch_merged(int kind, const char *name,
>  		    branch->merge[0] &&
>  		    branch->merge[0]->dst &&
>  		    (reference_name =
> -		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
> +		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {
> +			reference_name = xstrdup(reference_name);
>  			reference_rev = lookup_commit_reference(sha1);
> +		}
>  	}
>  	if (!reference_rev)
>  		reference_rev = head_rev;
> @@ -141,6 +143,7 @@ static int branch_merged(int kind, const char *name,
>  				"         '%s', even though it is merged to HEAD."),
>  				name, reference_name);
>  	}
> +	free((char*)reference_name);
>  	return merged;
>  }

Now reference_name stores the result of xstrdup(), it does not have reason
to be of type "const char *". It is preferable to lose the cast here, I
think. The same comment applies to the remainder of the patch.

Otherwise the patch looks good. Thanks.