[PATCH 1/3] Convert resolve_ref+xstrdup to new resolve_refdup function

[PATCH 1/3] Convert resolve_ref+xstrdup to new resolve_refdup function

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c        |   11 ++++-------
 builtin/checkout.c      |   15 +++++++--------
 builtin/fmt-merge-msg.c |    6 +++---
 builtin/for-each-ref.c  |    7 ++-----
 builtin/merge.c         |   12 +++++-------
 builtin/notes.c         |    6 +++---
 builtin/receive-pack.c  |    8 +++-----
 builtin/revert.c        |    2 +-
 builtin/show-branch.c   |    4 +---
 cache.h                 |    1 +
 reflog-walk.c           |   13 ++++++-------
 refs.c                  |    6 ++++++
 wt-status.c             |    4 +---
 13 files changed, 43 insertions(+), 52 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index e1e486e..72c4c31 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,10 +115,8 @@ 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) {
-			reference_name = xstrdup(reference_name);
+		     resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
 			reference_rev = lookup_commit_reference(sha1);
-		}
 	}
 	if (!reference_rev)
 		reference_rev = head_rev;
@@ -143,7 +141,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);
+	free(reference_name);
 	return merged;
 }
 
@@ -731,10 +729,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	track = git_branch_track;
 
-	head = resolve_ref("HEAD", head_sha1, 0, NULL);
+	head = resolve_refdup("HEAD", head_sha1, 0, NULL);
 	if (!head)
 		die(_("Failed to resolve HEAD as a valid ref."));
-	head = xstrdup(head);
 	if (!strcmp(head, "HEAD")) {
 		detached = 1;
 	} else {
diff --git a/builtin/checkout.c b/builtin/checkout.c
index b7c6302..a66d3eb 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 {
 	int ret = 0;
 	struct branch_info old;
+	char *path;
 	unsigned char rev[20];
 	int flag;
 	memset(&old, 0, sizeof(old));
-	old.path = resolve_ref("HEAD", rev, 0, &flag);
-	if (old.path)
-		old.path = xstrdup(old.path);
+	old.path = path = resolve_refdup("HEAD", rev, 0, &flag);
 	old.commit = lookup_commit_reference_gently(rev, 1);
-	if (!(flag & REF_ISSYMREF)) {
-		free((char *)old.path);
+	if (!(flag & REF_ISSYMREF))
 		old.path = NULL;
-	}
 
 	if (old.path && !prefixcmp(old.path, "refs/heads/"))
 		old.name = old.path + strlen("refs/heads/");
@@ -720,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	}
 
 	ret = merge_working_tree(opts, &old, new);
-	if (ret)
+	if (ret) {
+		free(path);
 		return ret;
+	}
 
 	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
 		orphaned_commit_warning(old.commit);
@@ -729,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	update_refs_for_switch(opts, &old, new);
 
 	ret = post_checkout_hook(old.commit, new->commit, 1);
-	free((char *)old.path);
+	free(path);
 	return ret || opts->writeout_error;
 }
 
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index bdfa0ea..a27efcd 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -372,14 +372,14 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 	int i = 0, pos = 0;
 	unsigned char head_sha1[20];
 	const char *current_branch;
+	char *ref;
 
 	/* get current branch */
-	current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
+	current_branch = ref = resolve_refdup("HEAD", head_sha1, 1, NULL);
 	if (!current_branch)
 		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) {
@@ -421,7 +421,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,
 	}
 
 	strbuf_complete_line(out);
-	free((char *)current_branch);
+	free(ref);
 	return 0;
 }
 
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index d90e5d2..b01d76a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)
 
 	if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
 		unsigned char unused1[20];
-		const char *symref;
-		symref = resolve_ref(ref->refname, unused1, 1, NULL);
-		if (symref)
-			ref->symref = xstrdup(symref);
-		else
+		ref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);
+		if (!ref->symref)
 			ref->symref = "";
 	}
 
diff --git a/builtin/merge.c b/builtin/merge.c
index a1c8534..6437db2 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1096,6 +1096,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);
@@ -1104,12 +1105,9 @@ 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("HEAD", head_sha1, 0, &flag);
-	if (branch) {
-		if (!prefixcmp(branch, "refs/heads/"))
-			branch += 11;
-		branch = xstrdup(branch);
-	}
+	branch = branch_ref = resolve_refdup("HEAD", head_sha1, 0, &flag);
+	if (branch && !prefixcmp(branch, "refs/heads/"))
+		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
 		head_commit = NULL;
 	else
@@ -1520,6 +1518,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = suggest_conflicts(option_renormalize);
 
 done:
-	free((char *)branch);
+	free(branch_ref);
 	return ret;
 }
diff --git a/builtin/notes.c b/builtin/notes.c
index 10b8bc7..816c998 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;
+	char *ref;
 	int ret;
 
 	/*
@@ -826,10 +827,9 @@ 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("NOTES_MERGE_REF", sha1, 0, NULL);
+	o->local_ref = ref = resolve_refdup("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");
@@ -846,7 +846,7 @@ static int merge_commit(struct notes_merge_options *o)
 	free_notes(t);
 	strbuf_release(&msg);
 	ret = merge_abort(o);
-	free((char *)o->local_ref);
+	free(ref);
 	return ret;
 }
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b6d957c..5cd6fc7 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,10 +695,8 @@ 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);
+	free(head_name);
+	head_name = resolve_refdup("HEAD", sha1, 0, NULL);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
 		if (!cmd->skip_update)
diff --git a/builtin/revert.c b/builtin/revert.c
index 1ea525c..0c52a83 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -901,7 +901,7 @@ static int rollback_single_pick(void)
 	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
 	    !file_exists(git_path("REVERT_HEAD")))
 		return error(_("no cherry-pick or revert in progress"));
-	if (!resolve_ref("HEAD", head_sha1, 0, NULL))
+	if (read_ref_full("HEAD", head_sha1, 0, NULL))
 		return error(_("cannot resolve HEAD"));
 	if (is_null_sha1(head_sha1))
 		return error(_("cannot abort from a branch yet to be born"));
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 4b480d7..a1f148e 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -726,10 +726,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 
 		if (ac == 0) {
 			static const char *fake_av[2];
-			const char *refname;
 
-			refname = resolve_ref("HEAD", sha1, 1, NULL);
-			fake_av[0] = xstrdup(refname);
+			fake_av[0] = resolve_refdup("HEAD", sha1, 1, NULL);
 			fake_av[1] = NULL;
 			av = fake_av;
 			ac = 1;
diff --git a/cache.h b/cache.h
index 8c98d05..4887a3e 100644
--- a/cache.h
+++ b/cache.h
@@ -866,6 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);
  * errno is sometimes set on errors, but not always.
  */
 extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
+extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
 
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/reflog-walk.c b/reflog-walk.c
index da71a85..5572b42 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,11 +50,10 @@ 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(ref, sha1, 1, NULL);
+		char *name = resolve_refdup(ref, sha1, 1, NULL);
 		if (name) {
-			name = xstrdup(name);
 			for_each_reflog_ent(name, read_one_reflog, reflogs);
-			free((char *)name);
+			free(name);
 		}
 	}
 	if (reflogs->nr == 0) {
@@ -171,11 +170,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 	else {
 		if (*branch == '\0') {
 			unsigned char sha1[20];
-			const char *head = resolve_ref("HEAD", sha1, 0, NULL);
-			if (!head)
-				die ("No current branch");
 			free(branch);
-			branch = xstrdup(head);
+			branch = resolve_refdup("HEAD", sha1, 0, NULL);
+			if (!branch)
+				die ("No current branch");
+
 		}
 		reflogs = read_complete_reflog(branch);
 		if (!reflogs || reflogs->nr == 0) {
diff --git a/refs.c b/refs.c
index f5cb297..8ffb32f 100644
--- a/refs.c
+++ b/refs.c
@@ -605,6 +605,12 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 	return ref;
 }
 
+char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)
+{
+	const char *ret = resolve_ref(ref, sha1, reading, flag);
+	return ret ? xstrdup(ret) : NULL;
+}
+
 /* The argument to filter_refs */
 struct ref_filter {
 	const char *pattern;
diff --git a/wt-status.c b/wt-status.c
index 70fdb76..9ffc535 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -111,7 +111,6 @@ void status_printf_more(struct wt_status *s, const char *color,
 void wt_status_prepare(struct wt_status *s)
 {
 	unsigned char sha1[20];
-	const char *head;
 
 	memset(s, 0, sizeof(*s));
 	memcpy(s->color_palette, default_wt_status_colors,
@@ -119,8 +118,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
 	s->use_color = -1;
 	s->relative_paths = 1;
-	head = resolve_ref("HEAD", sha1, 0, NULL);
-	s->branch = head ? xstrdup(head) : NULL;
+	s->branch = resolve_refdup("HEAD", sha1, 0, NULL);
 	s->reference = "HEAD";
 	s->fp = stdout;
 	s->index_file = get_index_file();
-- 
1.7.8.36.g69ee2

[PATCH 2/3] Guard memory overwriting in resolve_ref's static buffer

[Nguyễn Thái Ngọc Duy]

There is a potential problem with resolve_ref() and some other
functions in git. The return value returned by resolve_ref() may
be changed when the function is called again. Callers must make sure the
next call won't happen as long as the value is still being used.

It's usually hard to track down this kind of problem.  Michael Haggerty
has an idea [1] that, instead of passing the same static buffer to
caller every time the function is called, we free the old buffer and
allocate the new one. This way access to the old (now invalid) buffer
may be caught.

This patch applies the same principle for resolve_ref() with a
few modifications:

 - This behavior is enabled when GIT_DEBUG_MEMCHECK is set. The ability
   is always available. We may be able to ask users to rerun with this
   flag on in suspicious cases.

 - Rely on mmap/mprotect to catch illegal access. We need valgrind or
   some other memory tracking tool to reliably catch this in Michael's
   approach.

 - Because mprotect is used instead of munmap, we definitely leak
   memory. Hopefully callers will not put resolve_ref() in a
   loop that runs 1 million times.

 - Save caller location in the allocated buffer so we know who made this
   call in the core dump.

Also introduce a new target, "make memcheck", that runs tests with this
flag on.

[1] http://comments.gmane.org/gmane.comp.version-control.git/182209

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 .gitignore          |    1 +
 Makefile            |    4 ++++
 cache.h             |    3 ++-
 git-compat-util.h   |    9 +++++++++
 refs.c              |   13 +++++++++++--
 t/t0071-memcheck.sh |   12 ++++++++++++
 test-resolve-ref.c  |   12 ++++++++++++
 wrapper.c           |   22 ++++++++++++++++++++++
 8 files changed, 73 insertions(+), 3 deletions(-)
 create mode 100755 t/t0071-memcheck.sh
 create mode 100644 test-resolve-ref.c

diff --git a/.gitignore b/.gitignore
index 8572c8c..470e452 100644
--- a/.gitignore
+++ b/.gitignore
@@ -179,6 +179,7 @@
 /test-obj-pool
 /test-parse-options
 /test-path-utils
+/test-resolve-ref
 /test-run-command
 /test-sha1
 /test-sigchain
diff --git a/Makefile b/Makefile
index ed82320..d71cf04 100644
--- a/Makefile
+++ b/Makefile
@@ -444,6 +444,7 @@ TEST_PROGRAMS_NEED_X += test-obj-pool
 TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
 TEST_PROGRAMS_NEED_X += test-run-command
+TEST_PROGRAMS_NEED_X += test-resolve-ref
 TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-string-pool
@@ -2241,6 +2242,9 @@ export NO_SVN_TESTS
 test: all
 	$(MAKE) -C t/ all
 
+memcheck: all
+	GIT_DEBUG_MEMCHECK=1 $(MAKE) -C t/ all
+
 test-ctype$X: ctype.o
 
 test-date$X: date.o ctype.o
diff --git a/cache.h b/cache.h
index 4887a3e..a63d890 100644
--- a/cache.h
+++ b/cache.h
@@ -865,7 +865,8 @@ extern int read_ref(const char *filename, unsigned char *sha1);
  *
  * errno is sometimes set on errors, but not always.
  */
-extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
+#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)
+extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);
 extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
 
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/git-compat-util.h b/git-compat-util.h
index 77062ed..9249fc2 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -433,6 +433,15 @@ extern char *xstrndup(const char *str, size_t len);
 extern void *xrealloc(void *ptr, size_t size);
 extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+
+/*
+ * These functions are used to allocate new memory. When the memory
+ * area is no longer used, ban all access to it so any illegal access
+ * can be caught. xfree_mmap() does not really free memory.
+ */
+extern void *xmalloc_mmap(size_t, const char *file, int line);
+extern void xfree_mmap(void *);
+
 extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern int xdup(int fd);
diff --git a/refs.c b/refs.c
index 8ffb32f..cf8dfcc 100644
--- a/refs.c
+++ b/refs.c
@@ -497,12 +497,21 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)
 	return -1;
 }
 
-const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)
+const char *resolve_ref_real(const char *ref, unsigned char *sha1,
+			     int reading, int *flag, const char *file, int line)
 {
 	int depth = MAXDEPTH;
 	ssize_t len;
 	char buffer[256];
-	static char ref_buffer[256];
+	static char real_ref_buffer[256];
+	static char *ref_buffer;
+
+	if (!ref_buffer && !getenv("GIT_DEBUG_MEMCHECK"))
+		ref_buffer = real_ref_buffer;
+	if (ref_buffer != real_ref_buffer) {
+		xfree_mmap(ref_buffer);
+		ref_buffer = xmalloc_mmap(256, file, line);
+	}
 
 	if (flag)
 		*flag = 0;
diff --git a/t/t0071-memcheck.sh b/t/t0071-memcheck.sh
new file mode 100755
index 0000000..b594732
--- /dev/null
+++ b/t/t0071-memcheck.sh
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+test_description='test that GIT_DEBUG_MEMCHECK works correctly'
+. ./test-lib.sh
+
+test_expect_success 'test-resolve-ref must crash' '
+	GIT_DEBUG_MEMCHECK=1 test-resolve-ref
+	exit_code=$? &&
+	test $exit_code -eq 139
+'
+
+test_done
diff --git a/test-resolve-ref.c b/test-resolve-ref.c
new file mode 100644
index 0000000..934d764
--- /dev/null
+++ b/test-resolve-ref.c
@@ -0,0 +1,12 @@
+#include "cache.h"
+
+int main(int argc, char **argv)
+{
+	unsigned char sha1[20];
+	const char *ref1, *ref2;
+	setup_git_directory();
+	ref1 = resolve_ref("HEAD", sha1, 0, NULL);
+	ref2 = resolve_ref("HEAD", sha1, 0, NULL);
+	printf("ref1 = %s\nref2 = %s\n", ref1, ref2);
+	return 0;
+}
diff --git a/wrapper.c b/wrapper.c
index 85f09df..407443a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -60,6 +60,28 @@ void *xmallocz(size_t size)
 	return ret;
 }
 
+void *xmalloc_mmap(size_t size, const char *file, int line)
+{
+	int *ret;
+	size += sizeof(int*) * 3;
+	ret = mmap(NULL, size, PROT_READ | PROT_WRITE,
+		   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+	if (ret == (int*)-1)
+		die_errno("unable to mmap %lu bytes anonymously",
+			  (unsigned long)size);
+
+	ret[0] = (int)file;
+	ret[1] = line;
+	ret[2] = size;
+	return ret + 3;
+}
+
+void xfree_mmap(void *p)
+{
+	if (p && mprotect(((int*)p) - 3, ((int*)p)[-1], PROT_NONE) == -1)
+		die_errno("unable to remove memory access");
+}
+
 /*
  * xmemdupz() allocates (len + 1) bytes of memory, duplicates "len" bytes of
  * "data" to the allocated memory, zero terminates the allocated memory,
-- 
1.7.8.36.g69ee2

[PATCH 3/3] Rename resolve_ref() to resolve_ref_unsafe()

[Nguyễn Thái Ngọc Duy]

resolve_ref() may return a pointer to a shared buffer and can be
overwritten by the next resolve_ref() calls. Callers need to
pay attention, not to keep the pointer when the next call happens.

Rename with "_unsafe" suffix to warn developers (or reviewers) before
introducing new call sites.

This patch is generated using this command

git grep -l 'resolve_ref(' -- '*.[ch]'|xargs sed -i 's/resolve_ref(/resolve_ref_unsafe(/g'

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c               |    2 +-
 builtin/branch.c       |    2 +-
 builtin/commit.c       |    2 +-
 builtin/fsck.c         |    2 +-
 builtin/log.c          |    4 ++--
 builtin/receive-pack.c |    2 +-
 builtin/remote.c       |    2 +-
 builtin/show-branch.c  |    2 +-
 builtin/symbolic-ref.c |    2 +-
 cache.h                |    2 +-
 refs.c                 |   20 ++++++++++----------
 remote.c               |    6 +++---
 test-resolve-ref.c     |    4 ++--
 transport.c            |    2 +-
 14 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/branch.c b/branch.c
index d91a099..772a4c2 100644
--- a/branch.c
+++ b/branch.c
@@ -182,7 +182,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
 		const char *head;
 		unsigned char sha1[20];
 
-		head = resolve_ref("HEAD", sha1, 0, NULL);
+		head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
 		if (!is_bare_repository() && head && !strcmp(head, ref->buf))
 			die("Cannot force update the current branch.");
 	}
diff --git a/builtin/branch.c b/builtin/branch.c
index 72c4c31..641bee1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -251,7 +251,7 @@ static char *resolve_symref(const char *src, const char *prefix)
 	int flag;
 	const char *dst, *cp;
 
-	dst = resolve_ref(src, sha1, 0, &flag);
+	dst = resolve_ref_unsafe(src, sha1, 0, &flag);
 	if (!(dst && (flag & REF_ISSYMREF)))
 		return NULL;
 	if (prefix && (cp = skip_prefix(dst, prefix)))
diff --git a/builtin/commit.c b/builtin/commit.c
index e36e9ad..4d39d25 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1304,7 +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);
+	head = resolve_ref_unsafe("HEAD", junk_sha1, 0, NULL);
 	printf("[%s%s ",
 		!prefixcmp(head, "refs/heads/") ?
 			head + 11 :
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 30d0dc8..8c479a7 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -563,7 +563,7 @@ static int fsck_head_link(void)
 	if (verbose)
 		fprintf(stderr, "Checking HEAD link\n");
 
-	head_points_at = resolve_ref("HEAD", head_sha1, 0, &flag);
+	head_points_at = resolve_ref_unsafe("HEAD", head_sha1, 0, &flag);
 	if (!head_points_at)
 		return error("Invalid HEAD");
 	if (!strcmp(head_points_at, "HEAD"))
diff --git a/builtin/log.c b/builtin/log.c
index 4395f3e..89d0cc0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1040,7 +1040,7 @@ static char *find_branch_name(struct rev_info *rev)
 	if (positive < 0)
 		return NULL;
 	strbuf_addf(&buf, "refs/heads/%s", rev->cmdline.rev[positive].name);
-	branch = resolve_ref(buf.buf, branch_sha1, 1, NULL);
+	branch = resolve_ref_unsafe(buf.buf, branch_sha1, 1, NULL);
 	if (!branch ||
 	    prefixcmp(branch, "refs/heads/") ||
 	    hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
@@ -1268,7 +1268,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 
 			rev.pending.objects[0].item->flags |= UNINTERESTING;
 			add_head_to_pending(&rev);
-			ref = resolve_ref("HEAD", sha1, 1, NULL);
+			ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
 			if (ref && !prefixcmp(ref, "refs/heads/"))
 				branch_name = xstrdup(ref + strlen("refs/heads/"));
 			else
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5cd6fc7..a1a4b09 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -571,7 +571,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 	int flag;
 
 	strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
-	dst_name = resolve_ref(buf.buf, sha1, 0, &flag);
+	dst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag);
 	strbuf_release(&buf);
 
 	if (!(flag & REF_ISSYMREF))
diff --git a/builtin/remote.c b/builtin/remote.c
index 407abfb..583eec9 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -573,7 +573,7 @@ static int read_remote_branches(const char *refname,
 	strbuf_addf(&buf, "refs/remotes/%s/", rename->old);
 	if (!prefixcmp(refname, buf.buf)) {
 		item = string_list_append(rename->remote_branches, xstrdup(refname));
-		symref = resolve_ref(refname, orig_sha1, 1, &flag);
+		symref = resolve_ref_unsafe(refname, orig_sha1, 1, &flag);
 		if (flag & REF_ISSYMREF)
 			item->util = xstrdup(symref);
 		else
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index a1f148e..a59e088 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -789,7 +789,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		}
 	}
 
-	head_p = resolve_ref("HEAD", head_sha1, 1, NULL);
+	head_p = resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
 	if (head_p) {
 		head_len = strlen(head_p);
 		memcpy(head, head_p, head_len + 1);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index dea849c..2ef5962 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -12,7 +12,7 @@ static void check_symref(const char *HEAD, int quiet)
 {
 	unsigned char sha1[20];
 	int flag;
-	const char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);
+	const char *refs_heads_master = resolve_ref_unsafe(HEAD, sha1, 0, &flag);
 
 	if (!refs_heads_master)
 		die("No such ref: %s", HEAD);
diff --git a/cache.h b/cache.h
index a63d890..3ce8bda 100644
--- a/cache.h
+++ b/cache.h
@@ -865,7 +865,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);
  *
  * errno is sometimes set on errors, but not always.
  */
-#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)
+#define resolve_ref_unsafe(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)
 extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);
 extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
 
diff --git a/refs.c b/refs.c
index cf8dfcc..010bb72 100644
--- a/refs.c
+++ b/refs.c
@@ -361,7 +361,7 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha
 	if (!(flags & REF_ISSYMREF))
 		return 0;
 
-	resolves_to = resolve_ref(refname, junk, 0, NULL);
+	resolves_to = resolve_ref_unsafe(refname, junk, 0, NULL);
 	if (!resolves_to || strcmp(resolves_to, d->refname))
 		return 0;
 
@@ -616,7 +616,7 @@ const char *resolve_ref_real(const char *ref, unsigned char *sha1,
 
 char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)
 {
-	const char *ret = resolve_ref(ref, sha1, reading, flag);
+	const char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);
 	return ret ? xstrdup(ret) : NULL;
 }
 
@@ -629,7 +629,7 @@ struct ref_filter {
 
 int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)
 {
-	if (resolve_ref(ref, sha1, reading, flags))
+	if (resolve_ref_unsafe(ref, sha1, reading, flags))
 		return 0;
 	return -1;
 }
@@ -1126,7 +1126,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 
 		this_result = refs_found ? sha1_from_ref : sha1;
 		mksnpath(fullref, sizeof(fullref), *p, len, str);
-		r = resolve_ref(fullref, this_result, 1, &flag);
+		r = resolve_ref_unsafe(fullref, this_result, 1, &flag);
 		if (r) {
 			if (!refs_found++)
 				*ref = xstrdup(r);
@@ -1156,7 +1156,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 		const char *ref, *it;
 
 		mksnpath(path, sizeof(path), *p, len, str);
-		ref = resolve_ref(path, hash, 1, NULL);
+		ref = resolve_ref_unsafe(path, hash, 1, NULL);
 		if (!ref)
 			continue;
 		if (!stat(git_path("logs/%s", path), &st) &&
@@ -1192,7 +1192,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
-	ref = resolve_ref(ref, lock->old_sha1, mustexist, &type);
+	ref = resolve_ref_unsafe(ref, lock->old_sha1, mustexist, &type);
 	if (!ref && errno == EISDIR) {
 		/* we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
@@ -1205,7 +1205,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 			error("there are still refs under '%s'", orig_ref);
 			goto error_return;
 		}
-		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);
+		ref = resolve_ref_unsafe(orig_ref, lock->old_sha1, mustexist, &type);
 	}
 	if (type_p)
 	    *type_p = type;
@@ -1368,7 +1368,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
-	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+	symref = resolve_ref_unsafe(oldref, orig_sha1, 1, &flag);
 	if (flag & REF_ISSYMREF)
 		return error("refname %s is a symbolic ref, renaming it is not supported",
 			oldref);
@@ -1657,7 +1657,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unsigned char head_sha1[20];
 		int head_flag;
 		const char *head_ref;
-		head_ref = resolve_ref("HEAD", head_sha1, 1, &head_flag);
+		head_ref = resolve_ref_unsafe("HEAD", head_sha1, 1, &head_flag);
 		if (head_ref && (head_flag & REF_ISSYMREF) &&
 		    !strcmp(head_ref, lock->ref_name))
 			log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
@@ -1994,7 +1994,7 @@ int update_ref(const char *action, const char *refname,
 int ref_exists(const char *refname)
 {
 	unsigned char sha1[20];
-	return !!resolve_ref(refname, sha1, 1, NULL);
+	return !!resolve_ref_unsafe(refname, sha1, 1, NULL);
 }
 
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
diff --git a/remote.c b/remote.c
index 6655bb0..73a3809 100644
--- a/remote.c
+++ b/remote.c
@@ -482,7 +482,7 @@ static void read_config(void)
 		return;
 	default_remote_name = xstrdup("origin");
 	current_branch = NULL;
-	head_ref = resolve_ref("HEAD", sha1, 0, &flag);
+	head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
 	    !prefixcmp(head_ref, "refs/heads/")) {
 		current_branch =
@@ -1007,7 +1007,7 @@ static char *guess_ref(const char *name, struct ref *peer)
 	struct strbuf buf = STRBUF_INIT;
 	unsigned char sha1[20];
 
-	const char *r = resolve_ref(peer->name, sha1, 1, NULL);
+	const char *r = resolve_ref_unsafe(peer->name, sha1, 1, NULL);
 	if (!r)
 		return NULL;
 
@@ -1058,7 +1058,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		unsigned char sha1[20];
 		int flag;
 
-		dst_value = resolve_ref(matched_src->name, sha1, 1, &flag);
+		dst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);
 		if (!dst_value ||
 		    ((flag & REF_ISSYMREF) &&
 		     prefixcmp(dst_value, "refs/heads/")))
diff --git a/test-resolve-ref.c b/test-resolve-ref.c
index 934d764..1847cb4 100644
--- a/test-resolve-ref.c
+++ b/test-resolve-ref.c
@@ -5,8 +5,8 @@ int main(int argc, char **argv)
 	unsigned char sha1[20];
 	const char *ref1, *ref2;
 	setup_git_directory();
-	ref1 = resolve_ref("HEAD", sha1, 0, NULL);
-	ref2 = resolve_ref("HEAD", sha1, 0, NULL);
+	ref1 = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
+	ref2 = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
 	printf("ref1 = %s\nref2 = %s\n", ref1, ref2);
 	return 0;
 }
diff --git a/transport.c b/transport.c
index 51814b5..e9797c0 100644
--- a/transport.c
+++ b/transport.c
@@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
 		/* Follow symbolic refs (mainly for HEAD). */
 		localname = ref->peer_ref->name;
 		remotename = ref->name;
-		tmp = resolve_ref(localname, sha, 1, &flag);
+		tmp = resolve_ref_unsafe(localname, sha, 1, &flag);
 		if (tmp && flag & REF_ISSYMREF &&
 			!prefixcmp(tmp, "refs/heads/"))
 			localname = tmp;
-- 
1.7.8.36.g69ee2

Re: [PATCH 1/3] Convert resolve_ref+xstrdup to new resolve_refdup function

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> [Subject: Convert resolve_ref+xstrdup to new resolve_refdup function]

I like it.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -901,7 +901,7 @@ static int rollback_single_pick(void)
>  	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
>  	    !file_exists(git_path("REVERT_HEAD")))
>  		return error(_("no cherry-pick or revert in progress"));
> -	if (!resolve_ref("HEAD", head_sha1, 0, NULL))
> +	if (read_ref_full("HEAD", head_sha1, 0, NULL))
>  		return error(_("cannot resolve HEAD"));
>  	if (is_null_sha1(head_sha1))
>  		return error(_("cannot abort from a branch yet to be born"));

Unrelated change that snuck in, I assume?

The rest of the patch looks very sensible and no-op-like. :)

Re: [PATCH 1/3] Convert resolve_ref+xstrdup to new resolve_refdup function

[Nguyen Thai Ngoc Duy]

2011/12/10 Jonathan Nieder <jrnieder@gmail.com>:
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -901,7 +901,7 @@ static int rollback_single_pick(void)
>>       if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
>>           !file_exists(git_path("REVERT_HEAD")))
>>               return error(_("no cherry-pick or revert in progress"));
>> -     if (!resolve_ref("HEAD", head_sha1, 0, NULL))
>> +     if (read_ref_full("HEAD", head_sha1, 0, NULL))
>>               return error(_("cannot resolve HEAD"));
>>       if (is_null_sha1(head_sha1))
>>               return error(_("cannot abort from a branch yet to be born"));
>
> Unrelated change that snuck in, I assume?

Yeah that slipped in. It should be part of c689332 (Convert many
resolve_ref() calls to read_ref*() and ref_exists() - 2011-11-13). I
guess either I missed it or it was a new call site after that patch.
Split it out as a separate patch?
-- 
Duy

Re: [PATCH 3/3] Rename resolve_ref() to resolve_ref_unsafe()

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> resolve_ref() may return a pointer to a shared buffer and can be
> overwritten by the next resolve_ref() calls. Callers need to
> pay attention, not to keep the pointer when the next call happens.
[...]
> --- a/branch.c
> +++ b/branch.c
> @@ -182,7 +182,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
>  		const char *head;
>  		unsigned char sha1[20];
>  
> -		head = resolve_ref("HEAD", sha1, 0, NULL);
> +		head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);

I wonder if it would make sense to have a separate function that
maintains a shared buffer describing what HEAD resolves to, lazily
loaded.  Would invalidating the cache when appropriate be too fussy
and subtle?

[...]
> +++ b/transport.c
> @@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
>  		/* Follow symbolic refs (mainly for HEAD). */
>  		localname = ref->peer_ref->name;
>  		remotename = ref->name;
> -		tmp = resolve_ref(localname, sha, 1, &flag);
> +		tmp = resolve_ref_unsafe(localname, sha, 1, &flag);

Anyway, this patch looks sane.  The reminder seems useful and doesn't
feel over-the-top.

Re: [PATCH 2/3] Guard memory overwriting in resolve_ref's static buffer

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> Michael Haggerty
> has an idea [1] that, instead of passing the same static buffer to
> caller every time the function is called, we free the old buffer and
> allocate the new one. This way access to the old (now invalid) buffer
> may be caught.
>
> This patch applies the same principle for resolve_ref() with a
> few modifications:
[...]
>  - Rely on mmap/mprotect to catch illegal access. We need valgrind or
>    some other memory tracking tool to reliably catch this in Michael's
>    approach.

I wonder if the lower-tech approach would be so bad in practice.  On
systems using glibc, if MALLOC_PERTURB_ is set, then the value will
always be clobbered on free().  It would be possible to do the same
explicitly in resolve_ref() for platforms without such a feature.

>  - Because mprotect is used instead of munmap, we definitely leak
>    memory. Hopefully callers will not put resolve_ref() in a
>    loop that runs 1 million times.

Have you measured the performance impact when GIT_DEBUG_MEMCHECK is not
set?  (I don't expect major problems, but sometimes code surprises me.)

[...]
> Also introduce a new target, "make memcheck", that runs tests with this
> flag on.

Neat.  Did it catch any bugs?

> --- a/cache.h
> +++ b/cache.h
> @@ -865,7 +865,8 @@ extern int read_ref(const char *filename, unsigned char *sha1);
>   *
>   * errno is sometimes set on errors, but not always.
>   */
> -extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
> +#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)

__FUNCTION__ is nonstandard, though it's probably supported pretty
widely and we can do

	#ifndef __FUNCTION__
	#define __FUNCTION__ something-else
	#endif

in git-compat-util.h when we discover a platform that doesn't support
it if needed.  __func__ was introduced in C99.  __LINE__ and __FILE__
should work everywhere.

[...]
> --- /dev/null
> +++ b/t/t0071-memcheck.sh
> @@ -0,0 +1,12 @@
> +#!/bin/sh
> +
> +test_description='test that GIT_DEBUG_MEMCHECK works correctly'
> +. ./test-lib.sh
> +
> +test_expect_success 'test-resolve-ref must crash' '
> +	GIT_DEBUG_MEMCHECK=1 test-resolve-ref
> +	exit_code=$? &&
> +	test $exit_code -eq 139

Micronit: something like

	(
		GIT_DEBUG_MEMCHECK=1 &&
		export GIT_DEBUG_MEMCHECK &&
		test_expect_code 139 test-resolve-ref
	)

would fit better in an &&-list.

> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -60,6 +60,28 @@ void *xmallocz(size_t size)
>  	return ret;
>  }
>  
> +void *xmalloc_mmap(size_t size, const char *file, int line)
> +{
> +	int *ret;
> +	size += sizeof(int*) * 3;
> +	ret = mmap(NULL, size, PROT_READ | PROT_WRITE,
> +		   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +	if (ret == (int*)-1)
> +		die_errno("unable to mmap %lu bytes anonymously",
> +			  (unsigned long)size);
> +
> +	ret[0] = (int)file;
> +	ret[1] = line;
> +	ret[2] = size;
> +	return ret + 3;

Would this work on 64-bit platforms?

How does one retrieve the line and file number?  I guess one is
expected to retrieve them from the core dump, but a few words of
advice in Documentation/technical would be helpful.

Re: [PATCH 2/3] Guard memory overwriting in resolve_ref's static buffer

[Nguyen Thai Ngoc Duy]

2011/12/11 Jonathan Nieder <jrnieder@gmail.com>:
>>  - Rely on mmap/mprotect to catch illegal access. We need valgrind or
>>    some other memory tracking tool to reliably catch this in Michael's
>>    approach.
>
> I wonder if the lower-tech approach would be so bad in practice.  On
> systems using glibc, if MALLOC_PERTURB_ is set, then the value will
> always be clobbered on free().  It would be possible to do the same
> explicitly in resolve_ref() for platforms without such a feature.

Clobbered value may be carried around for some time before the code
detects wrong value. It'd be hard to track back where the root cause
is.

>>  - Because mprotect is used instead of munmap, we definitely leak
>>    memory. Hopefully callers will not put resolve_ref() in a
>>    loop that runs 1 million times.
>
> Have you measured the performance impact when GIT_DEBUG_MEMCHECK is not
> set?  (I don't expect major problems, but sometimes code surprises me.)

No. I wish we had a performance test suite. Which use cases should I test?

> [...]
>> Also introduce a new target, "make memcheck", that runs tests with this
>> flag on.
>
> Neat.  Did it catch any bugs?

No, otherwise I would have sent more patches ;)

>> --- a/cache.h
>> +++ b/cache.h
>> @@ -865,7 +865,8 @@ extern int read_ref(const char *filename, unsigned char *sha1);
>>   *
>>   * errno is sometimes set on errors, but not always.
>>   */
>> -extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
>> +#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)
>
> __FUNCTION__ is nonstandard, though it's probably supported pretty
> widely and we can do
>
>        #ifndef __FUNCTION__
>        #define __FUNCTION__ something-else
>        #endif
>
> in git-compat-util.h when we discover a platform that doesn't support
> it if needed.  __func__ was introduced in C99.  __LINE__ and __FILE__
> should work everywhere.

Will change to __FILE__ then

>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -60,6 +60,28 @@ void *xmallocz(size_t size)
>>       return ret;
>>  }
>>
>> +void *xmalloc_mmap(size_t size, const char *file, int line)
>> +{
>> +     int *ret;
>> +     size += sizeof(int*) * 3;
>> +     ret = mmap(NULL, size, PROT_READ | PROT_WRITE,
>> +                MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>> +     if (ret == (int*)-1)
>> +             die_errno("unable to mmap %lu bytes anonymously",
>> +                       (unsigned long)size);
>> +
>> +     ret[0] = (int)file;
>> +     ret[1] = line;
>> +     ret[2] = size;
>> +     return ret + 3;
>
> Would this work on 64-bit platforms?

No idea (I'm on 32-bit). I don't see any reasons why it would not
work. But that function may cause unaligned access, I think.

> How does one retrieve the line and file number?  I guess one is
> expected to retrieve them from the core dump, but a few words of
> advice in Documentation/technical would be helpful.

from coredump.
-- 
Duy

Re: [PATCH 3/3] Rename resolve_ref() to resolve_ref_unsafe()

[Nguyen Thai Ngoc Duy]

2011/12/11 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy wrote:
>
>> resolve_ref() may return a pointer to a shared buffer and can be
>> overwritten by the next resolve_ref() calls. Callers need to
>> pay attention, not to keep the pointer when the next call happens.
> [...]
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -182,7 +182,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
>>               const char *head;
>>               unsigned char sha1[20];
>>
>> -             head = resolve_ref("HEAD", sha1, 0, NULL);
>> +             head = resolve_ref_unsafe("HEAD", sha1, 0, NULL);
>
> I wonder if it would make sense to have a separate function that
> maintains a shared buffer describing what HEAD resolves to, lazily
> loaded.  Would invalidating the cache when appropriate be too fussy
> and subtle?

If we do not execute "git update-ref" from git binary (bisect does,
although on BISECT_HEAD, not HEAD) then it'd be safe to cache HEAD and
invalidate it in update_ref().
-- 
Duy

Re: [PATCH 1/3] Convert resolve_ref+xstrdup to new resolve_refdup function

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> Yeah that slipped in. It should be part of c689332 (Convert many
> resolve_ref() calls to read_ref*() and ref_exists() - 2011-11-13). I
> guess either I missed it or it was a new call site after that patch.
> Split it out as a separate patch?

Yeah, I think it makes sense to split the unrelated part out and place it
early in the series. It seems that you will be updating patch 2 in the
series for __FILE__ anyway so it's not like adding a useless code churn to
do so.

Thanks.