Re: [PATCH] Copy resolve_ref() return value for longer use

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2011, 05:57 UTC
Message-ID: <7vehxcpns5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1320719428-1802-1-git-send-email-pclouds@gmail.com>

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

Show 28 quoted lines

> resolve_ref() may return a pointer to a static buffer. Callers that
> use this value outside of a block 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
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Thanks.

The above log message with the callchain would be a perfect explanation for a patch to fix builtin/merge.c and nothing else, but among 50+ callsites of resolve_ref(), only 5 places are modified in this patch, and it is not explained in the commit log message at all how they were chosen. Were the other 40+ or so places inspected and deemed to be OK? Or is this "I just did a few of them in addition to the immediate need of fixing what was reported, and the rest are left as an exercise to the readers" patch?

Some variables that receive xstrdup()'ed result with this patch are globals whose lifetime lasts while the process is alive, and I do not think it is a huge problem that this patch does not free it, but it seems the patch does not free some other function scope variables once their use is done even when it is fairly easy to do so.

How much overhead would it incur if we return xstrdup()'ed memory from the resolve_ref() and make it the caller's responsibility to free it? Would it make the calling site too ugly?