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

[PATCH 5/8] Use resolve_ref() instead of resolve_ref_unsafe()

From
Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Date
Nov 17, 2011, 09:32 UTC
Message-ID
<1321522335-24193-6-git-send-email-pclouds@gmail.com>
In-Reply-To
<1321522335-24193-1-git-send-email-pclouds@gmail.com>

resolve_ref_unsaf() 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_unsafe("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
           read_ref_full
	    resolve_ref_unsafe

Convert all resolve_ref_unsafe() call sites, where the return value may be held across many function calls, to resolve_ref() and free buffer after use.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c        |    5 +++--
 builtin/commit.c        |    3 ++-
 builtin/fmt-merge-msg.c |    8 ++++++--
 builtin/merge.c         |    4 +++-
 builtin/notes.c         |    8 ++++++--
 builtin/receive-pack.c  |    5 +++--
 reflog-walk.c           |    6 ++++--
 7 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 6903b0d..633b56e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -103,7 +103,7 @@ static int branch_merged(int kind, const char *name,
 	 * safely to HEAD (or the other branch).
 	 */
 	struct commit *reference_rev = NULL;
-	const char *reference_name = NULL;
+	char *reference_name = NULL;
 	int merged;
 
 	if (kind == REF_LOCAL_BRANCH) {
@@ -115,7 +115,7 @@ static int branch_merged(int kind, const char *name,
 		    branch->merge[0] &&
 		    branch->merge[0]->dst &&
 		    (reference_name =
-		     resolve_ref_unsafe(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
+		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
 			reference_rev = lookup_commit_reference(sha1);
 	}
 	if (!reference_rev)
@@ -141,6 +141,7 @@ static int branch_merged(int kind, const char *name,
 				"         '%s', even though it is merged to HEAD."),
 				name, reference_name);
 	}
+	free(reference_name);
 	return merged;
 }
 
diff --git a/builtin/commit.c b/builtin/commit.c
index 0be3b45..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_unsafe("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 5c9b40e..dd94c3d 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -261,9 +261,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 	int i = 0, pos = 0;
 	unsigned char head_sha1[20];
 	const char *current_branch;
+	char *ref;
 
 	/* get current branch */
-	current_branch = resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
+	current_branch = ref = resolve_ref("HEAD", head_sha1, 1, NULL);
 	if (!current_branch)
 		die("No current branch");
 	if (!prefixcmp(current_branch, "refs/heads/"))
@@ -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(ref);
 		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(ref);
 	return 0;
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 0d597b3..8be0594 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1087,6 +1087,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
+	char *branch_ref;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_merge_usage, builtin_merge_options);
@@ -1095,7 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 	 * current branch.
 	 */
-	branch = resolve_ref_unsafe("HEAD", head_sha1, 0, &flag);
+	branch = branch_ref = resolve_ref("HEAD", head_sha1, 0, &flag);
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
@@ -1497,5 +1498,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = suggest_conflicts(option_renormalize);
 
 done:
+	free(branch_ref);
 	return ret;
 }
diff --git a/builtin/notes.c b/builtin/notes.c
index e191ce6..725a701 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -804,6 +804,8 @@ static int merge_commit(struct notes_merge_options *o)
 	struct notes_tree *t;
 	struct commit *partial;
 	struct pretty_print_context pretty_ctx;
+	char *ref;
+	int ret;
 
 	/*
 	 * Read partial merge result from .git/NOTES_MERGE_PARTIAL,
@@ -825,7 +827,7 @@ static int merge_commit(struct notes_merge_options *o)
 	t = xcalloc(1, sizeof(struct notes_tree));
 	init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
 
-	o->local_ref = resolve_ref_unsafe("NOTES_MERGE_REF", sha1, 0, NULL);
+	o->local_ref = ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL);
 	if (!o->local_ref)
 		die("Failed to resolve NOTES_MERGE_REF");
 
@@ -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(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 333f2b0..d41884a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -36,7 +36,7 @@ static int use_sideband;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
-static const char *head_name;
+static char *head_name;
 static int sent_capabilities;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
@@ -695,7 +695,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 
 	check_aliased_updates(commands);
 
-	head_name = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
+	free(head_name);
+	head_name = resolve_ref("HEAD", sha1, 0, NULL);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
 		if (!cmd->skip_update)
diff --git a/reflog-walk.c b/reflog-walk.c
index b9b2453..2d5aee0 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,9 +50,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 	for_each_reflog_ent(ref, read_one_reflog, reflogs);
 	if (reflogs->nr == 0) {
 		unsigned char sha1[20];
-		const char *name = resolve_ref_unsafe(ref, sha1, 1, NULL);
-		if (name)
+		char *name = resolve_ref(ref, sha1, 1, NULL);
+		if (name) {
 			for_each_reflog_ent(name, read_one_reflog, reflogs);
+			free(name);
+		}
 	}
 	if (reflogs->nr == 0) {
 		int len = strlen(ref);

-- 
1.7.4.74.g639db
Previous: Jonathan NiederNext: Nguyễn Thái Ngọc Duy
Message 8 of 27 in “nd/resolve-ref v2”
  1. 0/8 nd/resolve-ref v2Nguyễn Thái Ngọc Duy, Nov 17, 2011
  2. 1/8 Convert many resolve_ref() calls to read_ref*() and ref_exists()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  3. 2/8 Rename resolve_ref() to resolve_ref_unsafe()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  4. 3/8 Re-add resolve_ref() that always returns an allocated bufferNguyễn Thái Ngọc Duy, Nov 17, 2011
  5. 4/8 cmd_merge: convert to single exit pointNguyễn Thái Ngọc Duy, Nov 17, 2011
  6. Ramkumar RamachandraNov 17, 2011
  7. Jonathan NiederNov 17, 2011
  8. 5/8 Use resolve_ref() instead of resolve_ref_unsafe()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  9. 6/8 Convert resolve_ref_unsafe+xstrdup to resolve_refNguyễn Thái Ngọc Duy, Nov 17, 2011
  10. Ramkumar RamachandraNov 17, 2011
  11. Nguyen Thai Ngoc DuyNov 18, 2011
  12. 7/8 Guard memory overwriting in resolve_ref_unsafe's static bufferNguyễn Thái Ngọc Duy, Nov 17, 2011
  13. 8/8 Enable GIT_DEBUG_MEMCHECK on git_pathname()Nguyễn Thái Ngọc Duy, Nov 17, 2011
  14. Ramkumar RamachandraNov 17, 2011
  15. Jeff KingNov 17, 2011
  16. Nguyen Thai Ngoc DuyNov 18, 2011
  17. Jeff KingNov 18, 2011
  18. Jonathan NiederNov 18, 2011
  19. Nguyen Thai Ngoc DuyNov 18, 2011
  20. Jeff KingNov 18, 2011
  21. Jonathan NiederNov 18, 2011
  22. Johan HerlandNov 18, 2011
  23. Jonathan NiederNov 18, 2011
  24. Junio C HamanoNov 18, 2011
  25. Bernhard R. LinkNov 18, 2011
  26. Jonathan NiederNov 17, 2011
  27. Nguyen Thai Ngoc DuyDec 6, 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.