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 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@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
>
> 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@xxxxxxxxx>
> ---
> 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.
--
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