From d5a35c114ab6b4337a1c7598bf75c331d94ee092 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Sun, 13 Nov 2011 17:22:15 +0700 Subject: [PATCH] Copy resolve_ref() return value for longer use MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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 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 Signed-off-by: Junio C Hamano --- 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 0fe9c4dfe..3ef15f7fc 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; } diff --git a/builtin/checkout.c b/builtin/checkout.c index beeaee411..c6919f168 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -699,7 +699,9 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new) unsigned char rev[20]; int flag; memset(&old, 0, sizeof(old)); - old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag)); + old.path = resolve_ref("HEAD", rev, 0, &flag); + if (old.path) + old.path = xstrdup(old.path); old.commit = lookup_commit_reference_gently(rev, 1); if (!(flag & REF_ISSYMREF)) { free((char *)old.path); diff --git a/builtin/commit.c b/builtin/commit.c index c46f2d18e..f3a6ed2bf 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1, struct commit *commit; struct strbuf format = STRBUF_INIT; unsigned char junk_sha1[20]; - const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL); + const char *head; struct pretty_print_context pctx = {0}; struct strbuf author_ident = STRBUF_INIT; struct strbuf committer_ident = STRBUF_INIT; @@ -1304,6 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1, rev.diffopt.break_opt = 0; diff_setup_done(&rev.diffopt); + head = resolve_ref("HEAD", junk_sha1, 0, NULL); printf("[%s%s ", !prefixcmp(head, "refs/heads/") ? head + 11 : diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 7e2f22589..a3ba21520 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -268,6 +268,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in, die("No current branch"); if (!prefixcmp(current_branch, "refs/heads/")) current_branch += 11; + current_branch = xstrdup(current_branch); /* get a line */ while (pos < in->len) { @@ -283,8 +284,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in, die ("Error in line %d: %.*s", i, len, p); } - if (!srcs.nr) + if (!srcs.nr) { + free((char*)current_branch); return 0; + } if (merge_title) do_fmt_merge_msg_title(out, current_branch); @@ -306,6 +309,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in, shortlog(origins.items[i].string, origins.items[i].util, head, &rev, shortlog_len, out); } + free((char *)current_branch); return 0; } diff --git a/builtin/merge.c b/builtin/merge.c index 42b4f9e7b..9cda40003 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1082,7 +1082,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) struct commit *head_commit; struct strbuf buf = STRBUF_INIT; const char *head_arg; - int flag, i; + int flag, i, ret = 0; int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0; struct commit_list *common = NULL; const char *best_strategy = NULL, *wt_strategy = NULL; @@ -1096,8 +1096,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * current branch. */ branch = resolve_ref("HEAD", head_sha1, 0, &flag); - if (branch && !prefixcmp(branch, "refs/heads/")) - branch += 11; + if (branch) { + if (!prefixcmp(branch, "refs/heads/")) + branch += 11; + branch = xstrdup(branch); + } if (!branch || is_null_sha1(head_sha1)) head_commit = NULL; else @@ -1121,7 +1124,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) die(_("There is no merge to abort (MERGE_HEAD missing).")); /* Invoke 'git reset --merge' */ - return cmd_reset(nargc, nargv, prefix); + ret = cmd_reset(nargc, nargv, prefix); + goto done; } if (read_cache_unmerged()) @@ -1205,7 +1209,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) read_empty(remote_head->sha1, 0); update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0, DIE_ON_ERR); - return 0; + goto done; } else { struct strbuf merge_names = STRBUF_INIT; @@ -1292,7 +1296,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * but first the most common case of merging one remote. */ finish_up_to_date("Already up-to-date."); - return 0; + goto done; } else if (allow_fast_forward && !remoteheads->next && !common->next && !hashcmp(common->item->object.sha1, head_commit->object.sha1)) { @@ -1313,15 +1317,20 @@ int cmd_merge(int argc, const char **argv, const char *prefix) strbuf_addstr(&msg, " (no commit created; -m option ignored)"); o = want_commit(sha1_to_hex(remoteheads->item->object.sha1)); - if (!o) - return 1; + if (!o) { + ret = 1; + goto done; + } - if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1)) - return 1; + if (checkout_fast_forward(head_commit->object.sha1, + remoteheads->item->object.sha1)) { + ret = 1; + goto done; + } finish(head_commit, o->sha1, msg.buf); drop_save(); - return 0; + goto done; } else if (!remoteheads->next && common->next) ; /* @@ -1339,8 +1348,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix) git_committer_info(IDENT_ERROR_ON_NO_NAME); printf(_("Trying really trivial in-index merge...\n")); if (!read_tree_trivial(common->item->object.sha1, - head_commit->object.sha1, remoteheads->item->object.sha1)) - return merge_trivial(head_commit); + head_commit->object.sha1, + remoteheads->item->object.sha1)) { + ret = merge_trivial(head_commit); + goto done; + } printf(_("Nope.\n")); } } else { @@ -1368,7 +1380,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) } if (up_to_date) { finish_up_to_date("Already up-to-date. Yeeah!"); - return 0; + goto done; } } @@ -1450,9 +1462,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * If we have a resulting tree, that means the strategy module * auto resolved the merge cleanly. */ - if (automerge_was_ok) - return finish_automerge(head_commit, common, result_tree, - wt_strategy); + if (automerge_was_ok) { + ret = finish_automerge(head_commit, common, result_tree, + wt_strategy); + goto done; + } /* * Pick the result from the best strategy and have the user fix @@ -1466,7 +1480,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) else fprintf(stderr, _("Merge with strategy %s failed.\n"), use_strategies[0]->name); - return 2; + ret = 2; + goto done; } else if (best_strategy == wt_strategy) ; /* We already have its result in the working tree. */ else { @@ -1482,10 +1497,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix) else write_merge_state(); - if (merge_was_ok) { + if (merge_was_ok) fprintf(stderr, _("Automatic merge went well; " "stopped before committing as requested\n")); - return 0; - } else - return suggest_conflicts(option_renormalize); + else + ret = suggest_conflicts(option_renormalize); + +done: + free((char *)branch); + return ret; } diff --git a/builtin/notes.c b/builtin/notes.c index f8e437db0..10b8bc7ad 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -804,6 +804,7 @@ static int merge_commit(struct notes_merge_options *o) struct notes_tree *t; struct commit *partial; struct pretty_print_context pretty_ctx; + int ret; /* * Read partial merge result from .git/NOTES_MERGE_PARTIAL, @@ -828,6 +829,7 @@ static int merge_commit(struct notes_merge_options *o) o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL); if (!o->local_ref) die("Failed to resolve NOTES_MERGE_REF"); + o->local_ref = xstrdup(o->local_ref); if (notes_merge_commit(o, t, partial, sha1)) die("Failed to finalize notes merge"); @@ -843,7 +845,9 @@ static int merge_commit(struct notes_merge_options *o) free_notes(t); strbuf_release(&msg); - return merge_abort(o); + ret = merge_abort(o); + free((char *)o->local_ref); + return ret; } static int merge(int argc, const char **argv, const char *prefix) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 7ec68a1e8..b6d957cb0 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -695,7 +695,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro check_aliased_updates(commands); + free((char *)head_name); head_name = resolve_ref("HEAD", sha1, 0, NULL); + if (head_name) + head_name = xstrdup(head_name); for (cmd = commands; cmd; cmd = cmd->next) if (!cmd->skip_update) diff --git a/reflog-walk.c b/reflog-walk.c index 5d81d39a5..da71a8585 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -51,8 +51,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref) if (reflogs->nr == 0) { unsigned char sha1[20]; const char *name = resolve_ref(ref, sha1, 1, NULL); - if (name) + if (name) { + name = xstrdup(name); for_each_reflog_ent(name, read_one_reflog, reflogs); + free((char *)name); + } } if (reflogs->nr == 0) { int len = strlen(ref); -- 2.30.2