From 5b3b8fa2ab188908332358d2c2576491969cd580 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Tue, 4 Sep 2012 18:26:30 +0100 Subject: [PATCH 1/5] path.c: Remove the 'git_' prefix from a file scope function In particular, the git_vsnpath() function, despite the 'git_' prefix suggesting otherwise, is (correctly) declared with file scope. Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- path.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/path.c b/path.c index 66acd24fa..9eb5333ee 100644 --- a/path.c +++ b/path.c @@ -48,7 +48,7 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...) return cleanup_path(buf); } -static char *git_vsnpath(char *buf, size_t n, const char *fmt, va_list args) +static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args) { const char *git_dir = get_git_dir(); size_t len; @@ -72,7 +72,7 @@ char *git_snpath(char *buf, size_t n, const char *fmt, ...) { va_list args; va_start(args, fmt); - (void)git_vsnpath(buf, n, fmt, args); + (void)vsnpath(buf, n, fmt, args); va_end(args); return buf; } @@ -82,7 +82,7 @@ char *git_pathdup(const char *fmt, ...) char path[PATH_MAX]; va_list args; va_start(args, fmt); - (void)git_vsnpath(path, sizeof(path), fmt, args); + (void)vsnpath(path, sizeof(path), fmt, args); va_end(args); return xstrdup(path); } From 66a51a9aaec3f53250b1d515a1bd81044ebb354b Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Tue, 4 Sep 2012 18:27:54 +0100 Subject: [PATCH 2/5] path.c: Don't discard the return value of vsnpath() The git_snpath() and git_pathdup() functions both use the (static) function vsnpath() in their implementation. Also, they both discard the return value of vsnpath(), which has the effect of ignoring the side effect of calling cleanup_path() in the non-error return path. In order to ensure that the required cleanup happens, we use the pointer returned by vsnpath(), rather than the buffer passed into vsnpath(), to derive the return value from git_snpath() and git_pathdup(). Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- path.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/path.c b/path.c index 9eb5333ee..741ae77ac 100644 --- a/path.c +++ b/path.c @@ -70,21 +70,22 @@ static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args) char *git_snpath(char *buf, size_t n, const char *fmt, ...) { + char *ret; va_list args; va_start(args, fmt); - (void)vsnpath(buf, n, fmt, args); + ret = vsnpath(buf, n, fmt, args); va_end(args); - return buf; + return ret; } char *git_pathdup(const char *fmt, ...) { - char path[PATH_MAX]; + char path[PATH_MAX], *ret; va_list args; va_start(args, fmt); - (void)vsnpath(path, sizeof(path), fmt, args); + ret = vsnpath(path, sizeof(path), fmt, args); va_end(args); - return xstrdup(path); + return xstrdup(ret); } char *mkpathdup(const char *fmt, ...) From 5c44252e13db2324d7c79d2b498886a925055111 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Tue, 4 Sep 2012 18:29:22 +0100 Subject: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path() The current implementation of git_path() is essentially the same as that of vsnpath(), with two minor differences. First, git_path() currently insists that the git directory path is no longer than PATH_MAX-100 characters in length. However, vsnpath() does not attempt this arbitrary 100 character reservation for the remaining path components. Second, vsnpath() uses the "is_dir_sep()" macro, rather than comparing directly to '/', to determine if the git_dir path component ends with a path separator. In order to benefit from the above improvements, along with increased compatability with git_snpath() and git_pathdup(), we reimplement the git_path() function using vsnpath(). Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- path.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/path.c b/path.c index 741ae77ac..cbbdf7d6b 100644 --- a/path.c +++ b/path.c @@ -119,23 +119,14 @@ char *mkpath(const char *fmt, ...) char *git_path(const char *fmt, ...) { - const char *git_dir = get_git_dir(); char *pathname = get_pathname(); va_list args; - unsigned len; + char *ret; - len = strlen(git_dir); - if (len > PATH_MAX-100) - return bad_path; - memcpy(pathname, git_dir, len); - if (len && git_dir[len-1] != '/') - pathname[len++] = '/'; va_start(args, fmt); - len += vsnprintf(pathname + len, PATH_MAX - len, fmt, args); + ret = vsnpath(pathname, PATH_MAX, fmt, args); va_end(args); - if (len >= PATH_MAX) - return bad_path; - return cleanup_path(pathname); + return ret; } void home_config_paths(char **global, char **xdg, char *file) From d292bfaf356338b41e14e40ce4dbd6b9c8d600ec Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Tue, 4 Sep 2012 18:30:21 +0100 Subject: [PATCH 4/5] Call git_pathdup() rather than xstrdup(git_path("...")) In addition to updating the two xstrdup(git_path("...")) call sites with git_pathdup(), we also fix a memory leak by freeing the memory allocated to the ADD_EDIT.patch 'file' in the edit_patch() function. Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- bisect.c | 2 +- builtin/add.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/bisect.c b/bisect.c index 48acf7339..1aad49b1a 100644 --- a/bisect.c +++ b/bisect.c @@ -833,7 +833,7 @@ static int check_ancestors(const char *prefix) */ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout) { - char *filename = xstrdup(git_path("BISECT_ANCESTORS_OK")); + char *filename = git_pathdup("BISECT_ANCESTORS_OK"); struct stat st; int fd; diff --git a/builtin/add.c b/builtin/add.c index 89dce56a2..2fc267742 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -260,7 +260,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch) static int edit_patch(int argc, const char **argv, const char *prefix) { - char *file = xstrdup(git_path("ADD_EDIT.patch")); + char *file = git_pathdup("ADD_EDIT.patch"); const char *apply_argv[] = { "apply", "--recount", "--cached", NULL, NULL }; struct child_process child; @@ -303,6 +303,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix) die (_("Could not apply '%s'"), file); unlink(file); + free(file); return 0; } From 4e2d094dde4f078245d057dd6111ab9d013ae6d0 Mon Sep 17 00:00:00 2001 From: Ramsay Jones Date: Tue, 4 Sep 2012 18:31:14 +0100 Subject: [PATCH 5/5] Call mkpathdup() rather than xstrdup(mkpath(...)) In addition to updating the xstrdup(mkpath(...)) call sites with mkpathdup(), we also fix a memory leak (in merge_3way()) caused by neglecting to free the memory allocated to the 'base_name' variable. Signed-off-by: Ramsay Jones Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- builtin/clone.c | 4 ++-- builtin/prune.c | 2 +- merge-recursive.c | 13 +++++++------ 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 0e060f2e4..bdf8495aa 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -196,7 +196,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, free(name); - name = xstrdup(mkpath(fmt, bname.buf)); + name = mkpathdup(fmt, bname.buf); if (read_ref(name, sha1)) { error(remote_branch ? _("remote branch '%s' not found.") diff --git a/builtin/clone.c b/builtin/clone.c index e314b0b6d..c819757b3 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -236,7 +236,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data) /* Beware: real_path() and mkpath() return static buffer */ ref_git = xstrdup(real_path(item->string)); if (is_directory(mkpath("%s/.git/objects", ref_git))) { - char *ref_git_git = xstrdup(mkpath("%s/.git", ref_git)); + char *ref_git_git = mkpathdup("%s/.git", ref_git); free(ref_git); ref_git = ref_git_git; } else if (!is_directory(mkpath("%s/objects", ref_git))) @@ -700,7 +700,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) git_dir = xstrdup(dir); else { work_tree = dir; - git_dir = xstrdup(mkpath("%s/.git", dir)); + git_dir = mkpathdup("%s/.git", dir); } if (!option_bare) { diff --git a/builtin/prune.c b/builtin/prune.c index b99b635e4..f66ff676e 100644 --- a/builtin/prune.c +++ b/builtin/prune.c @@ -168,7 +168,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix) prune_packed_objects(show_only); remove_temporary_files(get_object_directory()); - s = xstrdup(mkpath("%s/pack", get_object_directory())); + s = mkpathdup("%s/pack", get_object_directory()); remove_temporary_files(s); free(s); return 0; diff --git a/merge-recursive.c b/merge-recursive.c index 39b2e165e..2f8febe0e 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -862,14 +862,14 @@ static int merge_3way(struct merge_options *o, if (strcmp(a->path, b->path) || (o->ancestor != NULL && strcmp(a->path, one->path) != 0)) { base_name = o->ancestor == NULL ? NULL : - xstrdup(mkpath("%s:%s", o->ancestor, one->path)); - name1 = xstrdup(mkpath("%s:%s", branch1, a->path)); - name2 = xstrdup(mkpath("%s:%s", branch2, b->path)); + mkpathdup("%s:%s", o->ancestor, one->path); + name1 = mkpathdup("%s:%s", branch1, a->path); + name2 = mkpathdup("%s:%s", branch2, b->path); } else { base_name = o->ancestor == NULL ? NULL : - xstrdup(mkpath("%s", o->ancestor)); - name1 = xstrdup(mkpath("%s", branch1)); - name2 = xstrdup(mkpath("%s", branch2)); + mkpathdup("%s", o->ancestor); + name1 = mkpathdup("%s", branch1); + name2 = mkpathdup("%s", branch2); } read_mmblob(&orig, one->sha1); @@ -879,6 +879,7 @@ static int merge_3way(struct merge_options *o, merge_status = ll_merge(result_buf, a->path, &orig, base_name, &src1, name1, &src2, name2, &ll_opts); + free(base_name); free(name1); free(name2); free(orig.ptr);