{"thread":{"id":"29137","subject":"[PATCH 1/4] revert: convert resolve_ref() to read_ref_full()","startedAt":"2011-12-12T11:20:29Z","lastAt":"2011-12-13T14:17:48Z","messageCount":9,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"180923","messageId":"1323688832-32016-1-git-send-email-pclouds@gmail.com","threadId":"29137","inReplyTo":null,"subject":"[PATCH 1/4] revert: convert resolve_ref() to read_ref_full()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-12T11:20:29Z","receivedAt":"2011-12-12T11:20:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This is the follow up of c689332 (Convert many resolve_ref() calls to\nread_ref*() and ref_exists() - 2011-11-13). See the said commit for\nrationale.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/revert.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 1ea525c..0c52a83 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -901,7 +901,7 @@ static int rollback_single_pick(void)\n \tif (!file_exists(git_path(\"CHERRY_PICK_HEAD\")) &&\n \t    !file_exists(git_path(\"REVERT_HEAD\")))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n-\tif (!resolve_ref(\"HEAD\", head_sha1, 0, NULL))\n+\tif (read_ref_full(\"HEAD\", head_sha1, 0, NULL))\n \t\treturn error(_(\"cannot resolve HEAD\"));\n \tif (is_null_sha1(head_sha1))\n \t\treturn error(_(\"cannot abort from a branch yet to be born\"));\n-- \n1.7.8.36.g69ee2\n"},{"id":"180924","messageId":"1323688832-32016-2-git-send-email-pclouds@gmail.com","threadId":"29137","inReplyTo":"1323688832-32016-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/4] Convert resolve_ref+xstrdup to new resolve_refdup function","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-12T11:20:30Z","receivedAt":"2011-12-12T11:20:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/branch.c        |   11 ++++-------\n builtin/checkout.c      |   15 +++++++--------\n builtin/fmt-merge-msg.c |    6 +++---\n builtin/for-each-ref.c  |    7 ++-----\n builtin/merge.c         |   12 +++++-------\n builtin/notes.c         |    6 +++---\n builtin/receive-pack.c  |    8 +++-----\n builtin/show-branch.c   |    4 +---\n cache.h                 |    1 +\n reflog-walk.c           |   13 ++++++-------\n refs.c                  |    6 ++++++\n wt-status.c             |    4 +---\n 12 files changed, 42 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e1e486e..72c4c31 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -103,7 +103,7 @@ static int branch_merged(int kind, const char *name,\n \t * safely to HEAD (or the other branch).\n \t */\n \tstruct commit *reference_rev = NULL;\n-\tconst char *reference_name = NULL;\n+\tchar *reference_name = NULL;\n \tint merged;\n \n \tif (kind == REF_LOCAL_BRANCH) {\n@@ -115,10 +115,8 @@ static int branch_merged(int kind, const char *name,\n \t\t    branch->merge[0] &&\n \t\t    branch->merge[0]->dst &&\n \t\t    (reference_name =\n-\t\t     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {\n-\t\t\treference_name = xstrdup(reference_name);\n+\t\t     resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n \t\t\treference_rev = lookup_commit_reference(sha1);\n-\t\t}\n \t}\n \tif (!reference_rev)\n \t\treference_rev = head_rev;\n@@ -143,7 +141,7 @@ static int branch_merged(int kind, const char *name,\n \t\t\t\t\"         '%s', even though it is merged to HEAD.\"),\n \t\t\t\tname, reference_name);\n \t}\n-\tfree((char *)reference_name);\n+\tfree(reference_name);\n \treturn merged;\n }\n \n@@ -731,10 +729,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \ttrack = git_branch_track;\n \n-\thead = resolve_ref(\"HEAD\", head_sha1, 0, NULL);\n+\thead = resolve_refdup(\"HEAD\", head_sha1, 0, NULL);\n \tif (!head)\n \t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n-\thead = xstrdup(head);\n \tif (!strcmp(head, \"HEAD\")) {\n \t\tdetached = 1;\n \t} else {\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b7c6302..a66d3eb 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n {\n \tint ret = 0;\n \tstruct branch_info old;\n+\tchar *path;\n \tunsigned char rev[20];\n \tint flag;\n \tmemset(&old, 0, sizeof(old));\n-\told.path = resolve_ref(\"HEAD\", rev, 0, &flag);\n-\tif (old.path)\n-\t\told.path = xstrdup(old.path);\n+\told.path = path = resolve_refdup(\"HEAD\", rev, 0, &flag);\n \told.commit = lookup_commit_reference_gently(rev, 1);\n-\tif (!(flag & REF_ISSYMREF)) {\n-\t\tfree((char *)old.path);\n+\tif (!(flag & REF_ISSYMREF))\n \t\told.path = NULL;\n-\t}\n \n \tif (old.path && !prefixcmp(old.path, \"refs/heads/\"))\n \t\told.name = old.path + strlen(\"refs/heads/\");\n@@ -720,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t}\n \n \tret = merge_working_tree(opts, &old, new);\n-\tif (ret)\n+\tif (ret) {\n+\t\tfree(path);\n \t\treturn ret;\n+\t}\n \n \tif (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n \t\torphaned_commit_warning(old.commit);\n@@ -729,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \tupdate_refs_for_switch(opts, &old, new);\n \n \tret = post_checkout_hook(old.commit, new->commit, 1);\n-\tfree((char *)old.path);\n+\tfree(path);\n \treturn ret || opts->writeout_error;\n }\n \ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex bdfa0ea..a27efcd 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -372,14 +372,14 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \tint i = 0, pos = 0;\n \tunsigned char head_sha1[20];\n \tconst char *current_branch;\n+\tchar *ref;\n \n \t/* get current branch */\n-\tcurrent_branch = resolve_ref(\"HEAD\", head_sha1, 1, NULL);\n+\tcurrent_branch = ref = resolve_refdup(\"HEAD\", head_sha1, 1, NULL);\n \tif (!current_branch)\n \t\tdie(\"No current branch\");\n \tif (!prefixcmp(current_branch, \"refs/heads/\"))\n \t\tcurrent_branch += 11;\n-\tcurrent_branch = xstrdup(current_branch);\n \n \t/* get a line */\n \twhile (pos < in->len) {\n@@ -421,7 +421,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t}\n \n \tstrbuf_complete_line(out);\n-\tfree((char *)current_branch);\n+\tfree(ref);\n \treturn 0;\n }\n \ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex d90e5d2..b01d76a 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)\n \n \tif (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {\n \t\tunsigned char unused1[20];\n-\t\tconst char *symref;\n-\t\tsymref = resolve_ref(ref->refname, unused1, 1, NULL);\n-\t\tif (symref)\n-\t\t\tref->symref = xstrdup(symref);\n-\t\telse\n+\t\tref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);\n+\t\tif (!ref->symref)\n \t\t\tref->symref = \"\";\n \t}\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a1c8534..6437db2 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n+\tchar *branch_ref;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n@@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n \t * current branch.\n \t */\n-\tbranch = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n-\tif (branch) {\n-\t\tif (!prefixcmp(branch, \"refs/heads/\"))\n-\t\t\tbranch += 11;\n-\t\tbranch = xstrdup(branch);\n-\t}\n+\tbranch = branch_ref = resolve_refdup(\"HEAD\", head_sha1, 0, &flag);\n+\tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n+\t\tbranch += 11;\n \tif (!branch || is_null_sha1(head_sha1))\n \t\thead_commit = NULL;\n \telse\n@@ -1520,6 +1518,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tret = suggest_conflicts(option_renormalize);\n \n done:\n-\tfree((char *)branch);\n+\tfree(branch_ref);\n \treturn ret;\n }\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 10b8bc7..816c998 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -804,6 +804,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tstruct notes_tree *t;\n \tstruct commit *partial;\n \tstruct pretty_print_context pretty_ctx;\n+\tchar *ref;\n \tint ret;\n \n \t/*\n@@ -826,10 +827,9 @@ static int merge_commit(struct notes_merge_options *o)\n \tt = xcalloc(1, sizeof(struct notes_tree));\n \tinit_notes(t, \"NOTES_MERGE_PARTIAL\", combine_notes_overwrite, 0);\n \n-\to->local_ref = resolve_ref(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n+\to->local_ref = ref = resolve_refdup(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n \tif (!o->local_ref)\n \t\tdie(\"Failed to resolve NOTES_MERGE_REF\");\n-\to->local_ref = xstrdup(o->local_ref);\n \n \tif (notes_merge_commit(o, t, partial, sha1))\n \t\tdie(\"Failed to finalize notes merge\");\n@@ -846,7 +846,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tfree_notes(t);\n \tstrbuf_release(&msg);\n \tret = merge_abort(o);\n-\tfree((char *)o->local_ref);\n+\tfree(ref);\n \treturn ret;\n }\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex b6d957c..5cd6fc7 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -36,7 +36,7 @@ static int use_sideband;\n static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n static int auto_gc = 1;\n-static const char *head_name;\n+static char *head_name;\n static int sent_capabilities;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n@@ -695,10 +695,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tcheck_aliased_updates(commands);\n \n-\tfree((char *)head_name);\n-\thead_name = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\tif (head_name)\n-\t\thead_name = xstrdup(head_name);\n+\tfree(head_name);\n+\thead_name = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n \n \tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\tif (!cmd->skip_update)\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 4b480d7..a1f148e 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -726,10 +726,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \n \t\tif (ac == 0) {\n \t\t\tstatic const char *fake_av[2];\n-\t\t\tconst char *refname;\n \n-\t\t\trefname = resolve_ref(\"HEAD\", sha1, 1, NULL);\n-\t\t\tfake_av[0] = xstrdup(refname);\n+\t\t\tfake_av[0] = resolve_refdup(\"HEAD\", sha1, 1, NULL);\n \t\t\tfake_av[1] = NULL;\n \t\t\tav = fake_av;\n \t\t\tac = 1;\ndiff --git a/cache.h b/cache.h\nindex 8c98d05..4887a3e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -866,6 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n  * errno is sometimes set on errors, but not always.\n  */\n extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n+extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex da71a85..5572b42 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -50,11 +50,10 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)\n \tfor_each_reflog_ent(ref, read_one_reflog, reflogs);\n \tif (reflogs->nr == 0) {\n \t\tunsigned char sha1[20];\n-\t\tconst char *name = resolve_ref(ref, sha1, 1, NULL);\n+\t\tchar *name = resolve_refdup(ref, sha1, 1, NULL);\n \t\tif (name) {\n-\t\t\tname = xstrdup(name);\n \t\t\tfor_each_reflog_ent(name, read_one_reflog, reflogs);\n-\t\t\tfree((char *)name);\n+\t\t\tfree(name);\n \t\t}\n \t}\n \tif (reflogs->nr == 0) {\n@@ -171,11 +170,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \telse {\n \t\tif (*branch == '\\0') {\n \t\t\tunsigned char sha1[20];\n-\t\t\tconst char *head = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\t\t\tif (!head)\n-\t\t\t\tdie (\"No current branch\");\n \t\t\tfree(branch);\n-\t\t\tbranch = xstrdup(head);\n+\t\t\tbranch = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n+\t\t\tif (!branch)\n+\t\t\t\tdie (\"No current branch\");\n+\n \t\t}\n \t\treflogs = read_complete_reflog(branch);\n \t\tif (!reflogs || reflogs->nr == 0) {\ndiff --git a/refs.c b/refs.c\nindex f5cb297..8ffb32f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -605,6 +605,12 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \treturn ref;\n }\n \n+char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n+{\n+\tconst char *ret = resolve_ref(ref, sha1, reading, flag);\n+\treturn ret ? xstrdup(ret) : NULL;\n+}\n+\n /* The argument to filter_refs */\n struct ref_filter {\n \tconst char *pattern;\ndiff --git a/wt-status.c b/wt-status.c\nindex 70fdb76..9ffc535 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -111,7 +111,6 @@ void status_printf_more(struct wt_status *s, const char *color,\n void wt_status_prepare(struct wt_status *s)\n {\n \tunsigned char sha1[20];\n-\tconst char *head;\n \n \tmemset(s, 0, sizeof(*s));\n \tmemcpy(s->color_palette, default_wt_status_colors,\n@@ -119,8 +118,7 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;\n \ts->use_color = -1;\n \ts->relative_paths = 1;\n-\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\ts->branch = head ? xstrdup(head) : NULL;\n+\ts->branch = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n \ts->reference = \"HEAD\";\n \ts->fp = stdout;\n \ts->index_file = get_index_file();\n-- \n1.7.8.36.g69ee2\n"},{"id":"180925","messageId":"1323688832-32016-3-git-send-email-pclouds@gmail.com","threadId":"29137","inReplyTo":"1323688832-32016-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/4] Guard memory overwriting in resolve_ref's static buffer","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-12T11:20:31Z","receivedAt":"2011-12-12T11:20:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a potential problem with resolve_ref() and some other\nfunctions in git. The return value returned by resolve_ref() may\nbe changed when the function is called again. Callers must make sure the\nnext call won't happen as long as the value is still being used.\n\nIt's usually hard to track down this kind of problem.  Michael Haggerty\nhas an idea [1] that, instead of passing the same static buffer to\ncaller every time the function is called, we free the old buffer and\nallocate the new one. This way access to the old (now invalid) buffer\nmay be caught.\n\nThis patch applies the same principle for resolve_ref() with a\nfew modifications:\n\n - This behavior is enabled when GIT_DEBUG_MEMCHECK is set. The ability\n   is always available. We may be able to ask users to rerun with this\n   flag on in suspicious cases.\n\n - Rely on mmap/mprotect to catch illegal access. We need valgrind or\n   some other memory tracking tool to reliably catch this in Michael's\n   approach.\n\n - Because mprotect is used instead of munmap, we definitely leak\n   memory. Hopefully callers will not put resolve_ref() in a\n   loop that runs 1 million times.\n\n - Save caller location in the allocated buffer so we know who made this\n   call in the core dump.\n\nAlso introduce a new target, \"make memcheck\", that runs tests with this\nflag on.\n\n[1] http://comments.gmane.org/gmane.comp.version-control.git/182209\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Changes include:\n\n  - __FUNCTION__ to __FILE__ for compiler compatibility\n  - x{malloc,free}_mmap() now put call site information in a struct,\n    it's clearer this way and hopefully will avoid platform issues\n  - update t0071 to follow '&&' convention\n  - add notes where to get caller site info in git-compat-util.h\n\n .gitignore          |    1 +\n Makefile            |    4 ++++\n cache.h             |    3 ++-\n git-compat-util.h   |   20 ++++++++++++++++++++\n refs.c              |   13 +++++++++++--\n t/t0071-memcheck.sh |   11 +++++++++++\n test-resolve-ref.c  |   18 ++++++++++++++++++\n wrapper.c           |   27 +++++++++++++++++++++++++++\n 8 files changed, 94 insertions(+), 3 deletions(-)\n create mode 100755 t/t0071-memcheck.sh\n create mode 100644 test-resolve-ref.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 8572c8c..470e452 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -179,6 +179,7 @@\n /test-obj-pool\n /test-parse-options\n /test-path-utils\n+/test-resolve-ref\n /test-run-command\n /test-sha1\n /test-sigchain\ndiff --git a/Makefile b/Makefile\nindex ed82320..d71cf04 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -444,6 +444,7 @@ TEST_PROGRAMS_NEED_X += test-obj-pool\n TEST_PROGRAMS_NEED_X += test-parse-options\n TEST_PROGRAMS_NEED_X += test-path-utils\n TEST_PROGRAMS_NEED_X += test-run-command\n+TEST_PROGRAMS_NEED_X += test-resolve-ref\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\n TEST_PROGRAMS_NEED_X += test-string-pool\n@@ -2241,6 +2242,9 @@ export NO_SVN_TESTS\n test: all\n \t$(MAKE) -C t/ all\n \n+memcheck: all\n+\tGIT_DEBUG_MEMCHECK=1 $(MAKE) -C t/ all\n+\n test-ctype$X: ctype.o\n \n test-date$X: date.o ctype.o\ndiff --git a/cache.h b/cache.h\nindex 4887a3e..ba5e911 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -865,7 +865,8 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n  *\n  * errno is sometimes set on errors, but not always.\n  */\n-extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n+#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FILE__, __LINE__)\n+extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);\n extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 77062ed..0cb6e34 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -433,6 +433,26 @@ extern char *xstrndup(const char *str, size_t len);\n extern void *xrealloc(void *ptr, size_t size);\n extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n+\n+/*\n+ * These functions are used to allocate new memory blocks and catch\n+ * invalid use after they are released (though the memory is never\n+ * returned to system, so do not allocate too much this way).\n+ *\n+ * mprotect() is used to remove all access to memory when xfree_mmap()\n+ * is called. Invalid access will cause sigsegv. The memory block is\n+ * preceded by struct alloc_header, describing where it is\n+ * allocated. This information can be found in the core dump.\n+ */\n+struct alloc_header {\n+\tconst char *file;\n+\tint line;\n+\tint size;\n+\tchar buf[FLEX_ARRAY];\n+};\n+extern void *xmalloc_mmap(size_t, const char *file, int line);\n+extern void xfree_mmap(void *);\n+\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern int xdup(int fd);\ndiff --git a/refs.c b/refs.c\nindex 8ffb32f..cf8dfcc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -497,12 +497,21 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)\n \treturn -1;\n }\n \n-const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)\n+const char *resolve_ref_real(const char *ref, unsigned char *sha1,\n+\t\t\t     int reading, int *flag, const char *file, int line)\n {\n \tint depth = MAXDEPTH;\n \tssize_t len;\n \tchar buffer[256];\n-\tstatic char ref_buffer[256];\n+\tstatic char real_ref_buffer[256];\n+\tstatic char *ref_buffer;\n+\n+\tif (!ref_buffer && !getenv(\"GIT_DEBUG_MEMCHECK\"))\n+\t\tref_buffer = real_ref_buffer;\n+\tif (ref_buffer != real_ref_buffer) {\n+\t\txfree_mmap(ref_buffer);\n+\t\tref_buffer = xmalloc_mmap(256, file, line);\n+\t}\n \n \tif (flag)\n \t\t*flag = 0;\ndiff --git a/t/t0071-memcheck.sh b/t/t0071-memcheck.sh\nnew file mode 100755\nindex 0000000..8904369\n--- /dev/null\n+++ b/t/t0071-memcheck.sh\n@@ -0,0 +1,11 @@\n+#!/bin/sh\n+\n+test_description='test that GIT_DEBUG_MEMCHECK works correctly'\n+. ./test-lib.sh\n+\n+test_expect_success 'test-resolve-ref must crash' '\n+\ttest-resolve-ref &&\n+\tGIT_DEBUG_MEMCHECK=1 test_expect_code 139 test-resolve-ref\n+'\n+\n+test_done\ndiff --git a/test-resolve-ref.c b/test-resolve-ref.c\nnew file mode 100644\nindex 0000000..b772038\n--- /dev/null\n+++ b/test-resolve-ref.c\n@@ -0,0 +1,18 @@\n+#include \"cache.h\"\n+\n+int main(int argc, char **argv)\n+{\n+\tunsigned char sha1[20];\n+\tconst char *ref1, *ref2;\n+\tsetup_git_directory();\n+\n+\t/*\n+\t * This is an invalid use of resolve_ref_unsafe(). This\n+\t * function returns a shared buffer, so by the time the second\n+\t * call is made, ref1 must _not_ be accessed any more.\n+\t */\n+\tref1 = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\tref2 = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\tprintf(\"ref1 = %s\\nref2 = %s\\n\", ref1, ref2);\n+\treturn 0;\n+}\ndiff --git a/wrapper.c b/wrapper.c\nindex 85f09df..02b6c81 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -60,6 +60,33 @@ void *xmallocz(size_t size)\n \treturn ret;\n }\n \n+void *xmalloc_mmap(size_t size, const char *file, int line)\n+{\n+\tstruct alloc_header *block;\n+\tsize += offsetof(struct alloc_header,buf);\n+\tblock = mmap(NULL, size, PROT_READ | PROT_WRITE,\n+\t\t   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);\n+\tif (block == (struct alloc_header*)-1)\n+\t\tdie_errno(\"unable to mmap %lu bytes anonymously\",\n+\t\t\t  (unsigned long)size);\n+\n+\tblock->file = file;\n+\tblock->line = line;\n+\tblock->size = size;\n+\treturn block->buf;\n+}\n+\n+void xfree_mmap(void *p)\n+{\n+\tstruct alloc_header *block;\n+\n+\tif (!p)\n+\t\treturn;\n+\tblock = (struct alloc_header *)((char*)p - offsetof(struct alloc_header,buf));\n+\tif (mprotect(block, block->size, PROT_NONE) == -1)\n+\t\tdie_errno(\"unable to remove memory access\");\n+}\n+\n /*\n  * xmemdupz() allocates (len + 1) bytes of memory, duplicates \"len\" bytes of\n  * \"data\" to the allocated memory, zero terminates the allocated memory,\n-- \n1.7.8.36.g69ee2\n"},{"id":"180926","messageId":"1323688832-32016-4-git-send-email-pclouds@gmail.com","threadId":"29137","inReplyTo":"1323688832-32016-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/4] Rename resolve_ref() to resolve_ref_unsafe()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-12T11:20:32Z","receivedAt":"2011-12-12T11:20:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"resolve_ref() may return a pointer to a shared buffer and can be\noverwritten by the next resolve_ref() calls. Callers need to\npay attention, not to keep the pointer when the next call happens.\n\nRename with \"_unsafe\" suffix to warn developers (or reviewers) before\nintroducing new call sites.\n\nThis patch is generated using the following command\n\ngit grep -l 'resolve_ref(' -- '*.[ch]'|xargs sed -i 's/resolve_ref(/resolve_ref_unsafe(/g'\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n branch.c               |    2 +-\n builtin/branch.c       |    2 +-\n builtin/commit.c       |    2 +-\n builtin/fsck.c         |    2 +-\n builtin/log.c          |    4 ++--\n builtin/receive-pack.c |    2 +-\n builtin/remote.c       |    2 +-\n builtin/show-branch.c  |    2 +-\n builtin/symbolic-ref.c |    2 +-\n cache.h                |    2 +-\n refs.c                 |   20 ++++++++++----------\n remote.c               |    6 +++---\n test-resolve-ref.c     |    4 ++--\n transport.c            |    2 +-\n 14 files changed, 27 insertions(+), 27 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex d91a099..772a4c2 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -182,7 +182,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,\n \t\tconst char *head;\n \t\tunsigned char sha1[20];\n \n-\t\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\t\thead = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n \t\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n \t\t\tdie(\"Cannot force update the current branch.\");\n \t}\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 72c4c31..641bee1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -251,7 +251,7 @@ static char *resolve_symref(const char *src, const char *prefix)\n \tint flag;\n \tconst char *dst, *cp;\n \n-\tdst = resolve_ref(src, sha1, 0, &flag);\n+\tdst = resolve_ref_unsafe(src, sha1, 0, &flag);\n \tif (!(dst && (flag & REF_ISSYMREF)))\n \t\treturn NULL;\n \tif (prefix && (cp = skip_prefix(dst, prefix)))\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e36e9ad..4d39d25 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1304,7 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \trev.diffopt.break_opt = 0;\n \tdiff_setup_done(&rev.diffopt);\n \n-\thead = resolve_ref(\"HEAD\", junk_sha1, 0, NULL);\n+\thead = resolve_ref_unsafe(\"HEAD\", junk_sha1, 0, NULL);\n \tprintf(\"[%s%s \",\n \t\t!prefixcmp(head, \"refs/heads/\") ?\n \t\t\thead + 11 :\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 30d0dc8..8c479a7 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -563,7 +563,7 @@ static int fsck_head_link(void)\n \tif (verbose)\n \t\tfprintf(stderr, \"Checking HEAD link\\n\");\n \n-\thead_points_at = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n+\thead_points_at = resolve_ref_unsafe(\"HEAD\", head_sha1, 0, &flag);\n \tif (!head_points_at)\n \t\treturn error(\"Invalid HEAD\");\n \tif (!strcmp(head_points_at, \"HEAD\"))\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4395f3e..89d0cc0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1040,7 +1040,7 @@ static char *find_branch_name(struct rev_info *rev)\n \tif (positive < 0)\n \t\treturn NULL;\n \tstrbuf_addf(&buf, \"refs/heads/%s\", rev->cmdline.rev[positive].name);\n-\tbranch = resolve_ref(buf.buf, branch_sha1, 1, NULL);\n+\tbranch = resolve_ref_unsafe(buf.buf, branch_sha1, 1, NULL);\n \tif (!branch ||\n \t    prefixcmp(branch, \"refs/heads/\") ||\n \t    hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))\n@@ -1268,7 +1268,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \n \t\t\trev.pending.objects[0].item->flags |= UNINTERESTING;\n \t\t\tadd_head_to_pending(&rev);\n-\t\t\tref = resolve_ref(\"HEAD\", sha1, 1, NULL);\n+\t\t\tref = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n \t\t\tif (ref && !prefixcmp(ref, \"refs/heads/\"))\n \t\t\t\tbranch_name = xstrdup(ref + strlen(\"refs/heads/\"));\n \t\t\telse\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 5cd6fc7..a1a4b09 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -571,7 +571,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \tint flag;\n \n \tstrbuf_addf(&buf, \"%s%s\", get_git_namespace(), cmd->ref_name);\n-\tdst_name = resolve_ref(buf.buf, sha1, 0, &flag);\n+\tdst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag);\n \tstrbuf_release(&buf);\n \n \tif (!(flag & REF_ISSYMREF))\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 407abfb..583eec9 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -573,7 +573,7 @@ static int read_remote_branches(const char *refname,\n \tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old);\n \tif (!prefixcmp(refname, buf.buf)) {\n \t\titem = string_list_append(rename->remote_branches, xstrdup(refname));\n-\t\tsymref = resolve_ref(refname, orig_sha1, 1, &flag);\n+\t\tsymref = resolve_ref_unsafe(refname, orig_sha1, 1, &flag);\n \t\tif (flag & REF_ISSYMREF)\n \t\t\titem->util = xstrdup(symref);\n \t\telse\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex a1f148e..a59e088 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -789,7 +789,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t}\n \t}\n \n-\thead_p = resolve_ref(\"HEAD\", head_sha1, 1, NULL);\n+\thead_p = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n \tif (head_p) {\n \t\thead_len = strlen(head_p);\n \t\tmemcpy(head, head_p, head_len + 1);\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex dea849c..2ef5962 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -12,7 +12,7 @@ static void check_symref(const char *HEAD, int quiet)\n {\n \tunsigned char sha1[20];\n \tint flag;\n-\tconst char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);\n+\tconst char *refs_heads_master = resolve_ref_unsafe(HEAD, sha1, 0, &flag);\n \n \tif (!refs_heads_master)\n \t\tdie(\"No such ref: %s\", HEAD);\ndiff --git a/cache.h b/cache.h\nindex ba5e911..051e7ee 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -865,7 +865,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n  *\n  * errno is sometimes set on errors, but not always.\n  */\n-#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FILE__, __LINE__)\n+#define resolve_ref_unsafe(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FILE__, __LINE__)\n extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);\n extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n \ndiff --git a/refs.c b/refs.c\nindex cf8dfcc..010bb72 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -361,7 +361,7 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha\n \tif (!(flags & REF_ISSYMREF))\n \t\treturn 0;\n \n-\tresolves_to = resolve_ref(refname, junk, 0, NULL);\n+\tresolves_to = resolve_ref_unsafe(refname, junk, 0, NULL);\n \tif (!resolves_to || strcmp(resolves_to, d->refname))\n \t\treturn 0;\n \n@@ -616,7 +616,7 @@ const char *resolve_ref_real(const char *ref, unsigned char *sha1,\n \n char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n {\n-\tconst char *ret = resolve_ref(ref, sha1, reading, flag);\n+\tconst char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);\n \treturn ret ? xstrdup(ret) : NULL;\n }\n \n@@ -629,7 +629,7 @@ struct ref_filter {\n \n int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)\n {\n-\tif (resolve_ref(ref, sha1, reading, flags))\n+\tif (resolve_ref_unsafe(ref, sha1, reading, flags))\n \t\treturn 0;\n \treturn -1;\n }\n@@ -1126,7 +1126,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n \t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n-\t\tr = resolve_ref(fullref, this_result, 1, &flag);\n+\t\tr = resolve_ref_unsafe(fullref, this_result, 1, &flag);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -1156,7 +1156,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tconst char *ref, *it;\n \n \t\tmksnpath(path, sizeof(path), *p, len, str);\n-\t\tref = resolve_ref(path, hash, 1, NULL);\n+\t\tref = resolve_ref_unsafe(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n \t\tif (!stat(git_path(\"logs/%s\", path), &st) &&\n@@ -1192,7 +1192,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n \n-\tref = resolve_ref(ref, lock->old_sha1, mustexist, &type);\n+\tref = resolve_ref_unsafe(ref, lock->old_sha1, mustexist, &type);\n \tif (!ref && errno == EISDIR) {\n \t\t/* we are trying to lock foo but we used to\n \t\t * have foo/bar which now does not exist;\n@@ -1205,7 +1205,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\t\terror(\"there are still refs under '%s'\", orig_ref);\n \t\t\tgoto error_return;\n \t\t}\n-\t\tref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);\n+\t\tref = resolve_ref_unsafe(orig_ref, lock->old_sha1, mustexist, &type);\n \t}\n \tif (type_p)\n \t    *type_p = type;\n@@ -1368,7 +1368,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldref);\n \n-\tsymref = resolve_ref(oldref, orig_sha1, 1, &flag);\n+\tsymref = resolve_ref_unsafe(oldref, orig_sha1, 1, &flag);\n \tif (flag & REF_ISSYMREF)\n \t\treturn error(\"refname %s is a symbolic ref, renaming it is not supported\",\n \t\t\toldref);\n@@ -1657,7 +1657,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunsigned char head_sha1[20];\n \t\tint head_flag;\n \t\tconst char *head_ref;\n-\t\thead_ref = resolve_ref(\"HEAD\", head_sha1, 1, &head_flag);\n+\t\thead_ref = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, &head_flag);\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t    !strcmp(head_ref, lock->ref_name))\n \t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n@@ -1994,7 +1994,7 @@ int update_ref(const char *action, const char *refname,\n int ref_exists(const char *refname)\n {\n \tunsigned char sha1[20];\n-\treturn !!resolve_ref(refname, sha1, 1, NULL);\n+\treturn !!resolve_ref_unsafe(refname, sha1, 1, NULL);\n }\n \n struct ref *find_ref_by_name(const struct ref *list, const char *name)\ndiff --git a/remote.c b/remote.c\nindex 6655bb0..73a3809 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -482,7 +482,7 @@ static void read_config(void)\n \t\treturn;\n \tdefault_remote_name = xstrdup(\"origin\");\n \tcurrent_branch = NULL;\n-\thead_ref = resolve_ref(\"HEAD\", sha1, 0, &flag);\n+\thead_ref = resolve_ref_unsafe(\"HEAD\", sha1, 0, &flag);\n \tif (head_ref && (flag & REF_ISSYMREF) &&\n \t    !prefixcmp(head_ref, \"refs/heads/\")) {\n \t\tcurrent_branch =\n@@ -1007,7 +1007,7 @@ static char *guess_ref(const char *name, struct ref *peer)\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned char sha1[20];\n \n-\tconst char *r = resolve_ref(peer->name, sha1, 1, NULL);\n+\tconst char *r = resolve_ref_unsafe(peer->name, sha1, 1, NULL);\n \tif (!r)\n \t\treturn NULL;\n \n@@ -1058,7 +1058,7 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\tunsigned char sha1[20];\n \t\tint flag;\n \n-\t\tdst_value = resolve_ref(matched_src->name, sha1, 1, &flag);\n+\t\tdst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);\n \t\tif (!dst_value ||\n \t\t    ((flag & REF_ISSYMREF) &&\n \t\t     prefixcmp(dst_value, \"refs/heads/\")))\ndiff --git a/test-resolve-ref.c b/test-resolve-ref.c\nindex b772038..59d04e0 100644\n--- a/test-resolve-ref.c\n+++ b/test-resolve-ref.c\n@@ -11,8 +11,8 @@ int main(int argc, char **argv)\n \t * function returns a shared buffer, so by the time the second\n \t * call is made, ref1 must _not_ be accessed any more.\n \t */\n-\tref1 = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\tref2 = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\tref1 = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n+\tref2 = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n \tprintf(\"ref1 = %s\\nref2 = %s\\n\", ref1, ref2);\n \treturn 0;\n }\ndiff --git a/transport.c b/transport.c\nindex 51814b5..e9797c0 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,\n \t\t/* Follow symbolic refs (mainly for HEAD). */\n \t\tlocalname = ref->peer_ref->name;\n \t\tremotename = ref->name;\n-\t\ttmp = resolve_ref(localname, sha, 1, &flag);\n+\t\ttmp = resolve_ref_unsafe(localname, sha, 1, &flag);\n \t\tif (tmp && flag & REF_ISSYMREF &&\n \t\t\t!prefixcmp(tmp, \"refs/heads/\"))\n \t\t\tlocalname = tmp;\n-- \n1.7.8.36.g69ee2\n"},{"id":"180940","messageId":"7vpqft4qap.fsf@alter.siamese.dyndns.org","threadId":"29137","inReplyTo":"1323688832-32016-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/4] Convert resolve_ref+xstrdup to new resolve_refdup function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-12T18:07:26Z","receivedAt":"2011-12-12T18:07:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nThanks.\n\nThe patch looks correct but I have a slight maintainability concern and a\nsuggestion for possible improvement.\n\n> ...\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index b7c6302..a66d3eb 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>  {\n>  \tint ret = 0;\n>  \tstruct branch_info old;\n> +\tchar *path;\n>  \tunsigned char rev[20];\n>  \tint flag;\n>  \tmemset(&old, 0, sizeof(old));\n> -\told.path = resolve_ref(\"HEAD\", rev, 0, &flag);\n> -\tif (old.path)\n> -\t\told.path = xstrdup(old.path);\n> +\told.path = path = resolve_refdup(\"HEAD\", rev, 0, &flag);\n\nThis uses \"one 'const char *' pointer that is used for reading data from\nand an extra 'char *' pointer that is used only for freeing\" approach,\nwhich has two advantages and one disadvantage:\n\n + Obviously, we do not have to cast away constness when freeing.\n\n + A caller that follows the pattern could move the \"for-data\" pointer\n   without having to worry about affecting \"for-freeing\" pointer. It could\n   even do something like this:\n\n     char *for_freeing;\n     const char *for_data;\n     for_data = for_freeing = resolve_refdup(...);\n\n     if (prefixcmp(for_data, \"refs/heads/\"))\n     \tfor_data = skip_prefix(for_data, \"refs/heads/\");\n     ... do other things using for_data pointer ...\n\n     free(for_freeing);\n\n - If the for-freeing and for-data pointers are named too similarly, the\n   code becomes harder to read. It is easy for a person who is new to the\n   codebase to start using the for-freeing pointer to manipulate the data\n   (mostly harmless) or even advance the pointer by mistake (bad).\n\nA handful of places in the existing codebase use this \"two pointers\"\napproach primarily for the second advantage. The way they avoid the\ndisadvantage is by naming the other pointer \"$something_to_free\" (and the\n\"$something_\" part is optional if there is only one such variable in the\ncontext) to make it clear that the variable is not meant to be looked at\nin the code and its primary purpose is for cleaning up at the end.\n\nPerhaps renaming this \"path\" to \"to_free\" (or \"path_to_free\" if you want\nto hint the \"path-ness\" to the readers, but see below) would make it less\nlikely that future tweakers mistakenly look at, modify the string thru, or\nworse yet move the pointer itself.\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index a1c8534..6437db2 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \tstruct commit_list *common = NULL;\n>  \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n>  \tstruct commit_list **remotes = &remoteheads;\n> +\tchar *branch_ref;\n>  \n>  \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n>  \t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n> @@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n>  \t * current branch.\n>  \t */\n> -\tbranch = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n> -\tif (branch) {\n> -\t\tif (!prefixcmp(branch, \"refs/heads/\"))\n> -\t\t\tbranch += 11;\n> -\t\tbranch = xstrdup(branch);\n> -\t}\n> +\tbranch = branch_ref = resolve_refdup(\"HEAD\", head_sha1, 0, &flag);\n\nWithout s/branch_ref/to_free/ this part is unreadable and unmaintainable,\nas it invites the \"which variable should I use?\" question.\n\nIt is more important to clarify that the \"branch\" variable is what the\ncode should look at and manipulate by *not* calling this for-freeing\npointer after what it contains (i.e. \"branch\").\n\nWhen naming a \"for-freeing\" pointer variable, the kind of data the area of\nmemory happens to contain is of secondary importance. Being deliberately\nvague about what the area of memory may contain is a good thing, because\nit actively discourages the program from looking at the area via the\npointer if the variable is named \"to_free\" or something that does not\nspecify what it contains.\n\nOther places in this patch that call the for-freeing variable \"ref\" share\nthe same issue but they are less specific than their for-data variable\ncounterpart, which makes it slightly less risky than this instance.\n"},{"id":"180999","messageId":"7v8vmhz4ql.fsf@alter.siamese.dyndns.org","threadId":"29137","inReplyTo":"1323688832-32016-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/4] Guard memory overwriting in resolve_ref's static buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-13T00:37:22Z","receivedAt":"2011-12-13T00:37:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> diff --git a/cache.h b/cache.h\n> index 4887a3e..ba5e911 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -865,7 +865,8 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n>   *\n>   * errno is sometimes set on errors, but not always.\n>   */\n> -extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n> +#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FILE__, __LINE__)\n> +extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);\n>  extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n>  \n>  extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n\nEek.\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 85f09df..02b6c81 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -60,6 +60,33 @@ void *xmallocz(size_t size)\n>  \treturn ret;\n>  }\n>  \n> +void *xmalloc_mmap(size_t size, const char *file, int line)\n> +{\n> +\tstruct alloc_header *block;\n> +\tsize += offsetof(struct alloc_header,buf);\n> +\tblock = mmap(NULL, size, PROT_READ | PROT_WRITE,\n> +\t\t   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);\n> +\tif (block == (struct alloc_header*)-1)\n> +\t\tdie_errno(\"unable to mmap %lu bytes anonymously\",\n> +\t\t\t  (unsigned long)size);\n> +\n> +\tblock->file = file;\n> +\tblock->line = line;\n> +\tblock->size = size;\n> +\treturn block->buf;\n> +}\n> +\n\nDouble eek. A refname is ordinarily way too small than a page and we spend\na full page every time resolve_ref_unsafe() is called. That is acceptable\nonly for debugging build, but then having to patch the main codepath like\nthe following, renaming the \"real\" implementation of a rather important\nfunction is not acceptable in a non-debugging build.\n\n> diff --git a/refs.c b/refs.c\n> index 8ffb32f..cf8dfcc 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -497,12 +497,21 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)\n>  \treturn -1;\n>  }\n>  \n> -const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)\n> +const char *resolve_ref_real(const char *ref, unsigned char *sha1,\n> +\t\t\t     int reading, int *flag, const char *file, int line)\n>  {\n>  \tint depth = MAXDEPTH;\n>  \tssize_t len;\n>  \tchar buffer[256];\n> -\tstatic char ref_buffer[256];\n> +\tstatic char real_ref_buffer[256];\n> +\tstatic char *ref_buffer;\n> +\n> +\tif (!ref_buffer && !getenv(\"GIT_DEBUG_MEMCHECK\"))\n> +\t\tref_buffer = real_ref_buffer;\n> +\tif (ref_buffer != real_ref_buffer) {\n> +\t\txfree_mmap(ref_buffer);\n> +\t\tref_buffer = xmalloc_mmap(256, file, line);\n> +\t}\n\nI'll drop 3/4 from the series, adjust 4/4, and queue the result as a\nthree-patch series for now.\n\nThanks.\n"},{"id":"181033","messageId":"1323779498-10396-1-git-send-email-pclouds@gmail.com","threadId":"29137","inReplyTo":"7vpqft4qap.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Convert resolve_ref+xstrdup to new resolve_refdup function","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-13T12:31:38Z","receivedAt":"2011-12-13T12:31:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n 2011/12/13 Junio C Hamano <gitster@pobox.com>:\n > A handful of places in the existing codebase use this \"two pointers\"\n > approach primarily for the second advantage. The way they avoid the\n > disadvantage is by naming the other pointer \"$something_to_free\" (and the\n > \"$something_\" part is optional if there is only one such variable in the\n > context) to make it clear that the variable is not meant to be looked at\n > in the code and its primary purpose is for cleaning up at the end.\n >\n > Perhaps renaming this \"path\" to \"to_free\" (or \"path_to_free\" if you want\n > to hint the \"path-ness\" to the readers, but see below) would make it less\n > likely that future tweakers mistakenly look at, modify the string thru, or\n > worse yet move the pointer itself.\n\n Thanks. The patch looks better with \"_to_free\" naming convention. I\n learn something new today.\n\n builtin/branch.c        |   12 +++++-------\n builtin/checkout.c      |   15 +++++++--------\n builtin/fmt-merge-msg.c |    7 ++++---\n builtin/for-each-ref.c  |    7 ++-----\n builtin/merge.c         |   12 +++++-------\n builtin/notes.c         |    7 ++++---\n builtin/receive-pack.c  |    7 +++----\n builtin/show-branch.c   |    4 +---\n cache.h                 |    1 +\n reflog-walk.c           |   15 ++++++++-------\n refs.c                  |    6 ++++++\n wt-status.c             |    4 +---\n 12 files changed, 47 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e1e486e..59efe2c 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -104,6 +104,7 @@ static int branch_merged(int kind, const char *name,\n \t */\n \tstruct commit *reference_rev = NULL;\n \tconst char *reference_name = NULL;\n+\tchar *reference_name_to_free = NULL;\n \tint merged;\n \n \tif (kind == REF_LOCAL_BRANCH) {\n@@ -114,11 +115,9 @@ static int branch_merged(int kind, const char *name,\n \t\t    branch->merge &&\n \t\t    branch->merge[0] &&\n \t\t    branch->merge[0]->dst &&\n-\t\t    (reference_name =\n-\t\t     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {\n-\t\t\treference_name = xstrdup(reference_name);\n+\t\t    (reference_name = reference_name_to_free =\n+\t\t     resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n \t\t\treference_rev = lookup_commit_reference(sha1);\n-\t\t}\n \t}\n \tif (!reference_rev)\n \t\treference_rev = head_rev;\n@@ -143,7 +142,7 @@ static int branch_merged(int kind, const char *name,\n \t\t\t\t\"         '%s', even though it is merged to HEAD.\"),\n \t\t\t\tname, reference_name);\n \t}\n-\tfree((char *)reference_name);\n+\tfree(reference_name_to_free);\n \treturn merged;\n }\n \n@@ -731,10 +730,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \ttrack = git_branch_track;\n \n-\thead = resolve_ref(\"HEAD\", head_sha1, 0, NULL);\n+\thead = resolve_refdup(\"HEAD\", head_sha1, 0, NULL);\n \tif (!head)\n \t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n-\thead = xstrdup(head);\n \tif (!strcmp(head, \"HEAD\")) {\n \t\tdetached = 1;\n \t} else {\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b7c6302..dde44d7 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n {\n \tint ret = 0;\n \tstruct branch_info old;\n+\tchar *path_to_free;\n \tunsigned char rev[20];\n \tint flag;\n \tmemset(&old, 0, sizeof(old));\n-\told.path = resolve_ref(\"HEAD\", rev, 0, &flag);\n-\tif (old.path)\n-\t\told.path = xstrdup(old.path);\n+\told.path = path_to_free = resolve_refdup(\"HEAD\", rev, 0, &flag);\n \told.commit = lookup_commit_reference_gently(rev, 1);\n-\tif (!(flag & REF_ISSYMREF)) {\n-\t\tfree((char *)old.path);\n+\tif (!(flag & REF_ISSYMREF))\n \t\told.path = NULL;\n-\t}\n \n \tif (old.path && !prefixcmp(old.path, \"refs/heads/\"))\n \t\told.name = old.path + strlen(\"refs/heads/\");\n@@ -720,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t}\n \n \tret = merge_working_tree(opts, &old, new);\n-\tif (ret)\n+\tif (ret) {\n+\t\tfree(path_to_free);\n \t\treturn ret;\n+\t}\n \n \tif (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n \t\torphaned_commit_warning(old.commit);\n@@ -729,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \tupdate_refs_for_switch(opts, &old, new);\n \n \tret = post_checkout_hook(old.commit, new->commit, 1);\n-\tfree((char *)old.path);\n+\tfree(path_to_free);\n \treturn ret || opts->writeout_error;\n }\n \ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex bdfa0ea..e55e994 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -372,14 +372,15 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \tint i = 0, pos = 0;\n \tunsigned char head_sha1[20];\n \tconst char *current_branch;\n+\tchar *current_branch_to_free;\n \n \t/* get current branch */\n-\tcurrent_branch = resolve_ref(\"HEAD\", head_sha1, 1, NULL);\n+\tcurrent_branch = current_branch_to_free =\n+\t\tresolve_refdup(\"HEAD\", head_sha1, 1, NULL);\n \tif (!current_branch)\n \t\tdie(\"No current branch\");\n \tif (!prefixcmp(current_branch, \"refs/heads/\"))\n \t\tcurrent_branch += 11;\n-\tcurrent_branch = xstrdup(current_branch);\n \n \t/* get a line */\n \twhile (pos < in->len) {\n@@ -421,7 +422,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t}\n \n \tstrbuf_complete_line(out);\n-\tfree((char *)current_branch);\n+\tfree(current_branch_to_free);\n \treturn 0;\n }\n \ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex d90e5d2..b01d76a 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)\n \n \tif (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {\n \t\tunsigned char unused1[20];\n-\t\tconst char *symref;\n-\t\tsymref = resolve_ref(ref->refname, unused1, 1, NULL);\n-\t\tif (symref)\n-\t\t\tref->symref = xstrdup(symref);\n-\t\telse\n+\t\tref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);\n+\t\tif (!ref->symref)\n \t\t\tref->symref = \"\";\n \t}\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a1c8534..d4079e6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n+\tchar *branch_to_free;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n@@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n \t * current branch.\n \t */\n-\tbranch = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n-\tif (branch) {\n-\t\tif (!prefixcmp(branch, \"refs/heads/\"))\n-\t\t\tbranch += 11;\n-\t\tbranch = xstrdup(branch);\n-\t}\n+\tbranch = branch_to_free = resolve_refdup(\"HEAD\", head_sha1, 0, &flag);\n+\tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n+\t\tbranch += 11;\n \tif (!branch || is_null_sha1(head_sha1))\n \t\thead_commit = NULL;\n \telse\n@@ -1520,6 +1518,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tret = suggest_conflicts(option_renormalize);\n \n done:\n-\tfree((char *)branch);\n+\tfree(branch_to_free);\n \treturn ret;\n }\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 10b8bc7..11a24d7 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -804,6 +804,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tstruct notes_tree *t;\n \tstruct commit *partial;\n \tstruct pretty_print_context pretty_ctx;\n+\tchar *local_ref_to_free;\n \tint ret;\n \n \t/*\n@@ -826,10 +827,10 @@ static int merge_commit(struct notes_merge_options *o)\n \tt = xcalloc(1, sizeof(struct notes_tree));\n \tinit_notes(t, \"NOTES_MERGE_PARTIAL\", combine_notes_overwrite, 0);\n \n-\to->local_ref = resolve_ref(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n+\to->local_ref = local_ref_to_free =\n+\t\tresolve_refdup(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n \tif (!o->local_ref)\n \t\tdie(\"Failed to resolve NOTES_MERGE_REF\");\n-\to->local_ref = xstrdup(o->local_ref);\n \n \tif (notes_merge_commit(o, t, partial, sha1))\n \t\tdie(\"Failed to finalize notes merge\");\n@@ -846,7 +847,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tfree_notes(t);\n \tstrbuf_release(&msg);\n \tret = merge_abort(o);\n-\tfree((char *)o->local_ref);\n+\tfree(local_ref_to_free);\n \treturn ret;\n }\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex b6d957c..fa251ec 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -37,6 +37,7 @@ static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n static int auto_gc = 1;\n static const char *head_name;\n+static char *head_name_to_free;\n static int sent_capabilities;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n@@ -695,10 +696,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tcheck_aliased_updates(commands);\n \n-\tfree((char *)head_name);\n-\thead_name = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\tif (head_name)\n-\t\thead_name = xstrdup(head_name);\n+\tfree(head_name_to_free);\n+\thead_name = head_name_to_free = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n \n \tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\tif (!cmd->skip_update)\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 4b480d7..a1f148e 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -726,10 +726,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \n \t\tif (ac == 0) {\n \t\t\tstatic const char *fake_av[2];\n-\t\t\tconst char *refname;\n \n-\t\t\trefname = resolve_ref(\"HEAD\", sha1, 1, NULL);\n-\t\t\tfake_av[0] = xstrdup(refname);\n+\t\t\tfake_av[0] = resolve_refdup(\"HEAD\", sha1, 1, NULL);\n \t\t\tfake_av[1] = NULL;\n \t\t\tav = fake_av;\n \t\t\tac = 1;\ndiff --git a/cache.h b/cache.h\nindex 8c98d05..4887a3e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -866,6 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n  * errno is sometimes set on errors, but not always.\n  */\n extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n+extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex da71a85..52eddb7 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -50,11 +50,12 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)\n \tfor_each_reflog_ent(ref, read_one_reflog, reflogs);\n \tif (reflogs->nr == 0) {\n \t\tunsigned char sha1[20];\n-\t\tconst char *name = resolve_ref(ref, sha1, 1, NULL);\n+\t\tconst char *name;\n+\t\tchar *name_to_free;\n+\t\tname = name_to_free = resolve_refdup(ref, sha1, 1, NULL);\n \t\tif (name) {\n-\t\t\tname = xstrdup(name);\n \t\t\tfor_each_reflog_ent(name, read_one_reflog, reflogs);\n-\t\t\tfree((char *)name);\n+\t\t\tfree(name_to_free);\n \t\t}\n \t}\n \tif (reflogs->nr == 0) {\n@@ -171,11 +172,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \telse {\n \t\tif (*branch == '\\0') {\n \t\t\tunsigned char sha1[20];\n-\t\t\tconst char *head = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\t\t\tif (!head)\n-\t\t\t\tdie (\"No current branch\");\n \t\t\tfree(branch);\n-\t\t\tbranch = xstrdup(head);\n+\t\t\tbranch = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n+\t\t\tif (!branch)\n+\t\t\t\tdie (\"No current branch\");\n+\n \t\t}\n \t\treflogs = read_complete_reflog(branch);\n \t\tif (!reflogs || reflogs->nr == 0) {\ndiff --git a/refs.c b/refs.c\nindex f5cb297..8ffb32f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -605,6 +605,12 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \treturn ref;\n }\n \n+char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n+{\n+\tconst char *ret = resolve_ref(ref, sha1, reading, flag);\n+\treturn ret ? xstrdup(ret) : NULL;\n+}\n+\n /* The argument to filter_refs */\n struct ref_filter {\n \tconst char *pattern;\ndiff --git a/wt-status.c b/wt-status.c\nindex 70fdb76..9ffc535 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -111,7 +111,6 @@ void status_printf_more(struct wt_status *s, const char *color,\n void wt_status_prepare(struct wt_status *s)\n {\n \tunsigned char sha1[20];\n-\tconst char *head;\n \n \tmemset(s, 0, sizeof(*s));\n \tmemcpy(s->color_palette, default_wt_status_colors,\n@@ -119,8 +118,7 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;\n \ts->use_color = -1;\n \ts->relative_paths = 1;\n-\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\ts->branch = head ? xstrdup(head) : NULL;\n+\ts->branch = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n \ts->reference = \"HEAD\";\n \ts->fp = stdout;\n \ts->index_file = get_index_file();\n-- \n1.7.8.36.g69ee2\n"},{"id":"181037","messageId":"4EE75C88.5060700@alum.mit.edu","threadId":"29137","inReplyTo":"7vpqft4qap.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/4] Convert resolve_ref+xstrdup to new resolve_refdup function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-12-13T14:09:12Z","receivedAt":"2011-12-13T14:09:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/12/2011 07:07 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n> \n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> \n> Thanks.\n> \n> The patch looks correct but I have a slight maintainability concern and a\n> suggestion for possible improvement.\n> \n>> ...\n>> diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> index b7c6302..a66d3eb 100644\n>> --- a/builtin/checkout.c\n>> +++ b/builtin/checkout.c\n>> @@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>>  {\n>>  \tint ret = 0;\n>>  \tstruct branch_info old;\n>> +\tchar *path;\n>>  \tunsigned char rev[20];\n>>  \tint flag;\n>>  \tmemset(&old, 0, sizeof(old));\n>> -\told.path = resolve_ref(\"HEAD\", rev, 0, &flag);\n>> -\tif (old.path)\n>> -\t\told.path = xstrdup(old.path);\n>> +\told.path = path = resolve_refdup(\"HEAD\", rev, 0, &flag);\n> \n> This uses \"one 'const char *' pointer that is used for reading data from\n> and an extra 'char *' pointer that is used only for freeing\" approach,\n> which has two advantages and one disadvantage:\n> [...]\n> When naming a \"for-freeing\" pointer variable, the kind of data the area of\n> memory happens to contain is of secondary importance. Being deliberately\n> vague about what the area of memory may contain is a good thing, because\n> it actively discourages the program from looking at the area via the\n> pointer if the variable is named \"to_free\" or something that does not\n> specify what it contains.\n\nThe to_free variable could even be declared void* to make it even less\n(ab)usable.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"181041","messageId":"1323785868-16601-1-git-send-email-pclouds@gmail.com","threadId":"29137","inReplyTo":"4EE75C88.5060700@alum.mit.edu","subject":"[PATCH] Convert resolve_ref+xstrdup to new resolve_refdup function","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-13T14:17:48Z","receivedAt":"2011-12-13T14:17:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n 2011/12/13 Michael Haggerty <mhagger@alum.mit.edu>:\n >> This uses \"one 'const char *' pointer that is used for reading data from\n >> and an extra 'char *' pointer that is used only for freeing\" approach,\n >> which has two advantages and one disadvantage:\n >> [...]\n >> When naming a \"for-freeing\" pointer variable, the kind of data the area of\n >> memory happens to contain is of secondary importance. Being deliberately\n >> vague about what the area of memory may contain is a good thing, because\n >> it actively discourages the program from looking at the area via the\n >> pointer if the variable is named \"to_free\" or something that does not\n >> specify what it contains.\n >\n > The to_free variable could even be declared void* to make it even less\n > (ab)usable.\n\n Good thinking. Here's the version with \"char *foo_to_free\" converted\n to \"void *foo_to_free\".\n\n builtin/branch.c        |   12 +++++-------\n builtin/checkout.c      |   15 +++++++--------\n builtin/fmt-merge-msg.c |    7 ++++---\n builtin/for-each-ref.c  |    7 ++-----\n builtin/merge.c         |   12 +++++-------\n builtin/notes.c         |    7 ++++---\n builtin/receive-pack.c  |    7 +++----\n builtin/show-branch.c   |    4 +---\n cache.h                 |    1 +\n reflog-walk.c           |   15 ++++++++-------\n refs.c                  |    6 ++++++\n wt-status.c             |    4 +---\n 12 files changed, 47 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex e1e486e..c459f0b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -104,6 +104,7 @@ static int branch_merged(int kind, const char *name,\n \t */\n \tstruct commit *reference_rev = NULL;\n \tconst char *reference_name = NULL;\n+\tvoid *reference_name_to_free = NULL;\n \tint merged;\n \n \tif (kind == REF_LOCAL_BRANCH) {\n@@ -114,11 +115,9 @@ static int branch_merged(int kind, const char *name,\n \t\t    branch->merge &&\n \t\t    branch->merge[0] &&\n \t\t    branch->merge[0]->dst &&\n-\t\t    (reference_name =\n-\t\t     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {\n-\t\t\treference_name = xstrdup(reference_name);\n+\t\t    (reference_name = reference_name_to_free =\n+\t\t     resolve_refdup(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n \t\t\treference_rev = lookup_commit_reference(sha1);\n-\t\t}\n \t}\n \tif (!reference_rev)\n \t\treference_rev = head_rev;\n@@ -143,7 +142,7 @@ static int branch_merged(int kind, const char *name,\n \t\t\t\t\"         '%s', even though it is merged to HEAD.\"),\n \t\t\t\tname, reference_name);\n \t}\n-\tfree((char *)reference_name);\n+\tfree(reference_name_to_free);\n \treturn merged;\n }\n \n@@ -731,10 +730,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \ttrack = git_branch_track;\n \n-\thead = resolve_ref(\"HEAD\", head_sha1, 0, NULL);\n+\thead = resolve_refdup(\"HEAD\", head_sha1, 0, NULL);\n \tif (!head)\n \t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n-\thead = xstrdup(head);\n \tif (!strcmp(head, \"HEAD\")) {\n \t\tdetached = 1;\n \t} else {\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex b7c6302..00740bd 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n {\n \tint ret = 0;\n \tstruct branch_info old;\n+\tvoid *path_to_free;\n \tunsigned char rev[20];\n \tint flag;\n \tmemset(&old, 0, sizeof(old));\n-\told.path = resolve_ref(\"HEAD\", rev, 0, &flag);\n-\tif (old.path)\n-\t\told.path = xstrdup(old.path);\n+\told.path = path_to_free = resolve_refdup(\"HEAD\", rev, 0, &flag);\n \told.commit = lookup_commit_reference_gently(rev, 1);\n-\tif (!(flag & REF_ISSYMREF)) {\n-\t\tfree((char *)old.path);\n+\tif (!(flag & REF_ISSYMREF))\n \t\told.path = NULL;\n-\t}\n \n \tif (old.path && !prefixcmp(old.path, \"refs/heads/\"))\n \t\told.name = old.path + strlen(\"refs/heads/\");\n@@ -720,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t}\n \n \tret = merge_working_tree(opts, &old, new);\n-\tif (ret)\n+\tif (ret) {\n+\t\tfree(path_to_free);\n \t\treturn ret;\n+\t}\n \n \tif (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n \t\torphaned_commit_warning(old.commit);\n@@ -729,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \tupdate_refs_for_switch(opts, &old, new);\n \n \tret = post_checkout_hook(old.commit, new->commit, 1);\n-\tfree((char *)old.path);\n+\tfree(path_to_free);\n \treturn ret || opts->writeout_error;\n }\n \ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex bdfa0ea..c81a7fe 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -372,14 +372,15 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \tint i = 0, pos = 0;\n \tunsigned char head_sha1[20];\n \tconst char *current_branch;\n+\tvoid *current_branch_to_free;\n \n \t/* get current branch */\n-\tcurrent_branch = resolve_ref(\"HEAD\", head_sha1, 1, NULL);\n+\tcurrent_branch = current_branch_to_free =\n+\t\tresolve_refdup(\"HEAD\", head_sha1, 1, NULL);\n \tif (!current_branch)\n \t\tdie(\"No current branch\");\n \tif (!prefixcmp(current_branch, \"refs/heads/\"))\n \t\tcurrent_branch += 11;\n-\tcurrent_branch = xstrdup(current_branch);\n \n \t/* get a line */\n \twhile (pos < in->len) {\n@@ -421,7 +422,7 @@ int fmt_merge_msg(struct strbuf *in, struct strbuf *out,\n \t}\n \n \tstrbuf_complete_line(out);\n-\tfree((char *)current_branch);\n+\tfree(current_branch_to_free);\n \treturn 0;\n }\n \ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex d90e5d2..b01d76a 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)\n \n \tif (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {\n \t\tunsigned char unused1[20];\n-\t\tconst char *symref;\n-\t\tsymref = resolve_ref(ref->refname, unused1, 1, NULL);\n-\t\tif (symref)\n-\t\t\tref->symref = xstrdup(symref);\n-\t\telse\n+\t\tref->symref = resolve_refdup(ref->refname, unused1, 1, NULL);\n+\t\tif (!ref->symref)\n \t\t\tref->symref = \"\";\n \t}\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a1c8534..a3cd5e8 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1096,6 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n+\tvoid *branch_to_free;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n@@ -1104,12 +1105,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n \t * current branch.\n \t */\n-\tbranch = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n-\tif (branch) {\n-\t\tif (!prefixcmp(branch, \"refs/heads/\"))\n-\t\t\tbranch += 11;\n-\t\tbranch = xstrdup(branch);\n-\t}\n+\tbranch = branch_to_free = resolve_refdup(\"HEAD\", head_sha1, 0, &flag);\n+\tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n+\t\tbranch += 11;\n \tif (!branch || is_null_sha1(head_sha1))\n \t\thead_commit = NULL;\n \telse\n@@ -1520,6 +1518,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tret = suggest_conflicts(option_renormalize);\n \n done:\n-\tfree((char *)branch);\n+\tfree(branch_to_free);\n \treturn ret;\n }\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 10b8bc7..667e20a 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -804,6 +804,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tstruct notes_tree *t;\n \tstruct commit *partial;\n \tstruct pretty_print_context pretty_ctx;\n+\tvoid *local_ref_to_free;\n \tint ret;\n \n \t/*\n@@ -826,10 +827,10 @@ static int merge_commit(struct notes_merge_options *o)\n \tt = xcalloc(1, sizeof(struct notes_tree));\n \tinit_notes(t, \"NOTES_MERGE_PARTIAL\", combine_notes_overwrite, 0);\n \n-\to->local_ref = resolve_ref(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n+\to->local_ref = local_ref_to_free =\n+\t\tresolve_refdup(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n \tif (!o->local_ref)\n \t\tdie(\"Failed to resolve NOTES_MERGE_REF\");\n-\to->local_ref = xstrdup(o->local_ref);\n \n \tif (notes_merge_commit(o, t, partial, sha1))\n \t\tdie(\"Failed to finalize notes merge\");\n@@ -846,7 +847,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tfree_notes(t);\n \tstrbuf_release(&msg);\n \tret = merge_abort(o);\n-\tfree((char *)o->local_ref);\n+\tfree(local_ref_to_free);\n \treturn ret;\n }\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex b6d957c..62afac3 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -37,6 +37,7 @@ static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n static int auto_gc = 1;\n static const char *head_name;\n+static void *head_name_to_free;\n static int sent_capabilities;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n@@ -695,10 +696,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tcheck_aliased_updates(commands);\n \n-\tfree((char *)head_name);\n-\thead_name = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\tif (head_name)\n-\t\thead_name = xstrdup(head_name);\n+\tfree(head_name_to_free);\n+\thead_name = head_name_to_free = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n \n \tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\tif (!cmd->skip_update)\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 4b480d7..a1f148e 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -726,10 +726,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \n \t\tif (ac == 0) {\n \t\t\tstatic const char *fake_av[2];\n-\t\t\tconst char *refname;\n \n-\t\t\trefname = resolve_ref(\"HEAD\", sha1, 1, NULL);\n-\t\t\tfake_av[0] = xstrdup(refname);\n+\t\t\tfake_av[0] = resolve_refdup(\"HEAD\", sha1, 1, NULL);\n \t\t\tfake_av[1] = NULL;\n \t\t\tav = fake_av;\n \t\t\tac = 1;\ndiff --git a/cache.h b/cache.h\nindex 8c98d05..4887a3e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -866,6 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n  * errno is sometimes set on errors, but not always.\n  */\n extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n+extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex da71a85..64c677f 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -50,11 +50,12 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)\n \tfor_each_reflog_ent(ref, read_one_reflog, reflogs);\n \tif (reflogs->nr == 0) {\n \t\tunsigned char sha1[20];\n-\t\tconst char *name = resolve_ref(ref, sha1, 1, NULL);\n+\t\tconst char *name;\n+\t\tvoid *name_to_free;\n+\t\tname = name_to_free = resolve_refdup(ref, sha1, 1, NULL);\n \t\tif (name) {\n-\t\t\tname = xstrdup(name);\n \t\t\tfor_each_reflog_ent(name, read_one_reflog, reflogs);\n-\t\t\tfree((char *)name);\n+\t\t\tfree(name_to_free);\n \t\t}\n \t}\n \tif (reflogs->nr == 0) {\n@@ -171,11 +172,11 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \telse {\n \t\tif (*branch == '\\0') {\n \t\t\tunsigned char sha1[20];\n-\t\t\tconst char *head = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\t\t\tif (!head)\n-\t\t\t\tdie (\"No current branch\");\n \t\t\tfree(branch);\n-\t\t\tbranch = xstrdup(head);\n+\t\t\tbranch = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n+\t\t\tif (!branch)\n+\t\t\t\tdie (\"No current branch\");\n+\n \t\t}\n \t\treflogs = read_complete_reflog(branch);\n \t\tif (!reflogs || reflogs->nr == 0) {\ndiff --git a/refs.c b/refs.c\nindex f5cb297..8ffb32f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -605,6 +605,12 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *\n \treturn ref;\n }\n \n+char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n+{\n+\tconst char *ret = resolve_ref(ref, sha1, reading, flag);\n+\treturn ret ? xstrdup(ret) : NULL;\n+}\n+\n /* The argument to filter_refs */\n struct ref_filter {\n \tconst char *pattern;\ndiff --git a/wt-status.c b/wt-status.c\nindex 70fdb76..9ffc535 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -111,7 +111,6 @@ void status_printf_more(struct wt_status *s, const char *color,\n void wt_status_prepare(struct wt_status *s)\n {\n \tunsigned char sha1[20];\n-\tconst char *head;\n \n \tmemset(s, 0, sizeof(*s));\n \tmemcpy(s->color_palette, default_wt_status_colors,\n@@ -119,8 +118,7 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;\n \ts->use_color = -1;\n \ts->relative_paths = 1;\n-\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\ts->branch = head ? xstrdup(head) : NULL;\n+\ts->branch = resolve_refdup(\"HEAD\", sha1, 0, NULL);\n \ts->reference = \"HEAD\";\n \ts->fp = stdout;\n \ts->index_file = get_index_file();\n-- \n1.7.8.36.g69ee2\n"}]}