git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/2] Copy resolve_ref() return value for longer use

From
Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date
Nov 13, 2011, 10:22 UTC
Message-ID
<1321179735-21890-2-git-send-email-pclouds@gmail.com>
In-Reply-To
<1321179735-21890-1-git-send-email-pclouds@gmail.com>

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;
 }
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index beeaee4..c6919f1 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 c46f2d1..f3a6ed2 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 7e2f225..6fe9102 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 42b4f9e..33be6c8 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 f8e437d..ccff770 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 7ec68a1..d3fe42a 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 5d81d39..efd13ad 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);

-- 
1.7.4.74.g639db
Previous: Nguyễn Thái Ngọc DuyNext: Junio C Hamano
Message 11 of 31 in “Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7”
  1. Nguyen Thai Ngoc DuyNov 7, 2011
  2. Tony WangNov 7, 2011
  3. Nguyen Thai Ngoc DuyNov 7, 2011
  4. Tony WangNov 7, 2011
  5. Nguyen Thai Ngoc DuyNov 7, 2011
  6. Copy resolve_ref() return value for longer useNguyễn Thái Ngọc Duy, Nov 8, 2011
  7. Junio C HamanoNov 13, 2011
  8. Nguyen Thai Ngoc DuyNov 13, 2011
  9. Junio C HamanoNov 13, 2011
  10. 1/2 Convert many resolve_ref() calls to read_ref*() and ref_exists()Nguyễn Thái Ngọc Duy, Nov 13, 2011
  11. 2/2 Copy resolve_ref() return value for longer useNguyễn Thái Ngọc Duy, Nov 13, 2011
  12. Junio C HamanoNov 13, 2011
  13. Nguyen Thai Ngoc DuyNov 14, 2011
  14. Junio C HamanoNov 14, 2011
  15. Jeff KingNov 14, 2011
  16. Nguyen Thai Ngoc DuyNov 15, 2011
  17. 01/10 Allow resolve_ref() caller to decide whether to receive static bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  18. 02/10 cmd_merge: convert to single exit pointNguyễn Thái Ngọc Duy, Nov 15, 2011
  19. 03/10 merge: do not point "branch" to a resolve_ref()'s static bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  20. 04/10 commit: move resolve_ref() closer to where the return value is usedNguyễn Thái Ngọc Duy, Nov 15, 2011
  21. 05/10 checkout: do not try xstrdup() on NULLNguyễn Thái Ngọc Duy, Nov 15, 2011
  22. 06/10 reflog-walk.c: request allocated buffer from resolve_ref()Nguyễn Thái Ngọc Duy, Nov 15, 2011
  23. 07/10 receive-pack: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  24. 08/10 notes: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  25. 09/10 fmt-merge-msg: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  26. 10/10 branch: request resolve_ref() to allocate new bufferNguyễn Thái Ngọc Duy, Nov 15, 2011
  27. Junio C HamanoNov 15, 2011
  28. Junio C HamanoNov 13, 2011
  29. Tony WangDec 10, 2011
  30. Nguyen Thai Ngoc DuyDec 10, 2011
  31. Tony WangDec 11, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.