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

[PATCH 4/4] commit-graph: close descriptors after mmap

From
Taylor Blau <me@ttaylorr.com>
Date
Apr 23, 2020, 21:41 UTC
Message-ID
<e05db264cb50760cab222157b436e82adeaeadc8.1587677671.git.me@ttaylorr.com>
In-Reply-To
<cover.1587677671.git.me@ttaylorr.com>
From: Jeff King <peff@peff.net>

We don't ever refer to the descriptor after mmap-ing it. And keeping it open means we can run out of descriptors in degenerate cases (e.g., thousands of split chain files). Let's close it as soon as possible.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c      | 15 +++++----------
 commit-graph.h      |  3 +--
 fuzz-commit-graph.c |  5 ++---
 3 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index b2d2fdfe3d..e9b458539f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -69,7 +69,6 @@ static uint8_t oid_version(void)
 static struct commit_graph *alloc_commit_graph(void)
 {
 	struct commit_graph *g = xcalloc(1, sizeof(*g));
-	g->graph_fd = -1;
 
 	return g;
 }
@@ -123,14 +122,13 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,
 		return NULL;
 	}
 	graph_map = xmmap(NULL, graph_size, PROT_READ, MAP_PRIVATE, fd, 0);
-	ret = parse_commit_graph(graph_map, fd, graph_size);
+	close(fd);
+	ret = parse_commit_graph(graph_map, graph_size);
 
 	if (ret)
 		ret->odb = odb;
-	else {
+	else
 		munmap(graph_map, graph_size);
-		close(fd);
-	}
 
 	return ret;
 }
@@ -165,8 +163,7 @@ static int verify_commit_graph_lite(struct commit_graph *g)
 	return 0;
 }
 
-struct commit_graph *parse_commit_graph(void *graph_map, int fd,
-					size_t graph_size)
+struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)
 {
 	const unsigned char *data, *chunk_lookup;
 	uint32_t i;
@@ -209,7 +206,6 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 
 	graph->hash_len = the_hash_algo->rawsz;
 	graph->num_chunks = *(unsigned char*)(data + 6);
-	graph->graph_fd = fd;
 	graph->data = graph_map;
 	graph->data_len = graph_size;
 
@@ -2125,10 +2121,9 @@ void free_commit_graph(struct commit_graph *g)
 {
 	if (!g)
 		return;
-	if (g->graph_fd >= 0) {
+	if (g->data) {
 		munmap((void *)g->data, g->data_len);
 		g->data = NULL;
-		close(g->graph_fd);
 	}
 	free(g->filename);
 	free(g);
diff --git a/commit-graph.h b/commit-graph.h
index 98ef121924..a0a2c4a1e5 100644
--- a/commit-graph.h
+++ b/commit-graph.h
@@ -66,8 +66,7 @@ struct commit_graph *load_commit_graph_one_fd_st(int fd, struct stat *st,
 						 struct object_directory *odb);
 struct commit_graph *read_commit_graph_one(struct repository *r,
 					   struct object_directory *odb);
-struct commit_graph *parse_commit_graph(void *graph_map, int fd,
-					size_t graph_size);
+struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);
 
 /*
  * Return 1 if and only if the repository has a commit-graph
diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c
index 0157acbf2e..9fd1c04edd 100644
--- a/fuzz-commit-graph.c
+++ b/fuzz-commit-graph.c
@@ -1,8 +1,7 @@
 #include "commit-graph.h"
 #include "repository.h"
 
-struct commit_graph *parse_commit_graph(void *graph_map, int fd,
-					size_t graph_size);
+struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size);
 
 int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
 
@@ -11,7 +10,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
 	struct commit_graph *g;
 
 	initialize_the_repository();
-	g = parse_commit_graph((void *)data, -1, size);
+	g = parse_commit_graph((void *)data, size);
 	repo_clear(the_repository);
 	free(g);
 
-- 
2.26.0.113.ge9739cdccc
Previous: Taylor BlauNext: Junio C Hamano
Message 4 of 14 in “commit-graph: handle file descriptor exhaustion”
  1. 0/4 commit-graph: handle file descriptor exhaustionTaylor Blau, Apr 23, 2020
  2. 1/4 commit-graph.c: don't use discarded graph_name in errorTaylor Blau, Apr 23, 2020
  3. 2/4 t/test-lib.sh: make ULIMIT_FILE_DESCRIPTORS available to testsTaylor Blau, Apr 23, 2020
  4. 4/4 commit-graph: close descriptors after mmapTaylor Blau, Apr 23, 2020
  5. Junio C HamanoApr 23, 2020
  6. Jeff KingApr 24, 2020
  7. Derrick StoleeApr 24, 2020
  8. Taylor BlauApr 24, 2020
  9. Junio C HamanoApr 24, 2020
  10. Derrick StoleeApr 27, 2020
  11. 3/4 commit-graph.c: gracefully handle file descriptor exhaustionTaylor Blau, Apr 23, 2020
  12. t5324-split-commit-graph.sh flaky due to assumptions about ulimit behaviorÆvar Arnfjörð Bjarmason, Jun 24, 2021
  13. Jeff KingJun 24, 2021
  14. Taylor BlauApr 23, 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.