* [PATCH v2 01/10] strbuf: add xstrfmt helper
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
@ 2014-06-19 21:18 ` Jeff King
2014-06-19 21:19 ` [PATCH v2 02/10] use xstrfmt in favor of manual size calculations Jeff King
` (8 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
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 <peff@peff.net>
---
strbuf.c | 19 +++++++++++++++++++
strbuf.h | 9 +++++++++
2 files changed, 28 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index ac62982..12c7865 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 e9ad03e..a594c24 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 */
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 02/10] use xstrfmt in favor of manual size calculations
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
2014-06-19 21:18 ` [PATCH v2 01/10] strbuf: add xstrfmt helper Jeff King
@ 2014-06-19 21:19 ` Jeff King
2014-06-19 21:19 ` [PATCH v2 03/10] use xstrdup instead of xmalloc + strcpy Jeff King
` (7 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
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 <peff@peff.net>
---
These could actually be squashed into later commits, I suppose, but I
left it separate since it had already been reviewed.
remote.c | 6 +-----
unpack-trees.c | 17 ++++++-----------
2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/remote.c b/remote.c
index 0e9459c..bf27e44 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 97fc995..c237370 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
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 03/10] use xstrdup instead of xmalloc + strcpy
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
2014-06-19 21:18 ` [PATCH v2 01/10] strbuf: add xstrfmt helper Jeff King
2014-06-19 21:19 ` [PATCH v2 02/10] use xstrfmt in favor of manual size calculations Jeff King
@ 2014-06-19 21:19 ` Jeff King
2014-06-19 21:24 ` [PATCH v2 04/10] use xstrfmt to replace xmalloc + sprintf Jeff King
` (6 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
This is one line shorter, and makes sure the length in the
malloc and copy steps match.
Signed-off-by: Jeff King <peff@peff.net>
---
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 c323081..18458e8 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 de00d16..95650a0 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 1516c5e..ab6a4fe 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;
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 04/10] use xstrfmt to replace xmalloc + sprintf
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
` (2 preceding siblings ...)
2014-06-19 21:19 ` [PATCH v2 03/10] use xstrdup instead of xmalloc + strcpy Jeff King
@ 2014-06-19 21:24 ` Jeff King
2014-06-19 21:26 ` [PATCH v2 05/10] use xstrfmt to replace xmalloc + strcpy/strcat Jeff King
` (5 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
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 <peff@peff.net>
---
Just a note on one thing I would look for as a reviewer:
In theory this could introduce a time-of-use error (either we xstrfmt
at the malloc, in which case the arguments to format might not be
ready yet, or we xstrfmt where the old sprintf was, in which case the
pointer is uninitialized earlier). In practice, this is not an issue.
The two are almost always right next to each other. And even when they
are not, the xmalloc already runs strlen() on the arguments, so it
should be safe to do xstrfmt there, too. I.e., if there is a bug, it
was already there. :)
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 3906eda..c462e19 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 d873172..5fd4e4e 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 95650a0..390f74c 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 ab6a4fe..dbddfaa 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 e80b4af..1ce0954 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 cab16fa..532a1da 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);
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 05/10] use xstrfmt to replace xmalloc + strcpy/strcat
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
` (3 preceding siblings ...)
2014-06-19 21:24 ` [PATCH v2 04/10] use xstrfmt to replace xmalloc + sprintf Jeff King
@ 2014-06-19 21:26 ` Jeff King
2014-06-19 21:28 ` [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf Jeff King
` (4 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
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 <peff@peff.net>
---
By the way, I think that the tip_name allocation in name_rev leaks
badly, but it's a little tricky to fix (we sometimes hand off ownership
of the variable, and sometimes not). However, this patch does not make
it any worse, and nobody is complaining, so I left it for now.
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 9c5724e..b796910 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 55f457c..40d989f 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 c824d4e..3c8f319 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 2b6322f..5e95690 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 5c0d47a..ace62e4 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)
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
` (4 preceding siblings ...)
2014-06-19 21:26 ` [PATCH v2 05/10] use xstrfmt to replace xmalloc + strcpy/strcat Jeff King
@ 2014-06-19 21:28 ` Jeff King
2014-06-23 10:21 ` Eric Sunshine
2014-06-24 13:30 ` Duy Nguyen
2014-06-19 21:28 ` [PATCH v2 07/10] sequencer: use argv_array_pushf Jeff King
` (3 subsequent siblings)
9 siblings, 2 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
This is shorter, harder to get wrong, and more clearly
captures the intent.
Signed-off-by: Jeff King <peff@peff.net>
---
I wondered if there was a reason to avoid this (because we are in
setup_git_env, which can potentially be called by git_pathdup). But the
git_graft_file initialization below already uses it, and I
double-checked that it is safe once git_dir is set.
environment.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/environment.c b/environment.c
index 4dac5e9..4de7b81 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");
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-19 21:28 ` [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf Jeff King
@ 2014-06-23 10:21 ` Eric Sunshine
2014-06-23 22:43 ` Junio C Hamano
2014-06-24 13:02 ` Duy Nguyen
2014-06-24 13:30 ` Duy Nguyen
1 sibling, 2 replies; 27+ messages in thread
From: Eric Sunshine @ 2014-06-23 10:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, René Scharfe, Git List
On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:
> This is shorter, harder to get wrong, and more clearly
> captures the intent.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I wondered if there was a reason to avoid this (because we are in
> setup_git_env, which can potentially be called by git_pathdup). But the
> git_graft_file initialization below already uses it, and I
> double-checked that it is safe once git_dir is set.
This patch will conflict textually with patch 6/28 of Duy's
nd/multiple-work-trees series [1].
[1]: http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649
> environment.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 4dac5e9..4de7b81 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");
> --
> 2.0.0.566.gfe3e6b2
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-23 10:21 ` Eric Sunshine
@ 2014-06-23 22:43 ` Junio C Hamano
2014-06-24 13:02 ` Duy Nguyen
1 sibling, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2014-06-23 22:43 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Jeff King, René Scharfe, Git List
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:
>> This is shorter, harder to get wrong, and more clearly
>> captures the intent.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I wondered if there was a reason to avoid this (because we are in
>> setup_git_env, which can potentially be called by git_pathdup). But the
>> git_graft_file initialization below already uses it, and I
>> double-checked that it is safe once git_dir is set.
>
> This patch will conflict textually with patch 6/28 of Duy's
> nd/multiple-work-trees series [1].
Thanks; I noticed that and dropped the other topic tentatively, as
it is being rerolled anyway. In addition to that, because this
series seems fairly focused and well done, and the owners of two
topics known to be competent and active folks, I do not think there
is not much to be worried about ;-).
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/242300/focus=243649
>
>> environment.c | 12 ++++--------
>> 1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/environment.c b/environment.c
>> index 4dac5e9..4de7b81 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");
>> --
>> 2.0.0.566.gfe3e6b2
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-23 10:21 ` Eric Sunshine
2014-06-23 22:43 ` Junio C Hamano
@ 2014-06-24 13:02 ` Duy Nguyen
1 sibling, 0 replies; 27+ messages in thread
From: Duy Nguyen @ 2014-06-24 13:02 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Jeff King, Junio C Hamano, René Scharfe, Git List
On Mon, Jun 23, 2014 at 5:21 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Jun 19, 2014 at 5:28 PM, Jeff King <peff@peff.net> wrote:
>> This is shorter, harder to get wrong, and more clearly
>> captures the intent.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I wondered if there was a reason to avoid this (because we are in
>> setup_git_env, which can potentially be called by git_pathdup). But the
>> git_graft_file initialization below already uses it, and I
>> double-checked that it is safe once git_dir is set.
>
> This patch will conflict textually with patch 6/28 of Duy's
> nd/multiple-work-trees series [1].
I'll just steal the conflicted bit about git_object_dir and put in the
re-roll. It's a better way anyway. If git_index_file change still
results in conflicts, Junio can resolve it easily (I don't touch it in
nd/multiple-work-trees).
--
Duy
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-19 21:28 ` [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf Jeff King
2014-06-23 10:21 ` Eric Sunshine
@ 2014-06-24 13:30 ` Duy Nguyen
2014-06-24 20:58 ` Jeff King
1 sibling, 1 reply; 27+ messages in thread
From: Duy Nguyen @ 2014-06-24 13:30 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, René Scharfe, Git Mailing List
While it's about malloc..
On Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:
> diff --git a/environment.c b/environment.c
> index 4dac5e9..4de7b81 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 DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because
getenv's return value is not guaranteed persistent. Since you're touch
this area, perhaps do it too (in this, or another patch)?
--
Duy
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-24 13:30 ` Duy Nguyen
@ 2014-06-24 20:58 ` Jeff King
2014-06-25 12:37 ` Duy Nguyen
2014-06-25 17:20 ` Junio C Hamano
0 siblings, 2 replies; 27+ messages in thread
From: Jeff King @ 2014-06-24 20:58 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, René Scharfe, Git Mailing List
On Tue, Jun 24, 2014 at 08:30:26PM +0700, Duy Nguyen wrote:
> On Fri, Jun 20, 2014 at 4:28 AM, Jeff King <peff@peff.net> wrote:
> > diff --git a/environment.c b/environment.c
> > index 4dac5e9..4de7b81 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 DB_ENVIRONMENT is set, we should xstrdup(git_object_dir) because
> getenv's return value is not guaranteed persistent. Since you're touch
> this area, perhaps do it too (in this, or another patch)?
Here's a replacement patch that handles this (and just drops the ugly
mallocs as a side effect).
-- >8 --
Subject: [PATCH] setup_git_env: copy getenv return value
The return value of getenv is not guaranteed to survive
across further invocations of setenv or even getenv. When we
are assigning it to globals that last the lifetime of the
program, we should make our own copy.
Signed-off-by: Jeff King <peff@peff.net>
---
environment.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/environment.c b/environment.c
index 4dac5e9..efb2fa0 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(path);
+}
+
static void setup_git_env(void)
{
const char *gitfile;
@@ -134,19 +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 = xmalloc(strlen(git_dir) + 9);
- sprintf(git_object_dir, "%s/objects", git_dir);
- }
- 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);
- }
- 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));
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-24 20:58 ` Jeff King
@ 2014-06-25 12:37 ` Duy Nguyen
2014-06-25 17:20 ` Junio C Hamano
1 sibling, 0 replies; 27+ messages in thread
From: Duy Nguyen @ 2014-06-25 12:37 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, René Scharfe, Git Mailing List
On Wed, Jun 25, 2014 at 3:58 AM, Jeff King <peff@peff.net> wrote:
> Here's a replacement patch that handles this (and just drops the ugly
> mallocs as a side effect).
Shortly after I wrote my email, I thought about getenvdup() and look
for similar getenv/xstrdup patterns. But I saw only one in config.c.
So let's forget about it. Your patch looks good.
>
> -- >8 --
> Subject: [PATCH] setup_git_env: copy getenv return value
>
> The return value of getenv is not guaranteed to survive
> across further invocations of setenv or even getenv. When we
> are assigning it to globals that last the lifetime of the
> program, we should make our own copy.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> environment.c | 22 +++++++++-------------
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index 4dac5e9..efb2fa0 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(path);
> +}
> +
> static void setup_git_env(void)
> {
> const char *gitfile;
> @@ -134,19 +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 = xmalloc(strlen(git_dir) + 9);
> - sprintf(git_object_dir, "%s/objects", git_dir);
> - }
> - 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);
> - }
> - 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));
> --
> 2.0.0.566.gfe3e6b2
>
--
Duy
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-24 20:58 ` Jeff King
2014-06-25 12:37 ` Duy Nguyen
@ 2014-06-25 17:20 ` Junio C Hamano
2014-06-25 17:22 ` Jeff King
1 sibling, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2014-06-25 17:20 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, René Scharfe, Git Mailing List
Jeff King <peff@peff.net> writes:
> Here's a replacement patch that handles this (and just drops the ugly
> mallocs as a side effect).
>
> -- >8 --
> Subject: [PATCH] setup_git_env: copy getenv return value
>
> The return value of getenv is not guaranteed to survive
> across further invocations of setenv or even getenv. When we
> are assigning it to globals that last the lifetime of the
> program, we should make our own copy.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
into next, 2014-06-23) with about 20 hours of lag.
I'd make it relative like the attached on top of the series. Note
that I tweaked the args to git_pathdup() to avoid the "are you sure
you want to give a variable format string to git_pathdup() which you
said is like printf(3)?" warning from the compiler.
Thanks.
-- >8 --
From: Jeff King <peff@peff.net>
Date: Tue, 24 Jun 2014 16:58:15 -0400
Subject: [PATCH] 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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
environment.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/environment.c b/environment.c
index 4de7b81..565f652 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));
--
2.0.0-641-g934bf98
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-25 17:20 ` Junio C Hamano
@ 2014-06-25 17:22 ` Jeff King
2014-06-25 19:54 ` Junio C Hamano
0 siblings, 1 reply; 27+ messages in thread
From: Jeff King @ 2014-06-25 17:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, René Scharfe, Git Mailing List
On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Here's a replacement patch that handles this (and just drops the ugly
> > mallocs as a side effect).
> >
> > -- >8 --
> > Subject: [PATCH] setup_git_env: copy getenv return value
> >
> > The return value of getenv is not guaranteed to survive
> > across further invocations of setenv or even getenv. When we
> > are assigning it to globals that last the lifetime of the
> > program, we should make our own copy.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
>
> Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
> into next, 2014-06-23) with about 20 hours of lag.
Ah, sorry. I had checked yesterday that jk/xstrfmt hadn't been merged
yet, but I didn't check when responding to Duy.
> I'd make it relative like the attached on top of the series. Note
> that I tweaked the args to git_pathdup() to avoid the "are you sure
> you want to give a variable format string to git_pathdup() which you
> said is like printf(3)?" warning from the compiler.
Both changes look good to me. Thanks for taking care of it.
-Peff
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf
2014-06-25 17:22 ` Jeff King
@ 2014-06-25 19:54 ` Junio C Hamano
0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2014-06-25 19:54 UTC (permalink / raw)
To: Jeff King; +Cc: Duy Nguyen, René Scharfe, Git Mailing List
Jeff King <peff@peff.net> writes:
> On Wed, Jun 25, 2014 at 10:20:13AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > Here's a replacement patch that handles this (and just drops the ugly
>> > mallocs as a side effect).
>> >
>> > -- >8 --
>> > Subject: [PATCH] setup_git_env: copy getenv return value
>> >
>> > The return value of getenv is not guaranteed to survive
>> > across further invocations of setenv or even getenv. When we
>> > are assigning it to globals that last the lifetime of the
>> > program, we should make our own copy.
>> >
>> > Signed-off-by: Jeff King <peff@peff.net>
>> > ---
>>
>> Sigh. This mail unfortunately crossed with 64f25581 (Merge branch 'jk/xstrfmt'
>> into next, 2014-06-23) with about 20 hours of lag.
>
> Ah, sorry. I had checked yesterday that jk/xstrfmt hadn't been merged
> yet, but I didn't check when responding to Duy.
Sorry to have sighed --- crossing e-mails happen all the time. No
need to feel sorry.
>> I'd make it relative like the attached on top of the series. Note
>> that I tweaked the args to git_pathdup() to avoid the "are you sure
>> you want to give a variable format string to git_pathdup() which you
>> said is like printf(3)?" warning from the compiler.
>
> Both changes look good to me. Thanks for taking care of it.
Thanks.
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 07/10] sequencer: use argv_array_pushf
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
` (5 preceding siblings ...)
2014-06-19 21:28 ` [PATCH v2 06/10] setup_git_env: use git_pathdup instead of xmalloc + sprintf Jeff King
@ 2014-06-19 21:28 ` Jeff King
2014-06-19 21:29 ` [PATCH v2 08/10] merge: use argv_array when spawning merge strategy Jeff King
` (2 subsequent siblings)
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
This avoids a manual allocation calculation, and is shorter
to boot.
Signed-off-by: Jeff King <peff@peff.net>
---
sequencer.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..2fea824 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) {
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 08/10] merge: use argv_array when spawning merge strategy
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
` (6 preceding siblings ...)
2014-06-19 21:28 ` [PATCH v2 07/10] sequencer: use argv_array_pushf Jeff King
@ 2014-06-19 21:29 ` Jeff King
2014-06-19 21:29 ` [PATCH v2 09/10] walker_fetch: fix minor memory leak Jeff King
2014-06-19 21:30 ` [PATCH v2 10/10] unique_path: fix unlikely heap overflow Jeff King
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
This is shorter, and avoids a rather complicated set of
allocation and free steps.
Signed-off-by: Jeff King <peff@peff.net>
---
merge.c | 42 +++++++++++++-----------------------------
1 file changed, 13 insertions(+), 29 deletions(-)
diff --git a/merge.c b/merge.c
index 70f1000..1fa6e52 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;
- }
- for (j = common; j; j = j->next)
- args[i++] = xstrdup(merge_argument(j->item));
- args[i++] = "--";
- args[i++] = 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++]);
+ 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)
- free((void *)args[i++]);
- i += 2;
+ 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)
- 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"));
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 09/10] walker_fetch: fix minor memory leak
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
` (7 preceding siblings ...)
2014-06-19 21:29 ` [PATCH v2 08/10] merge: use argv_array when spawning merge strategy Jeff King
@ 2014-06-19 21:29 ` Jeff King
2014-06-19 21:30 ` [PATCH v2 10/10] unique_path: fix unlikely heap overflow Jeff King
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
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 <peff@peff.net>
---
walker.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..0148264 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 @@ unlock_and_fail:
for (i = 0; i < targets; i++)
if (lock[i])
unlock_ref(lock[i]);
+ free(to_free);
return -1;
}
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 10/10] unique_path: fix unlikely heap overflow
2014-06-19 21:16 ` [PATCH v2] dropping manual malloc calculations Jeff King
` (8 preceding siblings ...)
2014-06-19 21:29 ` [PATCH v2 09/10] walker_fetch: fix minor memory leak Jeff King
@ 2014-06-19 21:30 ` Jeff King
9 siblings, 0 replies; 27+ messages in thread
From: Jeff King @ 2014-06-19 21:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
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 <peff@peff.net>
---
merge-recursive.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 532a1da..398a734 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++);
-
- string_list_insert(&o->current_file_set, newpath);
- return newpath;
+ size_t base_len;
+
+ 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)
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 27+ messages in thread