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

[PATCH v2 4/5] commit-graph.c: remove path normalization, comparison

From
Taylor Blau <me@ttaylorr.com>
Date
Feb 3, 2020, 21:18 UTC
Message-ID
<f14e95aa7eb2fa4dce2c3e1a1d81dd022efede0b.1580764494.git.me@ttaylorr.com>
In-Reply-To
<cover.1580764494.git.me@ttaylorr.com>

As of the previous patch, all calls to 'commit-graph.c' functions which perform path normalization (for e.g., 'get_commit_graph_filename()') are of the form 'ctx->odb->path', which is always in normalized form.

Now that there are no callers passing non-normalized paths to these functions, ensure that future callers are bound by the same restrictions by making these functions take a 'struct object_directory *' instead of a 'const char *'. To match, replace all calls with arguments of the form 'ctx->odb->path' with 'ctx->odb' To recover the path, functions that perform path manipulation simply use 'odb->path'.

Further, avoid string comparisons with arguments of the form 'odb->path', and instead prefer raw pointer comparisons, which accomplish the same effect, but are far less brittle.

This has a pleasant side-effect of making these functions much more robust to paths that cannot be normalized by 'normalize_path_copy()', i.e., because they are outside of the current working directory.

For example, prior to this patch, Valgrind reports that the following uninitialized memory read [1]:

  $ ( cd t && GIT_DIR=../.git valgrind git rev-parse HEAD^ )

because 'normalize_path_copy()' can't normalize '../.git' (since it's relative to but above of the current working directory) [2].

By using a 'struct object_directory *' directly, 'get_commit_graph_filename()' does not need to normalize, because all paths are relative to the current working directory since they are always read from the '->path' of an object directory.

[1]: https://lore.kernel.org/git/20191027042116.GA5801@sigill.intra.peff.net.
[2]: The bug here is that 'get_commit_graph_filename()' returns the
     result of 'normalize_path_copy()' without checking the return
     value.
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/commit-graph.c     |  2 +-
 commit-graph.c             | 47 +++++++++++++++-----------------------
 commit-graph.h             |  2 +-
 t/helper/test-read-graph.c |  6 ++---
 4 files changed, 24 insertions(+), 33 deletions(-)
diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
index 3501d9077b..b16eba2a7a 100644
--- a/builtin/commit-graph.c
+++ b/builtin/commit-graph.c
@@ -87,7 +87,7 @@ static int graph_verify(int argc, const char **argv)
 		flags |= COMMIT_GRAPH_WRITE_PROGRESS;
 
 	odb = find_odb(the_repository, opts.obj_dir);
-	graph_name = get_commit_graph_filename(odb->path);
+	graph_name = get_commit_graph_filename(odb);
 	open_ok = open_commit_graph(graph_name, &fd, &st);
 	if (!open_ok && errno != ENOENT)
 		die_errno(_("Could not open commit-graph '%s'"), graph_name);
diff --git a/commit-graph.c b/commit-graph.c
index 3af4a721ee..49541760b5 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -44,30 +44,21 @@
 /* Remember to update object flag allocation in object.h */
 #define REACHABLE       (1u<<15)
 
-char *get_commit_graph_filename(const char *obj_dir)
+char *get_commit_graph_filename(struct object_directory *odb)
 {
-	char *filename = xstrfmt("%s/info/commit-graph", obj_dir);
-	char *normalized = xmalloc(strlen(filename) + 1);
-	normalize_path_copy(normalized, filename);
-	free(filename);
-	return normalized;
+	return xstrfmt("%s/info/commit-graph", odb->path);
 }
 
-static char *get_split_graph_filename(const char *obj_dir,
+static char *get_split_graph_filename(struct object_directory *odb,
 				      const char *oid_hex)
 {
-	char *filename = xstrfmt("%s/info/commit-graphs/graph-%s.graph",
-				 obj_dir,
-				 oid_hex);
-	char *normalized = xmalloc(strlen(filename) + 1);
-	normalize_path_copy(normalized, filename);
-	free(filename);
-	return normalized;
+	return xstrfmt("%s/info/commit-graphs/graph-%s.graph", odb->path,
+		       oid_hex);
 }
 
-static char *get_chain_filename(const char *obj_dir)
+static char *get_chain_filename(struct object_directory *odb)
 {
-	return xstrfmt("%s/info/commit-graphs/commit-graph-chain", obj_dir);
+	return xstrfmt("%s/info/commit-graphs/commit-graph-chain", odb->path);
 }
 
 static uint8_t oid_version(void)
@@ -330,7 +321,7 @@ static struct commit_graph *load_commit_graph_one(const char *graph_file)
 static struct commit_graph *load_commit_graph_v1(struct repository *r,
 						 struct object_directory *odb)
 {
-	char *graph_name = get_commit_graph_filename(odb->path);
+	char *graph_name = get_commit_graph_filename(odb);
 	struct commit_graph *g = load_commit_graph_one(graph_name);
 	free(graph_name);
 
@@ -381,7 +372,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
 	struct stat st;
 	struct object_id *oids;
 	int i = 0, valid = 1, count;
-	char *chain_name = get_chain_filename(odb->path);
+	char *chain_name = get_chain_filename(odb);
 	FILE *fp;
 	int stat_res;
 
@@ -414,7 +405,7 @@ static struct commit_graph *load_commit_graph_chain(struct repository *r,
 
 		valid = 0;
 		for (odb = r->objects->odb; odb; odb = odb->next) {
-			char *graph_name = get_split_graph_filename(odb->path, line.buf);
+			char *graph_name = get_split_graph_filename(odb, line.buf);
 			struct commit_graph *g = load_commit_graph_one(graph_name);
 
 			free(graph_name);
@@ -1375,7 +1366,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 			    ctx->odb->path);
 		ctx->graph_name = strbuf_detach(&tmp_file, NULL);
 	} else {
-		ctx->graph_name = get_commit_graph_filename(ctx->odb->path);
+		ctx->graph_name = get_commit_graph_filename(ctx->odb);
 	}
 
 	if (safe_create_leading_directories(ctx->graph_name)) {
@@ -1386,7 +1377,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	}
 
 	if (ctx->split) {
-		char *lock_name = get_chain_filename(ctx->odb->path);
+		char *lock_name = get_chain_filename(ctx->odb);
 
 		hold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);
 
@@ -1474,7 +1465,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 	if (ctx->split && ctx->base_graph_name && ctx->num_commit_graphs_after > 1) {
 		char *new_base_hash = xstrdup(oid_to_hex(&ctx->new_base_graph->oid));
-		char *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb->path, new_base_hash);
+		char *new_base_name = get_split_graph_filename(ctx->new_base_graph->odb, new_base_hash);
 
 		free(ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 2]);
 		free(ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 2]);
@@ -1510,12 +1501,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 				}
 			}
 		} else {
-			char *graph_name = get_commit_graph_filename(ctx->odb->path);
+			char *graph_name = get_commit_graph_filename(ctx->odb);
 			unlink(graph_name);
 		}
 
 		ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1] = xstrdup(oid_to_hex(&file_hash));
-		final_graph_name = get_split_graph_filename(ctx->odb->path,
+		final_graph_name = get_split_graph_filename(ctx->odb,
 					ctx->commit_graph_hash_after[ctx->num_commit_graphs_after - 1]);
 		ctx->commit_graph_filenames_after[ctx->num_commit_graphs_after - 1] = final_graph_name;
 
@@ -1557,7 +1548,7 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
 
 	while (g && (g->num_commits <= size_mult * num_commits ||
 		    (max_commits && num_commits > max_commits))) {
-		if (strcmp(g->odb->path, ctx->odb->path))
+		if (g->odb != ctx->odb)
 			break;
 
 		num_commits += g->num_commits;
@@ -1569,10 +1560,10 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)
 	ctx->new_base_graph = g;
 
 	if (ctx->num_commit_graphs_after == 2) {
-		char *old_graph_name = get_commit_graph_filename(g->odb->path);
+		char *old_graph_name = get_commit_graph_filename(g->odb);
 
 		if (!strcmp(g->filename, old_graph_name) &&
-		    strcmp(g->odb->path, ctx->odb->path)) {
+		    g->odb != ctx->odb) {
 			ctx->num_commit_graphs_after = 1;
 			ctx->new_base_graph = NULL;
 		}
@@ -1723,7 +1714,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 	if (ctx->split_opts && ctx->split_opts->expire_time)
 		expire_time -= ctx->split_opts->expire_time;
 	if (!ctx->split) {
-		char *chain_file_name = get_chain_filename(ctx->odb->path);
+		char *chain_file_name = get_chain_filename(ctx->odb);
 		unlink(chain_file_name);
 		free(chain_file_name);
 		ctx->num_commit_graphs_after = 0;
diff --git a/commit-graph.h b/commit-graph.h
index 2448134378..5a690723b0 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -12,7 +12,7 @@
 
 struct commit;
 
-char *get_commit_graph_filename(const char *obj_dir);
+char *get_commit_graph_filename(struct object_directory *odb);
 int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 
 /*
diff --git a/t/helper/test-read-graph.c b/t/helper/test-read-graph.c
index d2884efe0a..2c2f65f06c 100644
--- a/t/helper/test-read-graph.c
+++ b/t/helper/test-read-graph.c
@@ -11,12 +11,12 @@ int cmd__read_graph(int argc, const char **argv)
 	int open_ok;
 	int fd;
 	struct stat st;
-	const char *object_dir;
+	struct object_directory *odb;
 
 	setup_git_directory();
-	object_dir = get_object_directory();
+	odb = the_repository->objects->odb;
 
-	graph_name = get_commit_graph_filename(object_dir);
+	graph_name = get_commit_graph_filename(odb);
 
 	open_ok = open_commit_graph(graph_name, &fd, &st);
 	if (!open_ok)

-- 
2.25.0.119.gaa12b7378b
Previous: Taylor BlauNext: Taylor Blau
Message 26 of 28 in “commit-graph: use 'struct object_directory *' everywhere”
  1. 0/6 commit-graph: use 'struct object_directory *' everywhereTaylor Blau, Jan 30, 2020
  2. 1/6 t5318: don't pass non-object directory to '--object-dir'Taylor Blau, Jan 30, 2020
  3. 5/6 commit-graph.c: remove path normalization, comparisonTaylor Blau, Jan 30, 2020
  4. 6/6 commit-graph.h: use odb in 'load_commit_graph_one_fd_st'Taylor Blau, Jan 30, 2020
  5. 2/6 commit-graph.h: store object directory in 'struct commit_graph'Taylor Blau, Jan 30, 2020
  6. Martin ÅgrenJan 31, 2020
  7. Jeff KingJan 31, 2020
  8. Martin ÅgrenJan 31, 2020
  9. Taylor BlauFeb 3, 2020
  10. Jeff KingFeb 3, 2020
  11. Junio C HamanoJan 31, 2020
  12. Taylor BlauFeb 3, 2020
  13. 4/6 commit-graph.h: store an odb in 'struct write_commit_graph_context'Taylor Blau, Jan 30, 2020
  14. 3/6 builtin/commit-graph.c: die() with unknown '--object-dir'Taylor Blau, Jan 30, 2020
  15. Jeff KingJan 31, 2020
  16. Derrick StoleeJan 31, 2020
  17. Taylor BlauFeb 3, 2020
  18. 0/5 commit-graph: use 'struct object_directory *' everywhereTaylor Blau, Feb 3, 2020
  19. 1/5 t5318: don't pass non-object directory to '--object-dir'Taylor Blau, Feb 3, 2020
  20. 2/5 commit-graph.h: store an odb in 'struct write_commit_graph_context'Taylor Blau, Feb 3, 2020
  21. 2/5 commit-graph.h: store an odb in 'struct write_commit_graph_context'Taylor Blau, Feb 4, 2020
  22. Junio C HamanoFeb 4, 2020
  23. Taylor BlauFeb 4, 2020
  24. Junio C HamanoFeb 4, 2020
  25. 5/5 commit-graph.h: use odb in 'load_commit_graph_one_fd_st'Taylor Blau, Feb 3, 2020
  26. 4/5 commit-graph.c: remove path normalization, comparisonTaylor Blau, Feb 3, 2020
  27. 3/5 commit-graph.h: store object directory in 'struct commit_graph'Taylor Blau, Feb 3, 2020
  28. Jeff KingFeb 5, 2020

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.