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

[PATCH 06/17] prefer git_pathdup to git_path in some possibly-dangerous cases

From
Jeff King <peff@peff.net>
Date
Aug 10, 2015, 09:35 UTC
Message-ID
<20150810093531.GF30981@sigill.intra.peff.net>
In-Reply-To
<20150810092731.GA9027@sigill.intra.peff.net>

Because git_path uses a static buffer that is shared with calls to git_path, mkpath, etc, it can be dangerous to assign the result to a variable or pass it to a non-trivial function. The value may change unexpectedly due to other calls.

None of the cases changed here has a known bug, but they're worth converting away from git_path because:

  1. It's easy to use git_pathdup in these cases.
  2. They use constructs (like assignment) that make it
     hard to tell whether they're safe or not.

The extra malloc overhead should be trivial, as an allocation should be an order of magnitude cheaper than a system call (which we are clearly about to make, since we are constructing a filename). The real cost is that we must remember to free the result.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fsck.c |  4 +++-
 fast-import.c  |  4 +++-
 http-backend.c |  3 ++-
 notes-merge.c  |  3 ++-
 refs.c         | 14 ++++++++------
 unpack-trees.c |  4 +++-
 6 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index f4b87e9..0794703 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -243,13 +243,14 @@ static void check_unreachable_object(struct object *obj)
 			printf("dangling %s %s\n", typename(obj->type),
 			       sha1_to_hex(obj->sha1));
 		if (write_lost_and_found) {
-			const char *filename = git_path("lost-found/%s/%s",
+			char *filename = git_pathdup("lost-found/%s/%s",
 				obj->type == OBJ_COMMIT ? "commit" : "other",
 				sha1_to_hex(obj->sha1));
 			FILE *f;
 
 			if (safe_create_leading_directories_const(filename)) {
 				error("Could not create lost-found");
+				free(filename);
 				return;
 			}
 			if (!(f = fopen(filename, "w")))
@@ -262,6 +263,7 @@ static void check_unreachable_object(struct object *obj)
 			if (fclose(f))
 				die_errno("Could not finish '%s'",
 					  filename);
+			free(filename);
 		}
 		return;
 	}
diff --git a/fast-import.c b/fast-import.c
index 2ad4fee..ad8848b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -407,7 +407,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);
 
 static void write_crash_report(const char *err)
 {
-	const char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
+	char *loc = git_pathdup("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
 	FILE *rpt = fopen(loc, "w");
 	struct branch *b;
 	unsigned long lu;
@@ -415,6 +415,7 @@ static void write_crash_report(const char *err)
 
 	if (!rpt) {
 		error("can't write crash report %s: %s", loc, strerror(errno));
+		free(loc);
 		return;
 	}
 
@@ -488,6 +489,7 @@ static void write_crash_report(const char *err)
 	fputs("-------------------\n", rpt);
 	fputs("END OF CRASH REPORT\n", rpt);
 	fclose(rpt);
+	free(loc);
 }
 
 static void end_packfile(void);
diff --git a/http-backend.c b/http-backend.c
index b977c00..bac40ef 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -164,7 +164,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 
 static void send_local_file(const char *the_type, const char *name)
 {
-	const char *p = git_path("%s", name);
+	char *p = git_pathdup("%s", name);
 	size_t buf_alloc = 8192;
 	char *buf = xmalloc(buf_alloc);
 	int fd;
@@ -191,6 +191,7 @@ static void send_local_file(const char *the_type, const char *name)
 	}
 	close(fd);
 	free(buf);
+	free(p);
 }
 
 static void get_text_file(char *name)
diff --git a/notes-merge.c b/notes-merge.c
index 0b2b82c..b3d1dab 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -295,7 +295,7 @@ static void write_buf_to_worktree(const unsigned char *obj,
 				  const char *buf, unsigned long size)
 {
 	int fd;
-	const char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
+	char *path = git_pathdup(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
 	if (safe_create_leading_directories_const(path))
 		die_errno("unable to create directory for '%s'", path);
 	if (file_exists(path))
@@ -320,6 +320,7 @@ static void write_buf_to_worktree(const unsigned char *obj,
 	}
 
 	close(fd);
+	free(path);
 }
 
 static void write_note_to_worktree(const unsigned char *obj,
diff --git a/refs.c b/refs.c
index 2db2975..93b250e 100644
--- a/refs.c
+++ b/refs.c
@@ -1288,12 +1288,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
 {
-	const char *packed_refs_file;
+	char *packed_refs_file;
 
 	if (*refs->name)
-		packed_refs_file = git_path_submodule(refs->name, "packed-refs");
+		packed_refs_file = git_pathdup_submodule(refs->name, "packed-refs");
 	else
-		packed_refs_file = git_path("packed-refs");
+		packed_refs_file = git_pathdup("packed-refs");
 
 	if (refs->packed &&
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
@@ -1312,6 +1312,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
 			fclose(f);
 		}
 	}
+	free(packed_refs_file);
 	return refs->packed;
 }
 
@@ -1481,14 +1482,15 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
 {
 	int fd, len;
 	char buffer[128], *p;
-	const char *path;
+	char *path;
 
 	if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
 		return -1;
 	path = *refs->name
-		? git_path_submodule(refs->name, "%s", refname)
-		: git_path("%s", refname);
+		? git_pathdup_submodule(refs->name, "%s", refname)
+		: git_pathdup("%s", refname);
 	fd = open(path, O_RDONLY);
+	free(path);
 	if (fd < 0)
 		return resolve_gitlink_packed_ref(refs, refname, sha1);
 
diff --git a/unpack-trees.c b/unpack-trees.c
index d6cf849..7bb446a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1029,10 +1029,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	if (!core_apply_sparse_checkout || !o->update)
 		o->skip_sparse_checkout = 1;
 	if (!o->skip_sparse_checkout) {
-		if (add_excludes_from_file_to_list(git_path("info/sparse-checkout"), "", 0, &el, 0) < 0)
+		char *sparse = git_pathdup("info/sparse-checkout");
+		if (add_excludes_from_file_to_list(sparse, "", 0, &el, 0) < 0)
 			o->skip_sparse_checkout = 1;
 		else
 			o->el = &el;
+		free(sparse);
 	}
 
 	memset(&o->result, 0, sizeof(o->result));

-- 
2.5.0.414.g670f2a4
Previous: Jeff KingNext: Jeff King
Message 11 of 34 in “removing questionable uses of git_path”
  1. 0/17 removing questionable uses of git_pathJeff King, Aug 10, 2015
  2. 01/17 cache.h: clarify documentation for git_path, et alJeff King, Aug 10, 2015
  3. 02/17 cache.h: complete set of git_path_submodule helpersJeff King, Aug 10, 2015
  4. 03/17 t5700: modernize styleJeff King, Aug 10, 2015
  5. 04/17 add_to_alternates_file: don't add duplicate entriesJeff King, Aug 10, 2015
  6. Michael HaggertyAug 11, 2015
  7. Jeff KingAug 11, 2015
  8. 05/17 remove hold_lock_file_for_appendJeff King, Aug 10, 2015
  9. Junio C HamanoAug 10, 2015
  10. Jeff KingAug 11, 2015
  11. 06/17 prefer git_pathdup to git_path in some possibly-dangerous casesJeff King, Aug 10, 2015
  12. 07/17 prefer mkpathdup to mkpath in assignmentsJeff King, Aug 10, 2015
  13. 08/17 remote.c: drop extraneous local variable from migrate_fileJeff King, Aug 10, 2015
  14. 09/17 refs.c: remove extra git_path calls from read_loose_refsJeff King, Aug 10, 2015
  15. 10/17 path.c: drop git_path_submoduleJeff King, Aug 10, 2015
  16. Junio C HamanoAug 10, 2015
  17. Junio C HamanoAug 10, 2015
  18. Junio C HamanoAug 10, 2015
  19. Jeff KingAug 11, 2015
  20. 11/17 refs.c: simplify strbufs in reflog setup and writingJeff King, Aug 10, 2015
  21. Michael HaggertyAug 10, 2015
  22. Jeff KingAug 10, 2015
  23. 12/17 refs.c: avoid repeated git_path calls in rename_tmp_logJeff King, Aug 10, 2015
  24. 13/17 refs.c: avoid git_path assignment in lock_ref_sha1_basicJeff King, Aug 10, 2015
  25. 14/17 refs.c: remove_empty_directories can take a strbufJeff King, Aug 10, 2015
  26. 15/17 find_hook: keep our own static bufferJeff King, Aug 10, 2015
  27. 16/17 get_repo_path: refactor path-allocationJeff King, Aug 10, 2015
  28. 17/17 memoize common git-path "constant" filesJeff King, Aug 10, 2015
  29. Michael HaggertyAug 10, 2015
  30. Jeff KingAug 10, 2015
  31. Michael HaggertyAug 10, 2015
  32. Junio C HamanoAug 10, 2015
  33. Jeff KingAug 10, 2015
  34. Duy NguyenAug 15, 2015

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.