git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH 2/2] use xstrdup_fmt in favor of manual size calculations

From
Jeff King <peff@peff.net>
Date
Jun 18, 2014, 20:02 UTC
Message-ID
<20140618200213.GB23057@sigill.intra.peff.net>
In-Reply-To
<20140618200000.GA22994@sigill.intra.peff.net>

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 xstrdup_fmt, which handles the allocation automatically, and makes the code shorter and more readable.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c       |  6 +-----
 unpack-trees.c | 17 ++++++-----------
 2 files changed, 7 insertions(+), 16 deletions(-)
diff --git a/remote.c b/remote.c
index 0e9459c..792dcee 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 = xstrdup_fmt("refs/heads/%s", ret->name);
 
 	return ret;
 }
diff --git a/unpack-trees.c b/unpack-trees.c
index 97fc995..dd1e06e 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] =
+		xstrdup_fmt(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] = xstrdup_fmt(msg, "removed", cmd, cmd2);
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_OVERWRITTEN] = xstrdup_fmt(msg, "overwritten", cmd, cmd2);
 
 	/*
 	 * Special case: ERROR_BIND_OVERLAP refers to a pair of paths, we

-- 
2.0.0.566.gfe3e6b2
Previous: René Scharfe
Message 27 of 27 in “dropping manual malloc calculations”
  1. 0/2 dropping manual malloc calculationsJeff King, Jun 18, 2014
  2. 1/2 strbuf: add xstrdup_fmt helperJeff King, Jun 18, 2014
  3. Junio C HamanoJun 18, 2014
  4. Jeff KingJun 19, 2014
  5. Junio C HamanoJun 19, 2014
  6. dropping manual malloc calculationsJeff King, Jun 19, 2014
  7. 01/10 strbuf: add xstrfmt helperJeff King, Jun 19, 2014
  8. 02/10 use xstrfmt in favor of manual size calculationsJeff King, Jun 19, 2014
  9. 03/10 use xstrdup instead of xmalloc + strcpyJeff King, Jun 19, 2014
  10. 04/10 use xstrfmt to replace xmalloc + sprintfJeff King, Jun 19, 2014
  11. 05/10 use xstrfmt to replace xmalloc + strcpy/strcatJeff King, Jun 19, 2014
  12. 06/10 setup_git_env: use git_pathdup instead of xmalloc + sprintfJeff King, Jun 19, 2014
  13. Eric SunshineJun 23, 2014
  14. Junio C HamanoJun 23, 2014
  15. Duy NguyenJun 24, 2014
  16. Duy NguyenJun 24, 2014
  17. Jeff KingJun 24, 2014
  18. Duy NguyenJun 25, 2014
  19. Junio C HamanoJun 25, 2014
  20. Jeff KingJun 25, 2014
  21. Junio C HamanoJun 25, 2014
  22. 07/10 sequencer: use argv_array_pushfJeff King, Jun 19, 2014
  23. 08/10 merge: use argv_array when spawning merge strategyJeff King, Jun 19, 2014
  24. 09/10 walker_fetch: fix minor memory leakJeff King, Jun 19, 2014
  25. 10/10 unique_path: fix unlikely heap overflowJeff King, Jun 19, 2014
  26. René ScharfeJun 19, 2014
  27. 2/2 use xstrdup_fmt in favor of manual size calculationsJeff King, Jun 18, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.