Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> 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@xxxxxxxxx> 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@xxxxxxxxx>
> ---
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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html