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

[PATCH 7/9] commit-graph: check order while reading fanout chunk

From
Jeff King <peff@peff.net>
Date
Nov 9, 2023, 07:25 UTC
Message-ID
<20231109072507.GG2698043@coredump.intra.peff.net>
In-Reply-To
<20231109070310.GA2697602@coredump.intra.peff.net>

We read the fanout chunk, storing a pointer to it, but only confirm that the entries are monotonic in a final "lite" verification step. Let's move that into the actual OIDF chunk callback, so that we can report problems immediately (for all the reasons given in the previous "commit-graph: abort as soon as we see a bogus chunk" commit).

Signed-off-by: Jeff King <peff@peff.net>
---
 commit-graph.c          | 25 +++++++++++++------------
 t/t5318-commit-graph.sh |  1 +
 2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/commit-graph.c b/commit-graph.c
index 094814c2ba..4ba523cd15 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -277,8 +277,6 @@ struct commit_graph *load_commit_graph_one_fd_st(struct repository *r,
 
 static int verify_commit_graph_lite(struct commit_graph *g)
 {
-	int i;
-
 	/*
 	 * Basic validation shared between parse_commit_graph()
 	 * which'll be called every time the graph is used, and the
@@ -291,27 +289,30 @@ static int verify_commit_graph_lite(struct commit_graph *g)
 	 * over g->num_commits, or runs a checksum on the commit-graph
 	 * itself.
 	 */
-	for (i = 0; i < 255; i++) {
-		uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
-		uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
-
-		if (oid_fanout1 > oid_fanout2) {
-			error("commit-graph fanout values out of order");
-			return 1;
-		}
-	}
-
 	return 0;
 }
 
 static int graph_read_oid_fanout(const unsigned char *chunk_start,
 				 size_t chunk_size, void *data)
 {
 	struct commit_graph *g = data;
+	int i;
+
 	if (chunk_size != 256 * sizeof(uint32_t))
 		return error("commit-graph oid fanout chunk is wrong size");
 	g->chunk_oid_fanout = (const uint32_t *)chunk_start;
 	g->num_commits = ntohl(g->chunk_oid_fanout[255]);
+
+	for (i = 0; i < 255; i++) {
+		uint32_t oid_fanout1 = ntohl(g->chunk_oid_fanout[i]);
+		uint32_t oid_fanout2 = ntohl(g->chunk_oid_fanout[i + 1]);
+
+		if (oid_fanout1 > oid_fanout2) {
+			error("commit-graph fanout values out of order");
+			return 1;
+		}
+	}
+
 	return 0;
 }
 
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 8bd7fcefb5..7fe7c72a87 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -867,6 +867,7 @@ test_expect_success 'reader notices out-of-bounds fanout' '
 	check_corrupt_chunk OIDF 0 $(printf "%02x000000" $(test_seq 0 254)) &&
 	cat >expect.err <<-\EOF &&
 	error: commit-graph fanout values out of order
+	error: commit-graph required OID fanout chunk missing or corrupted
 	EOF
 	test_cmp expect.err err
 '
-- 
2.43.0.rc1.572.g273fc7bed6
Previous: Jeff KingNext: Jeff King
Message 15 of 39 in “some more chunk-file bounds-checks fixes”
  1. 0/9 some more chunk-file bounds-checks fixesJeff King, Nov 9, 2023
  2. 1/9 commit-graph: handle overflow in chunk_size checksJeff King, Nov 9, 2023
  3. Taylor BlauNov 9, 2023
  4. Jeff KingNov 9, 2023
  5. 2/9 midx: check consistency of fanout tableJeff King, Nov 9, 2023
  6. 3/9 commit-graph: drop redundant call to "lite" verificationJeff King, Nov 9, 2023
  7. 4/9 commit-graph: clarify missing-chunk error messagesJeff King, Nov 9, 2023
  8. 5/9 commit-graph: abort as soon as we see a bogus chunkJeff King, Nov 9, 2023
  9. Taylor BlauNov 9, 2023
  10. 6/9 commit-graph: use fanout value for graph sizeJeff King, Nov 9, 2023
  11. Taylor BlauNov 9, 2023
  12. Jeff KingNov 9, 2023
  13. Taylor BlauNov 9, 2023
  14. Jeff KingNov 10, 2023
  15. 7/9 commit-graph: check order while reading fanout chunkJeff King, Nov 9, 2023
  16. 8/9 commit-graph: drop verify_commit_graph_lite()Jeff King, Nov 9, 2023
  17. 9/9 commit-graph: mark chunk error messages for translationJeff King, Nov 9, 2023
  18. Taylor BlauNov 9, 2023
  19. 0/7 chunk-format: introduce `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  20. 1/7 chunk-format: introduce `pair_chunk_expect()` helperTaylor Blau, Nov 9, 2023
  21. Junio C HamanoNov 10, 2023
  22. Taylor BlauNov 10, 2023
  23. Jeff KingNov 10, 2023
  24. Junio C HamanoNov 10, 2023
  25. Junio C HamanoNov 10, 2023
  26. Jeff KingNov 10, 2023
  27. Jeff KingNov 10, 2023
  28. Jeff KingNov 10, 2023
  29. Linus ArverJan 15, 2024
  30. Linus ArverJan 15, 2024
  31. Jeff KingJan 16, 2024
  32. Linus ArverJan 18, 2024
  33. 2/7 commit-graph: read `OIDL` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  34. Jeff KingNov 10, 2023
  35. 3/7 commit-graph: read `CDAT` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  36. 4/7 commit-graph: read `GDAT` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  37. 5/7 commit-graph: read `BIDX` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  38. 6/7 midx: read `OIDL` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023
  39. 7/7 midx: read `OOFF` chunk with `pair_chunk_expect()`Taylor Blau, Nov 9, 2023

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.