From 30a0ddb705678d512185e359831479a6b3567147 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 16:01:34 -0400 Subject: [PATCH 01/11] strbuf: add xstrfmt helper You can use a strbuf to build up a string from parts, and then detach it. In the general case, you might use multiple strbuf_add* functions to do the building. However, in many cases, a single strbuf_addf is sufficient, and we end up with: struct strbuf buf = STRBUF_INIT; ... strbuf_addf(&buf, fmt, some, args); str = strbuf_detach(&buf, NULL); We can make this much more readable (and avoid introducing an extra variable, which can clutter the code) by introducing a convenience function: str = xstrfmt(fmt, some, args); Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- strbuf.c | 19 +++++++++++++++++++ strbuf.h | 9 +++++++++ 2 files changed, 28 insertions(+) diff --git a/strbuf.c b/strbuf.c index ac62982e67..12c78656ca 100644 --- a/strbuf.c +++ b/strbuf.c @@ -600,3 +600,22 @@ char *xstrdup_tolower(const char *string) result[i] = '\0'; return result; } + +char *xstrvfmt(const char *fmt, va_list ap) +{ + struct strbuf buf = STRBUF_INIT; + strbuf_vaddf(&buf, fmt, ap); + return strbuf_detach(&buf, NULL); +} + +char *xstrfmt(const char *fmt, ...) +{ + va_list ap; + char *ret; + + va_start(ap, fmt); + ret = xstrvfmt(fmt, ap); + va_end(ap); + + return ret; +} diff --git a/strbuf.h b/strbuf.h index e9ad03eabe..a594c24b2b 100644 --- a/strbuf.h +++ b/strbuf.h @@ -187,4 +187,13 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...); char *xstrdup_tolower(const char *); +/* + * Create a newly allocated string using printf format. You can do this easily + * with a strbuf, but this provides a shortcut to save a few lines. + */ +__attribute__((format (printf, 1, 0))) +char *xstrvfmt(const char *fmt, va_list ap); +__attribute__((format (printf, 1, 2))) +char *xstrfmt(const char *fmt, ...); + #endif /* STRBUF_H */ From fa3f60b783b42e0d07c667a8f582c3df12791cec Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 18 Jun 2014 16:02:13 -0400 Subject: [PATCH 02/11] use xstrfmt in favor of manual size calculations In many parts of the code, we do an ugly and error-prone malloc like: const char *fmt = "something %s"; buf = xmalloc(strlen(foo) + 10 + 1); sprintf(buf, fmt, foo); This makes the code brittle, and if we ever get the allocation wrong, is a potential heap overflow. Let's instead favor xstrfmt, which handles the allocation automatically, and makes the code shorter and more readable. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- remote.c | 6 +----- unpack-trees.c | 17 ++++++----------- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/remote.c b/remote.c index 0e9459cc06..bf27e44762 100644 --- a/remote.c +++ b/remote.c @@ -170,7 +170,6 @@ static struct branch *make_branch(const char *name, int len) { struct branch *ret; int i; - char *refname; for (i = 0; i < branches_nr; i++) { if (len ? (!strncmp(name, branches[i]->name, len) && @@ -186,10 +185,7 @@ static struct branch *make_branch(const char *name, int len) ret->name = xstrndup(name, len); else ret->name = xstrdup(name); - refname = xmalloc(strlen(name) + strlen("refs/heads/") + 1); - strcpy(refname, "refs/heads/"); - strcpy(refname + strlen("refs/heads/"), ret->name); - ret->refname = refname; + ret->refname = xstrfmt("refs/heads/%s", ret->name); return ret; } diff --git a/unpack-trees.c b/unpack-trees.c index 97fc995467..c237370b1f 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -56,17 +56,15 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, int i; const char **msgs = opts->msgs; const char *msg; - char *tmp; const char *cmd2 = strcmp(cmd, "checkout") ? cmd : "switch branches"; + if (advice_commit_before_merge) msg = "Your local changes to the following files would be overwritten by %s:\n%%s" "Please, commit your changes or stash them before you can %s."; else msg = "Your local changes to the following files would be overwritten by %s:\n%%s"; - tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen(cmd2) - 2); - sprintf(tmp, msg, cmd, cmd2); - msgs[ERROR_WOULD_OVERWRITE] = tmp; - msgs[ERROR_NOT_UPTODATE_FILE] = tmp; + msgs[ERROR_WOULD_OVERWRITE] = msgs[ERROR_NOT_UPTODATE_FILE] = + xstrfmt(msg, cmd, cmd2); msgs[ERROR_NOT_UPTODATE_DIR] = "Updating the following directories would lose untracked files in it:\n%s"; @@ -76,12 +74,9 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts, "Please move or remove them before you can %s."; else msg = "The following untracked working tree files would be %s by %s:\n%%s"; - tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("removed") + strlen(cmd2) - 4); - sprintf(tmp, msg, "removed", cmd, cmd2); - msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = tmp; - tmp = xmalloc(strlen(msg) + strlen(cmd) + strlen("overwritten") + strlen(cmd2) - 4); - sprintf(tmp, msg, "overwritten", cmd, cmd2); - msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = tmp; + + msgs[ERROR_WOULD_LOSE_UNTRACKED_REMOVED] = xstrfmt(msg, "removed", cmd, cmd2); + msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrfmt(msg, "overwritten", cmd, cmd2); /* * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we From 95244ae3dd49b3eed4dbfe09299b9d8847622218 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:19:43 -0400 Subject: [PATCH 03/11] use xstrdup instead of xmalloc + strcpy This is one line shorter, and makes sure the length in the malloc and copy steps match. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/receive-pack.c | 5 +---- http-push.c | 6 ++---- http-walker.c | 3 +-- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c3230817db..18458e81c6 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -614,12 +614,9 @@ static void run_update_post_hook(struct command *commands) argv[0] = hook; for (argc = 1, cmd = commands; cmd; cmd = cmd->next) { - char *p; if (cmd->error_string || cmd->did_not_exist) continue; - p = xmalloc(strlen(cmd->ref_name) + 1); - strcpy(p, cmd->ref_name); - argv[argc] = p; + argv[argc] = xstrdup(cmd->ref_name); argc++; } argv[argc] = NULL; diff --git a/http-push.c b/http-push.c index de00d1693a..95650a0241 100644 --- a/http-push.c +++ b/http-push.c @@ -767,15 +767,13 @@ static void handle_new_lock_ctx(struct xml_ctx *ctx, int tag_closed) if (tag_closed && ctx->cdata) { if (!strcmp(ctx->name, DAV_ACTIVELOCK_OWNER)) { - lock->owner = xmalloc(strlen(ctx->cdata) + 1); - strcpy(lock->owner, ctx->cdata); + lock->owner = xstrdup(ctx->cdata); } else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TIMEOUT)) { if (starts_with(ctx->cdata, "Second-")) lock->timeout = strtol(ctx->cdata + 7, NULL, 10); } else if (!strcmp(ctx->name, DAV_ACTIVELOCK_TOKEN)) { - lock->token = xmalloc(strlen(ctx->cdata) + 1); - strcpy(lock->token, ctx->cdata); + lock->token = xstrdup(ctx->cdata); git_SHA1_Init(&sha_ctx); git_SHA1_Update(&sha_ctx, lock->token, strlen(lock->token)); diff --git a/http-walker.c b/http-walker.c index 1516c5eb29..ab6a4febeb 100644 --- a/http-walker.c +++ b/http-walker.c @@ -566,8 +566,7 @@ struct walker *get_http_walker(const char *url) struct walker *walker = xmalloc(sizeof(struct walker)); data->alt = xmalloc(sizeof(*data->alt)); - data->alt->base = xmalloc(strlen(url) + 1); - strcpy(data->alt->base, url); + data->alt->base = xstrdup(url); for (s = data->alt->base + strlen(data->alt->base) - 1; *s == '/'; --s) *s = 0; From 283101869bea8feb5d58f6ea1b568e9b197526d3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:24:33 -0400 Subject: [PATCH 04/11] use xstrfmt to replace xmalloc + sprintf This is one line shorter, and makes sure the length in the malloc and sprintf steps match. These conversions are very straightforward; we can drop the malloc entirely, and replace the sprintf with xstrfmt. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fmt-merge-msg.c | 7 ++----- builtin/show-branch.c | 10 ++++------ http-push.c | 18 +++++------------- http-walker.c | 3 +-- match-trees.c | 9 ++------- merge-recursive.c | 12 ++++-------- 6 files changed, 18 insertions(+), 41 deletions(-) diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 3906eda877..c462e19e23 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -178,11 +178,8 @@ static int handle_line(char *line, struct merge_parents *merge_parents) int len = strlen(origin); if (origin[0] == '\'' && origin[len - 1] == '\'') origin = xmemdupz(origin + 1, len - 2); - } else { - char *new_origin = xmalloc(strlen(origin) + strlen(src) + 5); - sprintf(new_origin, "%s of %s", origin, src); - origin = new_origin; - } + } else + origin = xstrfmt("%s of %s", origin, src); if (strcmp(".", src)) origin_data->is_local_branch = 0; string_list_append(&origins, origin)->util = origin_data; diff --git a/builtin/show-branch.c b/builtin/show-branch.c index d87317290c..5fd4e4e488 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -755,7 +755,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) } for (i = 0; i < reflog; i++) { - char *logmsg, *m; + char *logmsg; const char *msg; unsigned long timestamp; int tz; @@ -770,11 +770,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) msg = "(none)"; else msg++; - m = xmalloc(strlen(msg) + 200); - sprintf(m, "(%s) %s", - show_date(timestamp, tz, 1), - msg); - reflog_msg[i] = m; + reflog_msg[i] = xstrfmt("(%s) %s", + show_date(timestamp, tz, 1), + msg); free(logmsg); sprintf(nth_desc, "%s@{%d}", *av, base+i); append_ref(nth_desc, sha1, 1); diff --git a/http-push.c b/http-push.c index 95650a0241..390f74c97b 100644 --- a/http-push.c +++ b/http-push.c @@ -854,8 +854,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout) struct xml_ctx ctx; char *escaped; - url = xmalloc(strlen(repo->url) + strlen(path) + 1); - sprintf(url, "%s%s", repo->url, path); + url = xstrfmt("%s%s", repo->url, path); /* Make sure leading directories exist for the remote ref */ ep = strchr(url + strlen(repo->url) + 1, '/'); @@ -1115,7 +1114,7 @@ static void remote_ls(const char *path, int flags, void (*userFunc)(struct remote_ls_ctx *ls), void *userData) { - char *url = xmalloc(strlen(repo->url) + strlen(path) + 1); + char *url = xstrfmt("%s%s", repo->url, path); struct active_request_slot *slot; struct slot_results results; struct strbuf in_buffer = STRBUF_INIT; @@ -1131,8 +1130,6 @@ static void remote_ls(const char *path, int flags, ls.userData = userData; ls.userFunc = userFunc; - sprintf(url, "%s%s", repo->url, path); - strbuf_addf(&out_buffer.buf, PROPFIND_ALL_REQUEST); dav_headers = curl_slist_append(dav_headers, "Depth: 1"); @@ -1534,10 +1531,9 @@ static void update_remote_info_refs(struct remote_lock *lock) static int remote_exists(const char *path) { - char *url = xmalloc(strlen(repo->url) + strlen(path) + 1); + char *url = xstrfmt("%s%s", repo->url, path); int ret; - sprintf(url, "%s%s", repo->url, path); switch (http_get_strbuf(url, NULL, NULL)) { case HTTP_OK: @@ -1557,12 +1553,9 @@ static int remote_exists(const char *path) static void fetch_symref(const char *path, char **symref, unsigned char *sha1) { - char *url; + char *url = xstrfmt("%s%s", repo->url, path); struct strbuf buffer = STRBUF_INIT; - url = xmalloc(strlen(repo->url) + strlen(path) + 1); - sprintf(url, "%s%s", repo->url, path); - if (http_get_strbuf(url, &buffer, NULL) != HTTP_OK) die("Couldn't get %s for remote symref\n%s", url, curl_errorstr); @@ -1671,8 +1664,7 @@ static int delete_remote_branch(const char *pattern, int force) fprintf(stderr, "Removing remote branch '%s'\n", remote_ref->name); if (dry_run) return 0; - url = xmalloc(strlen(repo->url) + strlen(remote_ref->name) + 1); - sprintf(url, "%s%s", repo->url, remote_ref->name); + url = xstrfmt("%s%s", repo->url, remote_ref->name); slot = get_active_slot(); slot->results = &results; curl_setup_http_get(slot->curl, url, DAV_DELETE); diff --git a/http-walker.c b/http-walker.c index ab6a4febeb..dbddfaa177 100644 --- a/http-walker.c +++ b/http-walker.c @@ -341,8 +341,7 @@ static void fetch_alternates(struct walker *walker, const char *base) if (walker->get_verbosely) fprintf(stderr, "Getting alternates list for %s\n", base); - url = xmalloc(strlen(base) + 31); - sprintf(url, "%s/objects/info/http-alternates", base); + url = xstrfmt("%s/objects/info/http-alternates", base); /* * Use a callback to process the result, since another request diff --git a/match-trees.c b/match-trees.c index e80b4af354..1ce0954a3e 100644 --- a/match-trees.c +++ b/match-trees.c @@ -140,17 +140,12 @@ static void match_trees(const unsigned char *hash1, goto next; score = score_trees(elem, hash2); if (*best_score < score) { - char *newpath; - newpath = xmalloc(strlen(base) + strlen(path) + 1); - sprintf(newpath, "%s%s", base, path); free(*best_match); - *best_match = newpath; + *best_match = xstrfmt("%s%s", base, path); *best_score = score; } if (recurse_limit) { - char *newbase; - newbase = xmalloc(strlen(base) + strlen(path) + 2); - sprintf(newbase, "%s%s/", base, path); + char *newbase = xstrfmt("%s%s/", base, path); match_trees(elem, hash2, best_score, best_match, newbase, recurse_limit - 1); free(newbase); diff --git a/merge-recursive.c b/merge-recursive.c index cab16fafb5..532a1da2b0 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -969,14 +969,10 @@ merge_file_special_markers(struct merge_options *o, char *side2 = NULL; struct merge_file_info mfi; - if (filename1) { - side1 = xmalloc(strlen(branch1) + strlen(filename1) + 2); - sprintf(side1, "%s:%s", branch1, filename1); - } - if (filename2) { - side2 = xmalloc(strlen(branch2) + strlen(filename2) + 2); - sprintf(side2, "%s:%s", branch2, filename2); - } + if (filename1) + side1 = xstrfmt("%s:%s", branch1, filename1); + if (filename2) + side2 = xstrfmt("%s:%s", branch2, filename2); mfi = merge_file_1(o, one, a, b, side1 ? side1 : branch1, side2 ? side2 : branch2); From b2724c87872aaec55dd7e5529aa029c3108b43a5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:26:56 -0400 Subject: [PATCH 05/11] use xstrfmt to replace xmalloc + strcpy/strcat It's easy to get manual allocation calculations wrong, and the use of strcpy/strcat raise red flags for people looking for buffer overflows (though in this case each site was fine). It's also shorter to use xstrfmt, and the printf-format tends to be easier for a reader to see what the final string will look like. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/apply.c | 4 +--- builtin/fetch.c | 9 ++------- builtin/name-rev.c | 5 +---- sha1_name.c | 5 +---- shell.c | 6 +----- 5 files changed, 6 insertions(+), 23 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 9c5724eacc..b79691053f 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -1281,9 +1281,7 @@ static int parse_git_header(const char *line, int len, unsigned int size, struct */ patch->def_name = git_header_name(line, len); if (patch->def_name && root) { - char *s = xmalloc(root_len + strlen(patch->def_name) + 1); - strcpy(s, root); - strcpy(s + root_len, patch->def_name); + char *s = xstrfmt("%s%s", root, patch->def_name); free(patch->def_name); patch->def_name = s; } diff --git a/builtin/fetch.c b/builtin/fetch.c index 55f457c04f..40d989f9ff 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1053,16 +1053,11 @@ static int fetch_one(struct remote *remote, int argc, const char **argv) refs = xcalloc(argc + 1, sizeof(const char *)); for (i = 0; i < argc; i++) { if (!strcmp(argv[i], "tag")) { - char *ref; i++; if (i >= argc) die(_("You need to specify a tag name.")); - ref = xmalloc(strlen(argv[i]) * 2 + 22); - strcpy(ref, "refs/tags/"); - strcat(ref, argv[i]); - strcat(ref, ":refs/tags/"); - strcat(ref, argv[i]); - refs[j++] = ref; + refs[j++] = xstrfmt("refs/tags/%s:refs/tags/%s", + argv[i], argv[i]); } else refs[j++] = argv[i]; } diff --git a/builtin/name-rev.c b/builtin/name-rev.c index c824d4ec5f..3c8f319be6 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -33,10 +33,7 @@ static void name_rev(struct commit *commit, return; if (deref) { - char *new_name = xmalloc(strlen(tip_name)+3); - strcpy(new_name, tip_name); - strcat(new_name, "^0"); - tip_name = new_name; + tip_name = xstrfmt("%s^0", tip_name); if (generation) die("generation: %d, but deref?", generation); diff --git a/sha1_name.c b/sha1_name.c index 2b6322fad0..5e956904b6 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1252,10 +1252,7 @@ static void diagnose_invalid_sha1_path(const char *prefix, die("Path '%s' exists on disk, but not in '%.*s'.", filename, object_name_len, object_name); if (errno == ENOENT || errno == ENOTDIR) { - char *fullname = xmalloc(strlen(filename) - + strlen(prefix) + 1); - strcpy(fullname, prefix); - strcat(fullname, filename); + char *fullname = xstrfmt("%s%s", prefix, filename); if (!get_tree_entry(tree_sha1, fullname, sha1, &mode)) { diff --git a/shell.c b/shell.c index 5c0d47a5cc..ace62e4b65 100644 --- a/shell.c +++ b/shell.c @@ -46,11 +46,7 @@ static int is_valid_cmd_name(const char *cmd) static char *make_cmd(const char *prog) { - char *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2)); - strcpy(prefix, COMMAND_DIR); - strcat(prefix, "/"); - strcat(prefix, prog); - return prefix; + return xstrfmt("%s/%s", COMMAND_DIR, prog); } static void cd_to_homedir(void) From a0279e1865c7ce7417c0134e2ab32b20531f502a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:28:00 -0400 Subject: [PATCH 06/11] setup_git_env: use git_pathdup instead of xmalloc + sprintf This is shorter, harder to get wrong, and more clearly captures the intent. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- environment.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/environment.c b/environment.c index 4dac5e9edd..4de7b812ad 100644 --- a/environment.c +++ b/environment.c @@ -135,15 +135,11 @@ static void setup_git_env(void) gitfile = read_gitfile(git_dir); git_dir = xstrdup(gitfile ? gitfile : git_dir); git_object_dir = getenv(DB_ENVIRONMENT); - if (!git_object_dir) { - git_object_dir = xmalloc(strlen(git_dir) + 9); - sprintf(git_object_dir, "%s/objects", git_dir); - } + if (!git_object_dir) + git_object_dir = git_pathdup("objects"); git_index_file = getenv(INDEX_ENVIRONMENT); - if (!git_index_file) { - git_index_file = xmalloc(strlen(git_dir) + 7); - sprintf(git_index_file, "%s/index", git_dir); - } + if (!git_index_file) + git_index_file = git_pathdup("index"); git_graft_file = getenv(GRAFT_ENVIRONMENT); if (!git_graft_file) git_graft_file = git_pathdup("info/grafts"); From 3bdd55228b1b76a2c37143636966f333e3782888 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:28:20 -0400 Subject: [PATCH 07/11] sequencer: use argv_array_pushf This avoids a manual allocation calculation, and is shorter to boot. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sequencer.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/sequencer.c b/sequencer.c index 0a80c58d11..2fea824349 100644 --- a/sequencer.c +++ b/sequencer.c @@ -396,18 +396,13 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, { struct argv_array array; int rc; - char *gpg_sign; argv_array_init(&array); argv_array_push(&array, "commit"); argv_array_push(&array, "-n"); - if (opts->gpg_sign) { - gpg_sign = xmalloc(3 + strlen(opts->gpg_sign)); - sprintf(gpg_sign, "-S%s", opts->gpg_sign); - argv_array_push(&array, gpg_sign); - free(gpg_sign); - } + if (opts->gpg_sign) + argv_array_pushf(&array, "-S%s", opts->gpg_sign); if (opts->signoff) argv_array_push(&array, "-s"); if (!opts->edit) { From 5c1753b198a96ed6abaf6c09db4623df703005a6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:29:31 -0400 Subject: [PATCH 08/11] merge: use argv_array when spawning merge strategy This is shorter, and avoids a rather complicated set of allocation and free steps. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- merge.c | 42 +++++++++++++----------------------------- 1 file changed, 13 insertions(+), 29 deletions(-) diff --git a/merge.c b/merge.c index 70f1000fcb..1fa6e52bba 100644 --- a/merge.c +++ b/merge.c @@ -18,39 +18,23 @@ int try_merge_command(const char *strategy, size_t xopts_nr, const char **xopts, struct commit_list *common, const char *head_arg, struct commit_list *remotes) { - const char **args; - int i = 0, x = 0, ret; + struct argv_array args = ARGV_ARRAY_INIT; + int i, ret; struct commit_list *j; - struct strbuf buf = STRBUF_INIT; - args = xmalloc((4 + xopts_nr + commit_list_count(common) + - commit_list_count(remotes)) * sizeof(char *)); - strbuf_addf(&buf, "merge-%s", strategy); - args[i++] = buf.buf; - for (x = 0; x < xopts_nr; x++) { - char *s = xmalloc(strlen(xopts[x])+2+1); - strcpy(s, "--"); - strcpy(s+2, xopts[x]); - args[i++] = s; - } + argv_array_pushf(&args, "merge-%s", strategy); + for (i = 0; i < xopts_nr; i++) + argv_array_pushf(&args, "--%s", xopts[i]); for (j = common; j; j = j->next) - args[i++] = xstrdup(merge_argument(j->item)); - args[i++] = "--"; - args[i++] = head_arg; + argv_array_push(&args, merge_argument(j->item)); + argv_array_push(&args, "--"); + argv_array_push(&args, head_arg); for (j = remotes; j; j = j->next) - args[i++] = xstrdup(merge_argument(j->item)); - args[i] = NULL; - ret = run_command_v_opt(args, RUN_GIT_CMD); - strbuf_release(&buf); - i = 1; - for (x = 0; x < xopts_nr; x++) - free((void *)args[i++]); - for (j = common; j; j = j->next) - free((void *)args[i++]); - i += 2; - for (j = remotes; j; j = j->next) - free((void *)args[i++]); - free(args); + argv_array_push(&args, merge_argument(j->item)); + + ret = run_command_v_opt(args.argv, RUN_GIT_CMD); + argv_array_clear(&args); + discard_cache(); if (read_cache() < 0) die(_("failed to read the cache")); From f33206992de994424036a7d9912a968ab9829e6e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:29:48 -0400 Subject: [PATCH 09/11] walker_fetch: fix minor memory leak We sometimes allocate "msg" on the heap, but will fail to free it if we hit the failure code path. We can instead keep a separate variable that is safe to be freed no matter how we get to the failure code path. While we're here, we can also do two readability improvements: 1. Use xstrfmt instead of a manual malloc/sprintf 2. Due to the "maybe we allocate msg, maybe we don't" strategy, the logic for deciding which message to show was split into two parts. Since the deallocation is now pushed onto a separate variable, this is no longer a concern, and we can keep all of the logic in the same place. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- walker.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/walker.c b/walker.c index 1dd86b8f33..014826464e 100644 --- a/walker.c +++ b/walker.c @@ -253,7 +253,8 @@ int walker_fetch(struct walker *walker, int targets, char **target, { struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *)); unsigned char *sha1 = xmalloc(targets * 20); - char *msg; + const char *msg; + char *to_free = NULL; int ret; int i; @@ -285,21 +286,19 @@ int walker_fetch(struct walker *walker, int targets, char **target, if (loop(walker)) goto unlock_and_fail; - if (write_ref_log_details) { - msg = xmalloc(strlen(write_ref_log_details) + 12); - sprintf(msg, "fetch from %s", write_ref_log_details); - } else { - msg = NULL; - } + if (write_ref_log_details) + msg = to_free = xstrfmt("fetch from %s", write_ref_log_details); + else + msg = "fetch (unknown)"; for (i = 0; i < targets; i++) { if (!write_ref || !write_ref[i]) continue; - ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch (unknown)"); + ret = write_ref_sha1(lock[i], &sha1[20 * i], msg); lock[i] = NULL; if (ret) goto unlock_and_fail; } - free(msg); + free(to_free); return 0; @@ -307,6 +306,7 @@ int walker_fetch(struct walker *walker, int targets, char **target, for (i = 0; i < targets; i++) if (lock[i]) unlock_ref(lock[i]); + free(to_free); return -1; } From 45bc131dd3e1eb6edd903957cf9d42f37ad02181 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 19 Jun 2014 17:30:26 -0400 Subject: [PATCH 10/11] unique_path: fix unlikely heap overflow When merge-recursive creates a unique filename, it uses a template like: path~branch_%d where the final "_%d" is filled by an incrementing counter until we find a unique name. We allocate 8 characters for the counter, but there is no logic to limit the size of the integer. Of course, this is extremely unlikely, as you would need a hundred million collisions to trigger the problem. Even if an attacker constructed a specialized repo, it is unlikely that the victim would have the patience to run the merge. However, we can make it trivially correct (and hopefully more readable) by using a strbuf. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- merge-recursive.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/merge-recursive.c b/merge-recursive.c index 532a1da2b0..398a734f3f 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -601,25 +601,36 @@ static int remove_file(struct merge_options *o, int clean, return 0; } +/* add a string to a strbuf, but converting "/" to "_" */ +static void add_flattened_path(struct strbuf *out, const char *s) +{ + size_t i = out->len; + strbuf_addstr(out, s); + for (; i < out->len; i++) + if (out->buf[i] == '/') + out->buf[i] = '_'; +} + static char *unique_path(struct merge_options *o, const char *path, const char *branch) { - char *newpath = xmalloc(strlen(path) + 1 + strlen(branch) + 8 + 1); + struct strbuf newpath = STRBUF_INIT; int suffix = 0; struct stat st; - char *p = newpath + strlen(path); - strcpy(newpath, path); - *(p++) = '~'; - strcpy(p, branch); - for (; *p; ++p) - if ('/' == *p) - *p = '_'; - while (string_list_has_string(&o->current_file_set, newpath) || - string_list_has_string(&o->current_directory_set, newpath) || - lstat(newpath, &st) == 0) - sprintf(p, "_%d", suffix++); + size_t base_len; - string_list_insert(&o->current_file_set, newpath); - return newpath; + strbuf_addf(&newpath, "%s~", path); + add_flattened_path(&newpath, branch); + + base_len = newpath.len; + while (string_list_has_string(&o->current_file_set, newpath.buf) || + string_list_has_string(&o->current_directory_set, newpath.buf) || + lstat(newpath.buf, &st) == 0) { + strbuf_setlen(&newpath, base_len); + strbuf_addf(&newpath, "_%d", suffix++); + } + + string_list_insert(&o->current_file_set, newpath.buf); + return strbuf_detach(&newpath, NULL); } static int dir_in_way(const char *path, int check_working_copy) From cb6c38d5cce7d8d48a57346b332a68cea1489df1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 24 Jun 2014 16:58:15 -0400 Subject: [PATCH 11/11] setup_git_env(): introduce git_path_from_env() helper "Check the value of an environment and fall back to a known path inside $GIT_DIR" is repeated a few times to determine the location of the data store, the index and the graft file, but the return value of getenv is not guaranteed to survive across further invocations of setenv or even getenv. Make sure to xstrdup() the value we receive from getenv(3), and encapsulate the pattern into a helper function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- environment.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/environment.c b/environment.c index 4de7b812ad..565f65293b 100644 --- a/environment.c +++ b/environment.c @@ -124,6 +124,12 @@ static char *expand_namespace(const char *raw_namespace) return strbuf_detach(&buf, NULL); } +static char *git_path_from_env(const char *envvar, const char *path) +{ + const char *value = getenv(envvar); + return value ? xstrdup(value) : git_pathdup("%s", path); +} + static void setup_git_env(void) { const char *gitfile; @@ -134,15 +140,9 @@ static void setup_git_env(void) git_dir = DEFAULT_GIT_DIR_ENVIRONMENT; gitfile = read_gitfile(git_dir); git_dir = xstrdup(gitfile ? gitfile : git_dir); - git_object_dir = getenv(DB_ENVIRONMENT); - if (!git_object_dir) - git_object_dir = git_pathdup("objects"); - git_index_file = getenv(INDEX_ENVIRONMENT); - if (!git_index_file) - git_index_file = git_pathdup("index"); - git_graft_file = getenv(GRAFT_ENVIRONMENT); - if (!git_graft_file) - git_graft_file = git_pathdup("info/grafts"); + git_object_dir = git_path_from_env(DB_ENVIRONMENT, "objects"); + git_index_file = git_path_from_env(INDEX_ENVIRONMENT, "index"); + git_graft_file = git_path_from_env(GRAFT_ENVIRONMENT, "info/grafts"); if (getenv(NO_REPLACE_OBJECTS_ENVIRONMENT)) check_replace_refs = 0; namespace = expand_namespace(getenv(GIT_NAMESPACE_ENVIRONMENT));